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 <link rel="icon"> 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 <link> still falls back to
/favicon.ico.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

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 <link rel="icon"> 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