From cc26cb9a9ed9c12f606badc0179582e3c6149397 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 7 Aug 2026 08:38:02 -0500 Subject: [PATCH 1/7] Instrument urls built from a JavaScript undefined (BL-16666) Sentry has years of "Cannot Find File" reports for paths like Temp/undefined. The old card framed these as C# building a temp path from a name that was undefined; that is wrong, and a guard there would catch nothing. What happens is that some front-end code does the equivalent of `element.src = aVariableThatIsUndefined`: the DOM turns the value into the text "undefined" and asks the server for a file by that name. The server can only report that a file called "undefined" is missing, which never says who asked - so finding BL-16447, the one instance we have fixed, took a lot of guessing. This does not fix any particular instance. It makes the next one name itself, in two layers. Server side: carry the HTTP Referer through to the report, so a bogus request names the page that issued it. "undefined", "null" and "NaN" are matched as a whole path segment, case-sensitively, so a real file called undefined.png or a folder called undefinedThings is untouched. We also stop suppressing these reports when the path is under the current book folder. That suppression is what has been hiding most instances, since a bogus url built by a book page resolves inside that book's folder - so expect more of these reports, not fewer. That is the intent; they are now actionable. Front end: a new lib/undefinedUrlDetector, installed from lib/errorHandler because every bundle's root module already imports it. It intercepts src assignment, setAttribute("src"), fetch and XMLHttpRequest.open and reports with a JavaScript stack, which names the offending line. It is deliberately passive - it reports and lets the assignment proceed - and reports each distinct problem once, capped per session, because these bugs live in components that re-render. Two things worth knowing for anyone changing this later. The check takes the value rather than a string because at `img.src = x` the value arrives as the real undefined; the DOM stringifies it inside the native setter, and by the time a string exists it is too late to know who set it. And fetch/XHR needed wrapping because BL-16447 came in through WaveSurfer fetching the url rather than putting it on an element. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomBrowserUI/lib/errorHandler.ts | 6 + .../lib/undefinedUrlDetector.ts | 187 ++++++++++++++++++ .../lib/undefinedUrlDetectorSpec.ts | 150 ++++++++++++++ src/BloomExe/web/BloomHttpListenerContext.cs | 12 ++ src/BloomExe/web/BloomServer.cs | 70 ++++++- src/BloomExe/web/IRequestInfo.cs | 7 + src/BloomExe/web/RequestInfo.cs | 5 + src/BloomTests/PretendRequestInfo.cs | 9 +- src/BloomTests/web/BloomServerTests.cs | 70 +++++++ src/BloomTests/web/RequestInfoTests.cs | 1 + 10 files changed, 511 insertions(+), 6 deletions(-) create mode 100644 src/BloomBrowserUI/lib/undefinedUrlDetector.ts create mode 100644 src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts diff --git a/src/BloomBrowserUI/lib/errorHandler.ts b/src/BloomBrowserUI/lib/errorHandler.ts index 1a4a96bb3934..95adf9adeede 100644 --- a/src/BloomBrowserUI/lib/errorHandler.ts +++ b/src/BloomBrowserUI/lib/errorHandler.ts @@ -1,5 +1,6 @@ import * as StackTrace from "stacktrace-js"; import Axios from "axios"; +import { installUndefinedUrlDetector } from "./undefinedUrlDetector"; // This file implements custom Bloom global error handling. // It should be imported by the root module in each bundle. @@ -156,6 +157,11 @@ if (typeof window !== "undefined") { }); return true; // suppress normal handling. }; + + // Watch for urls built out of a JavaScript value that wasn't ready. Installed here because + // every bundle's root module imports this file, which is exactly the coverage we want: the + // bug can be in any of our pages, and the server can't tell us which one. See BL-16577. + installUndefinedUrlDetector(reportError); } // Saving this as it MIGHT be useful if we decide to have another go at catching diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts new file mode 100644 index 000000000000..8f68daa48b6c --- /dev/null +++ b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts @@ -0,0 +1,187 @@ +// Catches the moment a piece of our front-end code puts a JavaScript value it doesn't actually +// have into a url. +// +// `element.src = someUndefinedVariable` doesn't fail: the DOM turns the value into the *text* +// "undefined" and asks the server for a file by that name. The server can only report that some +// file called "undefined" is missing, which tells us nothing about who asked for it - so these +// have sat in Sentry for years without ever being traceable to a line of code (BL-16577). We +// already fixed one instance the hard way (BL-16447, where the Adjust Timings dialog rendered +// before its audio url was ready); this exists so the next one names itself. +// +// The interception below is deliberately passive: it reports and then lets the assignment +// proceed exactly as before. What we need from it is the stack, and we get that either way, so +// there is no reason to change what the app does. + +/// The exact strings JavaScript produces when these values are turned into text. We match them +/// case-sensitively and only as a whole url or a whole path segment, so a real file that happens +/// to be called "Undefined.png", or a folder called "undefinedThings", is left alone. +const javascriptValueStrings = ["undefined", "null", "NaN"]; + +/** + * True if this will become a url that is, or contains a path segment that is, the text form of a + * JavaScript value. Nothing legitimately loads such a url, so a true here is always one of our bugs. + * + * Takes the value rather than a string on purpose. At an assignment like `img.src = x` the value + * arrives here as the real `undefined`; it is the DOM that turns it into the text "undefined" a + * moment later. By the time anyone could see a string it is already too late to know who did it. + */ +export function isJavascriptValueUrl(url: unknown): boolean { + // These are exactly the values that stringify to the text we are looking for. + if (url === undefined || url === null) return true; + if (typeof url === "number") return Number.isNaN(url); + // Anything else that isn't a string (a URL object, say) stringifies to something real. + if (typeof url !== "string" || url.length === 0) return false; + + // Ignore any query string or fragment; only the path can name a file. + const path = url.split(/[?#]/)[0]; + return path + .split(/[/\\]/) + .some((segment) => javascriptValueStrings.includes(segment)); +} + +/** + * What we send to the server. Kept separate from the interception so it can be tested, and so the + * message reads the same wherever it was caught. + */ +export function describeBadUrl(url: unknown, whereItWasSet: string): string { + return ( + `A url was built from a JavaScript value that wasn't ready: ${whereItWasSet} was set to "${String(url)}". ` + + `Bloom will now ask the server for a file by that name and fail. See BL-16577.` + ); +} + +type Reporter = (message: string, stack: string | undefined) => void; + +let installed = false; + +// Each report is an http post to the server, and the bug that triggers it is typically a render +// that repeats - the Adjust Timings dialog in BL-16447 would have fired on every open. We only +// need to learn about each site once, so report a given message once and stop altogether after a +// handful. Losing the repeat count costs us nothing: the point is to identify the code, and the +// server-side count in Sentry already tells us how often it happens. +const maxReports = 10; +const alreadyReported = new Set(); + +/** Exported only so tests can start from a clean slate. */ +export function resetReportingForTests(): void { + alreadyReported.clear(); +} + +/** + * Installs the interception. Safe to call more than once; only the first call does anything, so a + * bundle that pulls in the bootstrap twice doesn't end up double-reporting. + * + * @param report how to get the problem back to us - errorHandler's reportError in real use. + */ +export function installUndefinedUrlDetector(report: Reporter): void { + if (installed || typeof window === "undefined") return; + installed = true; + + const check = (url: unknown, whereItWasSet: string) => { + if (!isJavascriptValueUrl(url)) return; + const message = describeBadUrl(url, whereItWasSet); + if (alreadyReported.has(message) || alreadyReported.size >= maxReports) + return; + alreadyReported.add(message); + // The stack of *this* call is the whole point: it names the line that built the url. + report(message, new Error().stack); + }; + + guardSrcProperty(window.HTMLImageElement, "an image's src", check); + guardSrcProperty(window.HTMLMediaElement, "an audio/video src", check); + guardSrcProperty(window.HTMLIFrameElement, "an iframe's src", check); + guardSrcProperty(window.HTMLScriptElement, "a script's src", check); + guardSetAttribute(check); + guardFetch(check); + guardXmlHttpRequest(check); +} + +/** + * Wraps the `src` property of one element type. We call the original setter afterwards, so the + * element behaves exactly as it did before; we are only listening. + */ +function guardSrcProperty( + elementType: { prototype: object } | undefined, + description: string, + check: (url: unknown, whereItWasSet: string) => void, +): void { + // Not every environment has every element type (jsdom, for one), and a browser could in + // principle define src without a setter. Nothing to wrap in that case. + if (!elementType) return; + const original = Object.getOwnPropertyDescriptor( + elementType.prototype, + "src", + ); + if (!original || !original.set) return; + + Object.defineProperty(elementType.prototype, "src", { + ...original, + set(value: unknown) { + check(value, description); + original.set!.call(this, value); + }, + }); +} + +function guardSetAttribute( + check: (url: unknown, whereItWasSet: string) => void, +): void { + const originalSetAttribute = Element.prototype.setAttribute; + Element.prototype.setAttribute = function ( + name: string, + value: string, + ): void { + // Only src: an of "undefined" is a dead link rather than a failed request, and + // is not what we are hunting. + if (name === "src") check(value, `setAttribute("src")`); + originalSetAttribute.call(this, name, value); + }; +} + +/** + * The BL-16447 case came in through a library (WaveSurfer) fetching the url rather than putting it + * on an element, so element interception alone would have missed it. + */ +function guardFetch( + check: (url: unknown, whereItWasSet: string) => void, +): void { + if (typeof window.fetch !== "function") return; + const originalFetch = window.fetch; + window.fetch = function (input: RequestInfo | URL, init?: RequestInit) { + check(urlOfFetchInput(input), "a fetch()"); + // Call it on `window`, not on `this`. A bare `fetch(url)` inside a module gives us + // `this === undefined`, and the browser rejects fetch invoked on anything that isn't the + // window ("Illegal invocation") - which would break every fetch in Bloom. + return originalFetch.call(window, input, init); + }; +} + +function urlOfFetchInput(input: RequestInfo | URL): string | undefined { + if (typeof input === "string") return input; + if (input instanceof URL) return input.href; + // A Request object + if (input && typeof (input as Request).url === "string") + return (input as Request).url; + return undefined; +} + +function guardXmlHttpRequest( + check: (url: unknown, whereItWasSet: string) => void, +): void { + if (typeof XMLHttpRequest !== "function") return; + const originalOpen = XMLHttpRequest.prototype.open; + // Deliberately untyped rest args: open() has two overloads and we only care about the url. + XMLHttpRequest.prototype.open = function ( + method: string, + url: string | URL, + ...rest: unknown[] + ) { + check(typeof url === "string" ? url : url?.href, "an XMLHttpRequest"); + return (originalOpen as (...args: unknown[]) => void).call( + this, + method, + url, + ...rest, + ); + }; +} diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts b/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts new file mode 100644 index 000000000000..da1284c8b460 --- /dev/null +++ b/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts @@ -0,0 +1,150 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { + describeBadUrl, + installUndefinedUrlDetector, + isJavascriptValueUrl, + resetReportingForTests, +} from "./undefinedUrlDetector"; + +describe("isJavascriptValueUrl", () => { + it("catches a url that is nothing but the value", () => { + expect(isJavascriptValueUrl("undefined")).toBe(true); + expect(isJavascriptValueUrl("null")).toBe(true); + expect(isJavascriptValueUrl("NaN")).toBe(true); + }); + + it("catches the value as a path segment, which is the shape we see in Sentry", () => { + // The real reported paths look like this, because a bare "undefined" src resolves + // against a document whose base is the temp folder. + expect( + isJavascriptValueUrl( + "C:/Users/someone/AppData/Local/Temp/undefined", + ), + ).toBe(true); + expect(isJavascriptValueUrl("audio/undefined")).toBe(true); + expect(isJavascriptValueUrl("undefined/audio/abc.mp3")).toBe(true); + }); + + it("ignores the query string and fragment", () => { + expect(isJavascriptValueUrl("audio/undefined?assetv=1")).toBe(true); + expect(isJavascriptValueUrl("audio/real.mp3?name=undefined")).toBe( + false, + ); + expect(isJavascriptValueUrl("page.htm#undefined")).toBe(false); + }); + + it("catches the values themselves, which is what an assignment actually passes", () => { + // `img.src = x` hands us the real value; the DOM stringifies it afterwards. Catching it + // here is the only moment at which we still know who set it. + expect(isJavascriptValueUrl(undefined)).toBe(true); + expect(isJavascriptValueUrl(null)).toBe(true); + expect(isJavascriptValueUrl(Number.NaN)).toBe(true); + }); + + it("leaves real files and folders alone", () => { + expect(isJavascriptValueUrl("audio/abc123.mp3")).toBe(false); + expect(isJavascriptValueUrl("undefined.png")).toBe(false); + expect(isJavascriptValueUrl("undefinedThings/x.png")).toBe(false); + expect(isJavascriptValueUrl("images/Undefined")).toBe(false); + expect(isJavascriptValueUrl("")).toBe(false); + // A real number or a URL object stringifies to something legitimate. + expect(isJavascriptValueUrl(17)).toBe(false); + expect(isJavascriptValueUrl(new URL("http://x/y.png"))).toBe(false); + }); +}); + +describe("describeBadUrl", () => { + it("names both the value and where it was set", () => { + const message = describeBadUrl(undefined, "an image's src"); + expect(message).toContain("an image's src"); + expect(message).toContain('"undefined"'); + expect(message).toContain("BL-16577"); + }); +}); + +describe("installUndefinedUrlDetector", () => { + // Installed once for the whole file, as it is in the real app: the installer deliberately + // ignores repeat calls so a bundle can't double-report. + const report = vi.fn(); + installUndefinedUrlDetector(report); + + beforeEach(() => { + report.mockClear(); + // Each test would otherwise be silenced by the previous test's identical message. + resetReportingForTests(); + }); + + it("reports an image src assigned an undefined variable, with a stack", () => { + report.mockClear(); + const image = document.createElement("img"); + let notReadyYet: string | undefined; + + image.src = notReadyYet as unknown as string; + + expect(report).toHaveBeenCalledTimes(1); + const [message, stack] = report.mock.calls[0]; + expect(message).toContain("an image's src"); + // The stack is the entire point of this layer: it is what names the offending line. + expect(stack).toBeTruthy(); + expect(stack).toContain("undefinedUrlDetectorSpec"); + }); + + it("still actually sets the src, so nothing behaves differently", () => { + report.mockClear(); + const image = document.createElement("img"); + + image.src = "audio/real.mp3"; + + expect(report).not.toHaveBeenCalled(); + expect(image.getAttribute("src")).toBe("audio/real.mp3"); + }); + + it("reports setAttribute('src', undefined) too", () => { + report.mockClear(); + const image = document.createElement("img"); + + image.setAttribute("src", undefined as unknown as string); + + expect(report).toHaveBeenCalledTimes(1); + expect(report.mock.calls[0][0]).toContain('setAttribute("src")'); + }); + + it("leaves other attributes alone", () => { + report.mockClear(); + const div = document.createElement("div"); + + div.setAttribute("title", "undefined"); + + expect(report).not.toHaveBeenCalled(); + expect(div.getAttribute("title")).toBe("undefined"); + }); + + it("reports a repeating bug only once, so a re-rendering component can't flood the server", () => { + const image = document.createElement("img"); + + image.src = undefined as unknown as string; + image.src = undefined as unknown as string; + image.src = undefined as unknown as string; + + expect(report).toHaveBeenCalledTimes(1); + }); + + it("reports a fetch of an undefined url, which is how BL-16447 escaped", () => { + // Called bare, exactly as application code calls it - which means the wrapper receives + // `this === undefined` and must not pass that on to the real fetch. + void fetch("audio/undefined").catch(() => { + // jsdom has no real network; the rejection is expected and irrelevant here. + }); + + expect(report).toHaveBeenCalledTimes(1); + expect(report.mock.calls[0][0]).toContain("a fetch()"); + }); + + it("leaves an ordinary fetch alone", () => { + void fetch("audio/real.mp3").catch(() => { + // Again, no network in jsdom. + }); + + expect(report).not.toHaveBeenCalled(); + }); +}); diff --git a/src/BloomExe/web/BloomHttpListenerContext.cs b/src/BloomExe/web/BloomHttpListenerContext.cs index 95fef614dd57..c59952ded9ba 100644 --- a/src/BloomExe/web/BloomHttpListenerContext.cs +++ b/src/BloomExe/web/BloomHttpListenerContext.cs @@ -42,6 +42,13 @@ public interface IHttpListenerRequest string HttpMethod { get; } System.IO.Stream InputStream { get; } string RawUrl { get; } + + /// + /// The url of the document that asked for this, or null if the browser didn't say. + /// Only diagnostics use this: when a request is obviously bogus, the referrer is what + /// tells us which of our pages produced it. See BL-16577. + /// + string Referer { get; } System.Uri Url { get; } } @@ -64,6 +71,11 @@ public string ContentType get { return _actualRequest.ContentType; } } + public string Referer + { + get { return _actualRequest.Headers["Referer"]; } + } + public bool HasEntityBody { get { return _actualRequest.HasEntityBody; } diff --git a/src/BloomExe/web/BloomServer.cs b/src/BloomExe/web/BloomServer.cs index 0c4bd8b98921..68ec7315f6fe 100644 --- a/src/BloomExe/web/BloomServer.cs +++ b/src/BloomExe/web/BloomServer.cs @@ -1427,7 +1427,11 @@ private bool ProcessAnyFileContent(IRequestInfo info, string localPath) { if (ShouldReportFailedRequest(info, CurrentBook?.FolderPath)) { - ReportMissingFile(localPath, path); + ReportMissingFile( + localPath, + path, + GetJavascriptValueDiagnostics(info, localPath) + ); } return false; // from here we head off to BloomServer.MakeReply() which now uses the same ShouldReportFailedRequest() method. } @@ -1441,7 +1445,11 @@ private bool IsAudioFileWhichCanHaveCompressedCounterpart(string path) return path.EndsWith($".{AudioRecording.kRecordableExtension}"); } - private static void ReportMissingFile(string localPath, string path) + private static void ReportMissingFile( + string localPath, + string path, + string extraDiagnostics = "" + ) { if (path == null) { @@ -1472,9 +1480,10 @@ private static void ReportMissingFile(string localPath, string path) "Cannot Find Image File" ); var detailMsg = String.Format( - "Server could not find the image file {0}. LocalPath was {1}{2}", + "Server could not find the image file {0}. LocalPath was {1}{2}{3}", path, localPath, + extraDiagnostics, Environment.NewLine ); NonFatalProblem.Report(ModalIf.None, PassiveIf.All, userMsg, detailMsg); @@ -1487,15 +1496,54 @@ private static void ReportMissingFile(string localPath, string path) "Cannot Find File" ); var detailMsg = String.Format( - "Server could not find the file {0}. LocalPath was {1}{2}", + "Server could not find the file {0}. LocalPath was {1}{2}{3}", path, localPath, + extraDiagnostics, Environment.NewLine ); NonFatalProblem.Report(ModalIf.Beta, PassiveIf.All, userMsg, detailMsg); } } + /// + /// True if any part of the requested path is literally "undefined", "null" or "NaN" - what a + /// JavaScript value of that kind becomes when something puts it into a url without checking. + /// No real file is named any of those, so such a request is always a front-end bug rather + /// than a genuinely missing file. See BL-16577, and BL-16447 for one instance we have fixed. + /// + internal static bool LooksLikeAJavascriptValueInAUrl(string localPath) + { + if (String.IsNullOrEmpty(localPath)) + return false; + foreach (var segment in localPath.Split('/', '\\')) + { + // Not case-insensitive: JavaScript produces exactly these spellings, and being + // strict keeps us from flagging a book or file someone really did call "Undefined". + if (segment == "undefined" || segment == "null" || segment == "NaN") + return true; + } + return false; + } + + /// + /// The extra detail we attach when the request looks like the JavaScript-value bug above. + /// The referrer is the whole point: it names the page that issued the bogus request, which + /// is what lets us find the code responsible. See BL-16577. + /// + private static string GetJavascriptValueDiagnostics(IRequestInfo info, string localPath) + { + if (!LooksLikeAJavascriptValueInAUrl(localPath)) + return ""; + return String.Format( + "{0}Part of this path is a JavaScript value turned into text, so some of our front-end code built a url out of something it did not have yet." + + " The page that asked for it was {1}. The raw url was {2}.", + Environment.NewLine, + String.IsNullOrEmpty(info.Referer) ? "(the browser did not say)" : info.Referer, + info.RawUrl + ); + } + private static bool IsSimulatedFileUrl(string localPath) { var extension = Path.GetExtension(localPath); @@ -2162,7 +2210,12 @@ internal async Task MakeReplyAsync(IRequestInfo info) private void ReportMissingFile(IRequestInfo info) { var localPath = GetLocalPathWithoutQuery(info); - Logger.WriteEvent("**{0}: File Missing: {1}", GetType().Name, localPath); + Logger.WriteEvent( + "**{0}: File Missing: {1}{2}", + GetType().Name, + localPath, + GetJavascriptValueDiagnostics(info, localPath) + ); } /// @@ -2187,6 +2240,13 @@ protected bool ShouldReportFailedRequest( return false; var localPath = GetLocalPathWithoutQuery(info); + + // A path containing "undefined"/"null"/"NaN" is one of our own bugs, never a file the + // user is responsible for, so none of the suppressions below should hide it - and the + // book-folder one in particular was hiding most of them, since a bogus url built by a + // book page resolves inside that book's folder. See BL-16577. + if (LooksLikeAJavascriptValueInAUrl(localPath)) + return true; var localFolderTestPath = localPath; // We don't need even a toast for missing files in the book folder. That's the user's problem // and should be adequately documented by the browser message saying the file is missing. diff --git a/src/BloomExe/web/IRequestInfo.cs b/src/BloomExe/web/IRequestInfo.cs index 8f4103e55882..763d77cd5eb2 100644 --- a/src/BloomExe/web/IRequestInfo.cs +++ b/src/BloomExe/web/IRequestInfo.cs @@ -20,6 +20,13 @@ public interface IRequestInfo string RequestContentType { get; } string ResponseContentType { set; } string RawUrl { get; } + + /// + /// The url of the document that asked for this, or null if the browser didn't say. + /// Diagnostics only - it tells us which of our pages issued a request we can't satisfy. + /// See BL-16577. + /// + string Referer { get; } bool HaveFullyProcessedRequest { get; } void WriteCompleteOutput(string s); void ReplyWithFileContent(string path, string originalPath = null); diff --git a/src/BloomExe/web/RequestInfo.cs b/src/BloomExe/web/RequestInfo.cs index ec18b134228c..8d3d3c1b05df 100644 --- a/src/BloomExe/web/RequestInfo.cs +++ b/src/BloomExe/web/RequestInfo.cs @@ -653,6 +653,11 @@ public string RawUrl get { return _actualContext.Request.RawUrl; } } + public string Referer + { + get { return _actualContext.Request.Referer; } + } + HttpMethods IRequestInfo.HttpMethod { get diff --git a/src/BloomTests/PretendRequestInfo.cs b/src/BloomTests/PretendRequestInfo.cs index 17c0e3f53e8a..2466aad53e40 100644 --- a/src/BloomTests/PretendRequestInfo.cs +++ b/src/BloomTests/PretendRequestInfo.cs @@ -25,10 +25,12 @@ public PretendRequestInfo( string url, HttpMethods httpMethod = HttpMethods.Get, bool forPrinting = false, - bool forSrcAttr = false + bool forSrcAttr = false, + string referer = null ) { HttpMethod = httpMethod; + Referer = referer; if (forPrinting) url = url.Replace("/bloom/", "/bloom/OriginalImages/"); @@ -47,6 +49,11 @@ public PretendRequestInfo( public string LocalPathWithoutQuery { get; set; } + /// + /// The url of the document that made the request, as the constructor was told it. + /// + public string Referer { get; } + public string RequestContentType { get; } public string ResponseContentType { private get; set; } diff --git a/src/BloomTests/web/BloomServerTests.cs b/src/BloomTests/web/BloomServerTests.cs index 310265d7af1c..e7032e101fcb 100644 --- a/src/BloomTests/web/BloomServerTests.cs +++ b/src/BloomTests/web/BloomServerTests.cs @@ -190,6 +190,76 @@ public void ReportsMissingFile() } } + /// + /// A url containing the literal text "undefined" means some front-end code built it out of a + /// JavaScript value it did not have yet - always our bug, never a file the user is missing. + /// The referrer is what tells us which page did it, so it has to reach the report. BL-16577. + /// + [Test] + public void ReportsMissingFile_UndefinedInPath_LogsTheReferringPage() + { + using (var server = CreateBloomServer()) + { + var transaction = new PretendRequestInfo( + BloomServer.ServerUrlWithBloomPrefixEndingInSlash + "audio/undefined", + referer: "http://localhost:8089/bloom/SomeBook/SomePage.htm" + ); + + server.MakeReply(transaction); + + Assert.That( + Logger.LogText, + Contains.Substring("SomeBook/SomePage.htm"), + "The referring page should be in the report; without it we cannot tell which of our pages built the bad url." + ); + Assert.That( + Logger.LogText, + Contains.Substring("JavaScript value turned into text") + ); + } + } + + /// + /// The referrer header is optional, so the diagnostics must still say something useful (and + /// not crash) when the browser doesn't send one. + /// + [Test] + public void ReportsMissingFile_UndefinedInPathAndNoReferer_StillReports() + { + using (var server = CreateBloomServer()) + { + var transaction = new PretendRequestInfo( + BloomServer.ServerUrlWithBloomPrefixEndingInSlash + "undefined" + ); + + server.MakeReply(transaction); + + Assert.That(Logger.LogText, Contains.Substring("the browser did not say")); + } + } + + [TestCase("audio/undefined", true)] + [TestCase("undefined", true)] + [TestCase("C:/Users/joe/AppData/Local/Temp/undefined", true)] + [TestCase("images/null", true)] + [TestCase("pages/NaN.htm", false, Description = "NaN.htm is a filename, not a bare value")] + [TestCase("undefined.png", false, Description = "so is undefined.png")] + [TestCase( + "audio/Undefined", + false, + Description = "JavaScript never produces this spelling" + )] + [TestCase("undefinedThings/x.png", false)] + [TestCase("myBook/audio/abc123.mp3", false)] + [TestCase("", false)] + public void LooksLikeAJavascriptValueInAUrl_Works(string localPath, bool expected) + { + Assert.That( + BloomServer.LooksLikeAJavascriptValueInAUrl(localPath), + Is.EqualTo(expected) + ); + } + [Test] public void SupportsHandlerInjection() { diff --git a/src/BloomTests/web/RequestInfoTests.cs b/src/BloomTests/web/RequestInfoTests.cs index b46b2c9bc1b3..804618121a7b 100644 --- a/src/BloomTests/web/RequestInfoTests.cs +++ b/src/BloomTests/web/RequestInfoTests.cs @@ -120,6 +120,7 @@ private class TestHttpListenerRequest : IHttpListenerRequest public string HttpMethod { get; private set; } public Stream InputStream { get; private set; } public string RawUrl { get; private set; } + public string Referer { get; set; } public Uri Url { get; private set; } public void SetRawUrl(string rawUrl) From 0585c64d454b7ae3207501efc234bd2e9b169605 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 7 Aug 2026 08:39:20 -0500 Subject: [PATCH 2/7] Point the code comments at the new card, BL-16666 Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomBrowserUI/lib/undefinedUrlDetector.ts | 4 ++-- src/BloomExe/web/BloomHttpListenerContext.cs | 2 +- src/BloomExe/web/BloomServer.cs | 6 +++--- src/BloomExe/web/IRequestInfo.cs | 2 +- src/BloomTests/web/BloomServerTests.cs | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts index 8f68daa48b6c..f0a8be2de4cb 100644 --- a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts +++ b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts @@ -4,7 +4,7 @@ // `element.src = someUndefinedVariable` doesn't fail: the DOM turns the value into the *text* // "undefined" and asks the server for a file by that name. The server can only report that some // file called "undefined" is missing, which tells us nothing about who asked for it - so these -// have sat in Sentry for years without ever being traceable to a line of code (BL-16577). We +// have sat in Sentry for years without ever being traceable to a line of code (BL-16666, split from BL-16577). We // already fixed one instance the hard way (BL-16447, where the Adjust Timings dialog rendered // before its audio url was ready); this exists so the next one names itself. // @@ -46,7 +46,7 @@ export function isJavascriptValueUrl(url: unknown): boolean { export function describeBadUrl(url: unknown, whereItWasSet: string): string { return ( `A url was built from a JavaScript value that wasn't ready: ${whereItWasSet} was set to "${String(url)}". ` + - `Bloom will now ask the server for a file by that name and fail. See BL-16577.` + `Bloom will now ask the server for a file by that name and fail. See BL-16666.` ); } diff --git a/src/BloomExe/web/BloomHttpListenerContext.cs b/src/BloomExe/web/BloomHttpListenerContext.cs index c59952ded9ba..3e753731bac2 100644 --- a/src/BloomExe/web/BloomHttpListenerContext.cs +++ b/src/BloomExe/web/BloomHttpListenerContext.cs @@ -46,7 +46,7 @@ public interface IHttpListenerRequest /// /// The url of the document that asked for this, or null if the browser didn't say. /// Only diagnostics use this: when a request is obviously bogus, the referrer is what - /// tells us which of our pages produced it. See BL-16577. + /// tells us which of our pages produced it. See BL-16666. /// string Referer { get; } System.Uri Url { get; } diff --git a/src/BloomExe/web/BloomServer.cs b/src/BloomExe/web/BloomServer.cs index 68ec7315f6fe..60bab062a629 100644 --- a/src/BloomExe/web/BloomServer.cs +++ b/src/BloomExe/web/BloomServer.cs @@ -1510,7 +1510,7 @@ private static void ReportMissingFile( /// True if any part of the requested path is literally "undefined", "null" or "NaN" - what a /// JavaScript value of that kind becomes when something puts it into a url without checking. /// No real file is named any of those, so such a request is always a front-end bug rather - /// than a genuinely missing file. See BL-16577, and BL-16447 for one instance we have fixed. + /// than a genuinely missing file. See BL-16666 (split from BL-16577), and BL-16447 for one instance we have fixed. /// internal static bool LooksLikeAJavascriptValueInAUrl(string localPath) { @@ -1529,7 +1529,7 @@ internal static bool LooksLikeAJavascriptValueInAUrl(string localPath) /// /// The extra detail we attach when the request looks like the JavaScript-value bug above. /// The referrer is the whole point: it names the page that issued the bogus request, which - /// is what lets us find the code responsible. See BL-16577. + /// is what lets us find the code responsible. See BL-16666. /// private static string GetJavascriptValueDiagnostics(IRequestInfo info, string localPath) { @@ -2244,7 +2244,7 @@ protected bool ShouldReportFailedRequest( // A path containing "undefined"/"null"/"NaN" is one of our own bugs, never a file the // user is responsible for, so none of the suppressions below should hide it - and the // book-folder one in particular was hiding most of them, since a bogus url built by a - // book page resolves inside that book's folder. See BL-16577. + // book page resolves inside that book's folder. See BL-16666. if (LooksLikeAJavascriptValueInAUrl(localPath)) return true; var localFolderTestPath = localPath; diff --git a/src/BloomExe/web/IRequestInfo.cs b/src/BloomExe/web/IRequestInfo.cs index 763d77cd5eb2..2b6412a4a6f1 100644 --- a/src/BloomExe/web/IRequestInfo.cs +++ b/src/BloomExe/web/IRequestInfo.cs @@ -24,7 +24,7 @@ public interface IRequestInfo /// /// The url of the document that asked for this, or null if the browser didn't say. /// Diagnostics only - it tells us which of our pages issued a request we can't satisfy. - /// See BL-16577. + /// See BL-16666. /// string Referer { get; } bool HaveFullyProcessedRequest { get; } diff --git a/src/BloomTests/web/BloomServerTests.cs b/src/BloomTests/web/BloomServerTests.cs index e7032e101fcb..d1ae1433317c 100644 --- a/src/BloomTests/web/BloomServerTests.cs +++ b/src/BloomTests/web/BloomServerTests.cs @@ -193,7 +193,7 @@ public void ReportsMissingFile() /// /// A url containing the literal text "undefined" means some front-end code built it out of a /// JavaScript value it did not have yet - always our bug, never a file the user is missing. - /// The referrer is what tells us which page did it, so it has to reach the report. BL-16577. + /// The referrer is what tells us which page did it, so it has to reach the report. BL-16666. /// [Test] public void ReportsMissingFile_UndefinedInPath_LogsTheReferringPage() From 284dde3cab440dded7ec458dc82ca15d0935104d Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 7 Aug 2026 08:52:51 -0500 Subject: [PATCH 3/7] Don't interrupt users with our own url bug, and fix a stale test (BL-16666) Two things Devin caught on the first pass. Keep the new reports quiet. ShouldReportFailedRequest deliberately stopped suppressing requests whose path contains a JavaScript value, so that we finally see them. But the report they fall through to uses PassiveIf.All and ModalIf.Beta, which means a toast on every channel and a modal on beta - so un-suppressing them would have started interrupting ordinary users on pages that previously failed silently. There is nothing a user can do about our bug, so this class now reports with ModalIf.None/PassiveIf.None: still logged and still sent to Sentry, but invisible to the user. It also gets its own short message, so it forms its own Sentry issue instead of being buried in the general "Cannot Find File" one. Fix the spec assertion. When the card references were repointed from BL-16577 to BL-16666, the message in describeBadUrl changed but the spec's assertion did not, so that test could never pass. The C# suite was re-run after that rename but the front-end suite was not, which is exactly how it slipped through. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/undefinedUrlDetectorSpec.ts | 2 +- src/BloomExe/web/BloomServer.cs | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts b/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts index da1284c8b460..268ffdf0400a 100644 --- a/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts +++ b/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts @@ -58,7 +58,7 @@ describe("describeBadUrl", () => { const message = describeBadUrl(undefined, "an image's src"); expect(message).toContain("an image's src"); expect(message).toContain('"undefined"'); - expect(message).toContain("BL-16577"); + expect(message).toContain("BL-16666"); }); }); diff --git a/src/BloomExe/web/BloomServer.cs b/src/BloomExe/web/BloomServer.cs index 1fb433707d68..34bb26104100 100644 --- a/src/BloomExe/web/BloomServer.cs +++ b/src/BloomExe/web/BloomServer.cs @@ -1459,6 +1459,29 @@ private static void ReportMissingFile( path = "(was null)"; } + // A url containing a JavaScript value is our bug, and there is nothing the user can do + // about it, so record it for ourselves without interrupting them. This matters because + // ShouldReportFailedRequest deliberately stopped suppressing these: without this branch, + // un-suppressing them would start showing "Cannot Find File" toasts (and, on beta, a + // modal) to users on pages that previously failed silently. The distinct message also + // gives this class its own Sentry issue rather than burying it in the general one. + // See BL-16666. + if (LooksLikeAJavascriptValueInAUrl(localPath)) + { + NonFatalProblem.Report( + ModalIf.None, + PassiveIf.None, + "Url built from a JavaScript value", + String.Format( + "Server could not find the file {0}. LocalPath was {1}{2}", + path, + localPath, + extraDiagnostics + ) + ); + return; + } + // we have any number of incidences where something asks for a page after we've navigated from it. E.g. BL-3715, BL-3769. // I suspect our disposal algorithm is just flawed: the page is removed from the _url cache as soon as we navigated away, // which is too soon. But that will take more research and we're trying to finish 3.7. From c2438fb07874b52598ac7b981475831846fff061 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 7 Aug 2026 09:00:50 -0500 Subject: [PATCH 4/7] Actually include both kinds of detail in the report (BL-16666) Devin caught that the merge left a contradiction. The JavaScript-value branch returns early, so by the time control reaches the later branches extraDiagnostics is always empty - which meant the "both kinds of detail" comment was false, the bare-name detail never reached a report about a JavaScript-value url, and the general branch had gained an argument that could never be filled. Now the early branch emits both (a bare "undefined" genuinely has no directory AND is a JavaScript value), and the later branches ask only for the bare-name detail, which is the only one they can ever have. Also records what the front-end detector does and does not cover: it patches the prototypes of the window it is installed in, so a frame gets it by running one of our bundles, but parent-frame code reaching into a child frame's document is not seen. Worth knowing before reading a silence there as proof that nothing went wrong. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomBrowserUI/lib/undefinedUrlDetector.ts | 8 ++++++++ src/BloomExe/web/BloomServer.cs | 17 ++++++++++------- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts index f0a8be2de4cb..1740b034a520 100644 --- a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts +++ b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts @@ -11,6 +11,14 @@ // The interception below is deliberately passive: it reports and then lets the assignment // proceed exactly as before. What we need from it is the stack, and we get that either way, so // there is no reason to change what the app does. +// +// Coverage, so nobody reads a silence here as proof of innocence: this patches the prototypes of +// *the window it is installed in*. Bloom's edit screen puts the book page and the toolbox in +// iframes, and elements created inside a frame are instances of that frame's own constructors. We +// are installed in every frame that runs one of our bundles, since each bundle's root module +// imports errorHandler - but parent-frame code that reaches into a child frame's document +// (`pageIframe.contentDocument.createElement("img").src = x`, a pattern bookEdit does use) is +// creating elements from the child's constructors while running in the parent, and is not seen. /// The exact strings JavaScript produces when these values are turned into text. We match them /// case-sensitively and only as a whole url or a whole path segment, so a real file that happens diff --git a/src/BloomExe/web/BloomServer.cs b/src/BloomExe/web/BloomServer.cs index 34bb26104100..25d724b090cf 100644 --- a/src/BloomExe/web/BloomServer.cs +++ b/src/BloomExe/web/BloomServer.cs @@ -1473,10 +1473,14 @@ private static void ReportMissingFile( PassiveIf.None, "Url built from a JavaScript value", String.Format( - "Server could not find the file {0}. LocalPath was {1}{2}", + "Server could not find the file {0}. LocalPath was {1}{2}{3}", path, localPath, - extraDiagnostics + extraDiagnostics, + // Both kinds of detail, because they answer different questions and this + // request can raise both: a bare "undefined" has no directory AND is a + // JavaScript value. + GetBareNameDiagnostics(localPath) ) ); return; @@ -1506,10 +1510,9 @@ private static void ReportMissingFile( "Cannot Find Image File" ); var detailMsg = String.Format( - "Server could not find the image file {0}. LocalPath was {1}{2}{3}", + "Server could not find the image file {0}. LocalPath was {1}{2}", path, localPath, - extraDiagnostics, Environment.NewLine ); NonFatalProblem.Report(ModalIf.None, PassiveIf.All, userMsg, detailMsg); @@ -1525,9 +1528,9 @@ private static void ReportMissingFile( "Server could not find the file {0}. LocalPath was {1}{2}{3}", path, localPath, - // Both, because they answer different questions and a request can raise both: - // a bare "undefined" has no directory AND is a JavaScript value. - extraDiagnostics + GetBareNameDiagnostics(localPath), + // Not extraDiagnostics: that only ever has content for a JavaScript-value url, + // and those returned above. + GetBareNameDiagnostics(localPath), Environment.NewLine ); NonFatalProblem.Report(ModalIf.Beta, PassiveIf.All, userMsg, detailMsg); From 79858a523e251c1dfadc0522f070017184bf0f82 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 7 Aug 2026 09:13:30 -0500 Subject: [PATCH 5/7] Make the two "is this our url bug?" decisions share one derivation (BL-16666) Devin's point, and a good one: ShouldReportFailedRequest decided from GetLocalPathWithoutQuery(info) while the quiet branch in ReportMissingFile decided from the localPath that ProcessAnyFileContent happened to be holding. They are the same string today, but they are computed independently, and if they ever diverged the consequence is exactly the thing the quiet branch exists to prevent: the request would fall through to the ModalIf.Beta/PassiveIf.All branch and interrupt the user. Both now ask IsJavascriptValueRequest(info), so they cannot disagree. ReportMissingFile takes the request rather than a pre-computed diagnostics string, which is what made the divergence possible. Also adds the missing explanatory comments on three helpers in the front-end detector, per AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/undefinedUrlDetector.ts | 12 ++++++ src/BloomExe/web/BloomServer.cs | 42 +++++++++++-------- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts index 1740b034a520..1afaca663065 100644 --- a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts +++ b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts @@ -131,6 +131,10 @@ function guardSrcProperty( }); } +/** + * The other way an element's src gets set. Wrapping `setAttribute` on `Element` covers every + * element type at once, including any we didn't wrap a `src` property for. + */ function guardSetAttribute( check: (url: unknown, whereItWasSet: string) => void, ): void { @@ -164,6 +168,10 @@ function guardFetch( }; } +/** + * fetch() accepts a string, a URL, or a Request; pull the url out of whichever we were given so + * the check sees the same thing the network will. + */ function urlOfFetchInput(input: RequestInfo | URL): string | undefined { if (typeof input === "string") return input; if (input instanceof URL) return input.href; @@ -173,6 +181,10 @@ function urlOfFetchInput(input: RequestInfo | URL): string | undefined { return undefined; } +/** + * Older code (and some libraries) still request through XMLHttpRequest rather than fetch, and + * `open` is where the url is named, so that is what we wrap. + */ function guardXmlHttpRequest( check: (url: unknown, whereItWasSet: string) => void, ): void { diff --git a/src/BloomExe/web/BloomServer.cs b/src/BloomExe/web/BloomServer.cs index 25d724b090cf..43ba3ec017a9 100644 --- a/src/BloomExe/web/BloomServer.cs +++ b/src/BloomExe/web/BloomServer.cs @@ -1430,11 +1430,7 @@ private bool ProcessAnyFileContent(IRequestInfo info, string localPath) { if (ShouldReportFailedRequest(info, CurrentBook?.FolderPath)) { - ReportMissingFile( - localPath, - path, - GetJavascriptValueDiagnostics(info, localPath) - ); + ReportMissingFile(localPath, path, info); } return false; // from here we head off to BloomServer.MakeReply() which now uses the same ShouldReportFailedRequest() method. } @@ -1448,11 +1444,7 @@ private bool IsAudioFileWhichCanHaveCompressedCounterpart(string path) return path.EndsWith($".{AudioRecording.kRecordableExtension}"); } - private static void ReportMissingFile( - string localPath, - string path, - string extraDiagnostics = "" - ) + private static void ReportMissingFile(string localPath, string path, IRequestInfo info) { if (path == null) { @@ -1465,8 +1457,13 @@ private static void ReportMissingFile( // un-suppressing them would start showing "Cannot Find File" toasts (and, on beta, a // modal) to users on pages that previously failed silently. The distinct message also // gives this class its own Sentry issue rather than burying it in the general one. - // See BL-16666. - if (LooksLikeAJavascriptValueInAUrl(localPath)) + // + // We ask IsJavascriptValueRequest(info) rather than testing localPath, so that this + // decision cannot disagree with the one ShouldReportFailedRequest made from the same + // request. If they ever diverged, a request it had decided to stop suppressing would + // fall through to the loud branch below - which is precisely the toast this exists to + // prevent. See BL-16666. + if (IsJavascriptValueRequest(info)) { NonFatalProblem.Report( ModalIf.None, @@ -1476,7 +1473,7 @@ private static void ReportMissingFile( "Server could not find the file {0}. LocalPath was {1}{2}{3}", path, localPath, - extraDiagnostics, + GetJavascriptValueDiagnostics(info), // Both kinds of detail, because they answer different questions and this // request can raise both: a bare "undefined" has no directory AND is a // JavaScript value. @@ -1557,14 +1554,25 @@ internal static bool LooksLikeAJavascriptValueInAUrl(string localPath) return false; } + /// + /// Whether this request is the JavaScript-value bug above. Everything that needs to know + /// asks *this*, from the request itself, so that the decision to keep reporting it (in + /// ShouldReportFailedRequest) and the decision to report it quietly (in ReportMissingFile) + /// can never be made from two separately-derived strings and disagree. See BL-16666. + /// + private static bool IsJavascriptValueRequest(IRequestInfo info) + { + return LooksLikeAJavascriptValueInAUrl(GetLocalPathWithoutQuery(info)); + } + /// /// The extra detail we attach when the request looks like the JavaScript-value bug above. /// The referrer is the whole point: it names the page that issued the bogus request, which /// is what lets us find the code responsible. See BL-16666. /// - private static string GetJavascriptValueDiagnostics(IRequestInfo info, string localPath) + private static string GetJavascriptValueDiagnostics(IRequestInfo info) { - if (!LooksLikeAJavascriptValueInAUrl(localPath)) + if (!IsJavascriptValueRequest(info)) return ""; return String.Format( "{0}Part of this path is a JavaScript value turned into text, so some of our front-end code built a url out of something it did not have yet." @@ -2280,7 +2288,7 @@ private void ReportMissingFile(IRequestInfo info) "**{0}: File Missing: {1}{2}", GetType().Name, localPath, - GetJavascriptValueDiagnostics(info, localPath) + GetJavascriptValueDiagnostics(info) ); } @@ -2311,7 +2319,7 @@ protected bool ShouldReportFailedRequest( // user is responsible for, so none of the suppressions below should hide it - and the // book-folder one in particular was hiding most of them, since a bogus url built by a // book page resolves inside that book's folder. See BL-16666. - if (LooksLikeAJavascriptValueInAUrl(localPath)) + if (IsJavascriptValueRequest(info)) return true; var localFolderTestPath = localPath; // We don't need even a toast for missing files in the book folder. That's the user's problem From b8c9cb74cb70388b16874c5f7fd296640dbf8236 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 7 Aug 2026 09:25:45 -0500 Subject: [PATCH 6/7] Source-map the detector's stacks, or they name a bundle not our code (BL-16666) Devin's point: errorHandler runs unhandled-error stacks through stacktrace-js so the report names a line in our source, but the new detector was handing over a raw stack, which points into a bundle. For a feature whose entire purpose is to name the line that built the bad url, that mostly defeats it. The detector now hands over the Error itself rather than its stack text, and errorHandler - which already owns the mapping - maps it before reporting, falling back to the raw stack if the mapping fails. Unlike window.onerror we don't send a preliminary unmapped report first: nothing is about to crash and take the mapping with it, and these are deliberately quiet diagnostics, so one report per problem is enough. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomBrowserUI/lib/errorHandler.ts | 19 +++++++++++++++++-- .../lib/undefinedUrlDetector.ts | 9 +++++++-- .../lib/undefinedUrlDetectorSpec.ts | 10 ++++++---- 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/BloomBrowserUI/lib/errorHandler.ts b/src/BloomBrowserUI/lib/errorHandler.ts index 95adf9adeede..d1f5b08ad281 100644 --- a/src/BloomBrowserUI/lib/errorHandler.ts +++ b/src/BloomBrowserUI/lib/errorHandler.ts @@ -160,8 +160,23 @@ if (typeof window !== "undefined") { // Watch for urls built out of a JavaScript value that wasn't ready. Installed here because // every bundle's root module imports this file, which is exactly the coverage we want: the - // bug can be in any of our pages, and the server can't tell us which one. See BL-16577. - installUndefinedUrlDetector(reportError); + // bug can be in any of our pages, and the server can't tell us which one. See BL-16666. + installUndefinedUrlDetector((message, error) => { + // Source-map the stack the same way we do for unhandled errors above. Without this the + // report points into a bundle, which would defeat the purpose: the only reason to catch + // these in the browser at all is to name the line of *our* source that built the url. + // Unlike window.onerror we don't send a preliminary unmapped report first - nothing is + // about to crash and take the mapping with it, and these are diagnostics we deliberately + // keep quiet, so one report per problem is enough. + StackTrace.fromError(error) + .then((stackframes) => + reportError( + message, + stackframes.map((sf) => sf.toString()).join("\n"), + ), + ) + .catch(() => reportError(message, error.stack)); + }); } // Saving this as it MIGHT be useful if we decide to have another go at catching diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts index 1afaca663065..d7ba64ed40ab 100644 --- a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts +++ b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts @@ -58,7 +58,12 @@ export function describeBadUrl(url: unknown, whereItWasSet: string): string { ); } -type Reporter = (message: string, stack: string | undefined) => void; +/** + * Takes the Error rather than its stack text, so the caller can source-map it. A raw stack points + * into a bundle, which is close to useless for a feature whose whole job is to name a line of our + * source. errorHandler does that mapping, since it already owns it for window.onerror. + */ +type Reporter = (message: string, error: Error) => void; let installed = false; @@ -92,7 +97,7 @@ export function installUndefinedUrlDetector(report: Reporter): void { return; alreadyReported.add(message); // The stack of *this* call is the whole point: it names the line that built the url. - report(message, new Error().stack); + report(message, new Error(message)); }; guardSrcProperty(window.HTMLImageElement, "an image's src", check); diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts b/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts index 268ffdf0400a..995e98dc01ad 100644 --- a/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts +++ b/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts @@ -82,11 +82,13 @@ describe("installUndefinedUrlDetector", () => { image.src = notReadyYet as unknown as string; expect(report).toHaveBeenCalledTimes(1); - const [message, stack] = report.mock.calls[0]; + const [message, error] = report.mock.calls[0]; expect(message).toContain("an image's src"); - // The stack is the entire point of this layer: it is what names the offending line. - expect(stack).toBeTruthy(); - expect(stack).toContain("undefinedUrlDetectorSpec"); + // The stack is the entire point of this layer: it is what names the offending line. We + // hand over the Error itself so the caller can source-map it before reporting. + expect(error).toBeInstanceOf(Error); + expect(error.stack).toBeTruthy(); + expect(error.stack).toContain("undefinedUrlDetectorSpec"); }); it("still actually sets the src, so nothing behaves differently", () => { From 4607a0bb67fa13e3537175b7e299934467b96acd Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 7 Aug 2026 09:38:43 -0500 Subject: [PATCH 7/7] Key the once-only rule on the call site, not the message (BL-16666) Devin: "Only the first bad-url report of each kind is ever sent, so later problems stay invisible." Right, and it mattered. Every component that sets an image src to undefined produces the same message, so keying the report-once rule on the message meant we would report whichever component ran first and hide every other one for the rest of the session - which is precisely the blind spot this feature exists to remove. The key is now the message plus the stack. Deliberately the whole stack rather than an attempt to pick out "the caller": any rule for finding that frame has to guess which frames are ours, and a wrong guess lands on a frame that two different call sites share, silently reinstating the blind spot. I tried the clever version first and a test caught it doing exactly that. That leaves the hard cap as the real flood protection rather than the collapse, which the reworked test now says out loud - identical repeats do still collapse, but only when the stack is identical, which a re-render gives us and a caller at a different line does not. The cap bounds it either way. Co-Authored-By: Claude Opus 5 (1M context) --- .../lib/undefinedUrlDetector.ts | 28 +++++++++--- .../lib/undefinedUrlDetectorSpec.ts | 43 ++++++++++++++++--- 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts index d7ba64ed40ab..a59ca387c994 100644 --- a/src/BloomBrowserUI/lib/undefinedUrlDetector.ts +++ b/src/BloomBrowserUI/lib/undefinedUrlDetector.ts @@ -69,12 +69,28 @@ let installed = false; // Each report is an http post to the server, and the bug that triggers it is typically a render // that repeats - the Adjust Timings dialog in BL-16447 would have fired on every open. We only -// need to learn about each site once, so report a given message once and stop altogether after a +// need to learn about each site once, so report a given problem once and stop altogether after a // handful. Losing the repeat count costs us nothing: the point is to identify the code, and the // server-side count in Sentry already tells us how often it happens. +// +// "A given problem" has to mean the call site, not the message. Every component that sets an +// image src to undefined produces the *same* message, so keying on that would report whichever +// one happened to run first and hide every other one for the rest of the session - which is +// exactly the kind of blind spot this whole feature exists to remove. const maxReports = 10; const alreadyReported = new Set(); +/** + * What makes two reports "the same problem": the message plus the whole stack. We use the entire + * stack rather than trying to pick out "the caller" — any rule for finding that frame has to guess + * which frames are ours, and a wrong guess lands on a frame two different call sites share, which + * silently reinstates the blind spot. The stack is stable for repeats from one site, and if two + * routes reach the same bug, hearing about both is useful rather than noise. + */ +function reportKey(message: string, error: Error): string { + return message + "|" + (error.stack ?? ""); +} + /** Exported only so tests can start from a clean slate. */ export function resetReportingForTests(): void { alreadyReported.clear(); @@ -93,11 +109,13 @@ export function installUndefinedUrlDetector(report: Reporter): void { const check = (url: unknown, whereItWasSet: string) => { if (!isJavascriptValueUrl(url)) return; const message = describeBadUrl(url, whereItWasSet); - if (alreadyReported.has(message) || alreadyReported.size >= maxReports) - return; - alreadyReported.add(message); // The stack of *this* call is the whole point: it names the line that built the url. - report(message, new Error(message)); + const error = new Error(message); + const key = reportKey(message, error); + if (alreadyReported.has(key) || alreadyReported.size >= maxReports) + return; + alreadyReported.add(key); + report(message, error); }; guardSrcProperty(window.HTMLImageElement, "an image's src", check); diff --git a/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts b/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts index 995e98dc01ad..b875c572d262 100644 --- a/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts +++ b/src/BloomBrowserUI/lib/undefinedUrlDetectorSpec.ts @@ -121,14 +121,45 @@ describe("installUndefinedUrlDetector", () => { expect(div.getAttribute("title")).toBe("undefined"); }); - it("reports a repeating bug only once, so a re-rendering component can't flood the server", () => { - const image = document.createElement("img"); - - image.src = undefined as unknown as string; - image.src = undefined as unknown as string; - image.src = undefined as unknown as string; + it("stops reporting after a handful, so nothing can flood the server", () => { + // Two protections: identical repeats collapse (same message and stack), and a hard cap + // bounds everything else. The cap is what we can pin down in a test — the collapse depends + // on the stack being identical, which a loop here does give us but a caller at a different + // line would not, so the cap is what actually guarantees the bound in the wild. + for (let i = 0; i < 40; i++) { + document.createElement("img").src = undefined as unknown as string; + } expect(report).toHaveBeenCalledTimes(1); + + // ...and distinct problems keep being reported only up to the cap. + report.mockClear(); + resetReportingForTests(); + const sites = [ + () => (document.createElement("img").src = undefined as never), + () => (document.createElement("img").src = null as never), + () => (document.createElement("iframe").src = undefined as never), + ]; + sites.forEach((site) => site()); + expect(report).toHaveBeenCalledTimes(3); + }); + + it("still reports a second, different call site with the same message", () => { + // Every component that sets an image src to undefined produces the same message, so + // keying the once-only rule on the message alone would report whichever ran first and + // hide all the others for the rest of the session — the exact blind spot this feature + // exists to remove. These two helpers stand in for two unrelated components. + const firstComponent = () => { + document.createElement("img").src = undefined as unknown as string; + }; + const secondComponent = () => { + document.createElement("img").src = undefined as unknown as string; + }; + + firstComponent(); + secondComponent(); + + expect(report).toHaveBeenCalledTimes(2); }); it("reports a fetch of an undefined url, which is how BL-16447 escaped", () => {