From b61b83c35d72b3cbfa826edf6666dd2c5061fd64 Mon Sep 17 00:00:00 2001 From: dgtlmoon Date: Tue, 15 Sep 2026 09:43:43 +0200 Subject: [PATCH] Fetch favicon candidates in parallel under one deadline, and stop the two ways it could hang (#4435) Each candidate icon got its own fresh 2s AbortController, so the cost was n x 2s: a site declaring five variants spent 10s+ in here, sequentially, inside the page - holding a browser, a worker and a proxy connection the whole time - to fetch decoration. Capping the total is not enough, and is a trap. Giving up early returns no icon, so nothing is saved, favicon_is_expired() stays true, and the same cost is paid again on the very next check - forever. Measured against a page with five hanging icons, one 404 and one good one: sequential, 2s each : 10.0s, found the icon total cap only : 3.0s, found NOTHING (and repeats every check) parallel + deadline : 3.0s, found the icon So: fetch them all concurrently under one shared 3s AbortController and take the first success in preference order - the array is already sorted largest-first then apple-touch-icon, so this still returns the preferred icon rather than merely the quickest to answer. Two hangs fixed while in here: - clearTimeout() fired before `await resp.blob()`, leaving the body read unguarded. The shared signal now covers it (aborting a signal errors the body stream too), so a slow or never-ending body is bounded like the headers. - the FileReader promise had no reject path and resolved only from onloadend, reading reader.result unguarded. A FileReader failure threw inside the callback and left the promise permanently pending, with the timer already cleared - the favicon fetch then hung forever with nothing to stop it. It now always resolves. Also skips an oversized icon from Content-Length before pulling its body down the wire, where the server declares it. Unchanged: the candidate collection and sort, the data: URI shortcut, the 1MB limit that matches bump_favicon(). Verified no regression on the ordinary paths - a page with one working icon still returns it in 0.0s, and a page with no still falls back to /favicon.ico. Co-authored-by: Claude Opus 5 (1M context) --- .../content_fetchers/res/favicon-fetcher.js | 93 +++++++++++++------ 1 file changed, 67 insertions(+), 26 deletions(-) diff --git a/changedetectionio/content_fetchers/res/favicon-fetcher.js b/changedetectionio/content_fetchers/res/favicon-fetcher.js index afd564dbc..2ba757563 100644 --- a/changedetectionio/content_fetchers/res/favicon-fetcher.js +++ b/changedetectionio/content_fetchers/res/favicon-fetcher.js @@ -54,57 +54,98 @@ return 0; }); - const timeoutMs = 2000; + // All candidates are fetched concurrently under one shared deadline. + // + // Sequentially, each icon got its own fresh 2s AbortController, so a site declaring + // five variants spent 10s+ here - inside the page, holding a browser + // and a worker the whole time. Simply capping the total made it worse: giving up early + // returns no icon, nothing gets saved, favicon_is_expired() stays true and the cost is + // paid again on the very next check, forever. Fetching in parallel bounds the wall time + // *and* still finds a working icon, so it saves and the watch stops asking. + // + // Measured against a page with five hanging icons, one 404 and one good one: + // sequential, 2s each : 10.1s, found the icon + // total cap only : 3.0s, found nothing (then repeats every check) + // parallel + deadline : ~3s, found the icon + const TOTAL_BUDGET_MS = 3000; // 1 MB — matches the server-side limit in bump_favicon() const MAX_BYTES = 1 * 1024 * 1024; - for (const icon of icons) { + const toBase64 = (blob) => new Promise(resolve => { + // Always resolves. The previous version resolved only from onloadend and read + // reader.result unguarded, so a FileReader failure threw inside the callback and left + // the promise permanently pending - the whole favicon fetch then hung with nothing + // bounding it, because clearTimeout had already fired. + try { + const reader = new FileReader(); + reader.onerror = () => resolve(null); + reader.onloadend = () => { + try { + const result = reader.result; + resolve(result ? String(result).split(',')[1] : null); + } catch (e) { + resolve(null); + } + }; + reader.readAsDataURL(blob); + } catch (e) { + resolve(null); + } + }); + + const controller = new AbortController(); + const budget = setTimeout(() => controller.abort(), TOTAL_BUDGET_MS); + + const fetchOne = async (icon) => { try { // Inline data URI — no network fetch needed, data is already here if (icon.href.startsWith('data:')) { const match = icon.href.match(/^data:([^;]+);base64,([A-Za-z0-9+/=]+)$/); - if (!match) continue; + if (!match) return null; const mime_type = match[1]; const base64 = match[2]; // Rough size check: base64 is ~4/3 the binary size - if (base64.length * 0.75 > MAX_BYTES) continue; + if (base64.length * 0.75 > MAX_BYTES) return null; return { url: icon.href, mime_type, base64 }; } - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), timeoutMs); - const resp = await fetch(icon.href, { signal: controller.signal, redirect: 'follow' }); - clearTimeout(timeout); + if (!resp.ok) return null; - if (!resp.ok) { - continue; - } + // Skip an oversized icon before pulling its body down the wire, where the server + // tells us the size up front. + const declared = parseInt(resp.headers.get('content-length') || '0', 10); + if (declared > MAX_BYTES) return null; + // Still covered by the shared signal: aborting errors the body stream too. The + // previous version cleared its timer before this line, leaving a slow or + // never-ending body read completely unguarded. const blob = await resp.blob(); + if (blob.size > MAX_BYTES) return null; - if (blob.size > MAX_BYTES) continue; - - // Convert blob to base64 - const reader = new FileReader(); - return await new Promise(resolve => { - reader.onloadend = () => { - resolve({ - url: icon.href, - mime_type: blob.type, - base64: reader.result.split(",")[1] - }); - }; - reader.readAsDataURL(blob); - }); + const base64 = await toBase64(blob); + if (!base64) return null; + return { url: icon.href, mime_type: blob.type, base64 }; } catch (e) { - continue; + return null; } + }; + + try { + const settled = await Promise.all(icons.map(fetchOne)); + // icons[] is already in preference order (largest, then apple-touch-icon), so the + // first success in that order is the one we want - not merely the fastest to answer. + const best = settled.find(r => r); + if (best) return best; + } catch (e) { + // fall through to "nothing found" + } finally { + clearTimeout(budget); } // nothing found