1830100 - Tidy the stale ContentBlockLog short-circuit comment in Document::EffectiveStoragePrincipal

Status: NEW

(Core :: Privacy: Anti-Tracking, task)

Description

[Beth Rennie [:beth] (she/her)]

The following comment appears in dom/base/Document.cpp:

  // Calling StorageAllowedForDocument will notify the ContentBlockLog. This
  // loads TrackingDBService.jsm, which in turn pulls in osfile.jsm, making us
  // fail // browser/base/content/test/performance/browser_startup.js. To avoid
  // that, we short-circuit the check here by allowing storage access to system
  // and addon principles, avoiding the test-failure.

This is no longer the case and might consider re-evaluation.

Comment 1

[Dan Mehic]

Evaluated this against current central. The OSFile reference is gone, but the comment still has problems and the question in comment 0 was never actually answered, so I have re-scoped rather than closed. Details below.

The reported defect is fixed

The comment no longer mentions OSFile. Current text at Document.cpp#20930, inside Document::EffectiveStoragePrincipal():

  // Calling StorageAllowedForDocument will notify the ContentBlockLog. This
  // loads TrackingDBService.sys.mjs, making us potentially
  // fail // browser/base/content/test/performance/browser_startup.js. To avoid
  // that, we short-circuit the check here by allowing storage access to system
  // and addon principles, avoiding the test-failure.

Compared with comment 0: the osfile.jsm clause is gone, TrackingDBService.jsm became .sys.mjs, and "making us fail" was softened to "making us potentially fail". That looks like incidental tidying during the JSM to ESM migration rather than deliberate work on this bug. osfile.jsm now survives in the tree only in dom/docs/ioutils_migration.md.

The re-evaluation comment 0 asked for still holds

Comment 0 suggested the short-circuit itself might no longer be needed. It still is — the causal chain the comment describes is intact:

• ContentBlockingLog.cpp includes nsITrackingDBService.h and instantiates nsITrackingDBService in ReportLog();
• toolkit/components/antitracking/components.conf maps that contract to resource://gre/modules/TrackingDBService.sys.mjs;
• browser/base/content/test/performance/browser_startup.js still enforces a per-phase module allowlist.

So the short-circuit for system and add-on principals continues to have a live reason to exist, and should not be removed. Recording that here so the question does not need re-asking.

What is actually left

Two defects in the comment text, both present in the original and untouched by the ESM cleanup:

1. // fail // — a stray duplicated comment marker mid-sentence, so the text reads "making us potentially fail // browser/base/content/test/performance/browser_startup.js".
2. "principles" should be "principals". The code immediately below calls IsSystemPrincipal() and GetIsAddonOrExpandedAddonPrincipal(), so this is the wrong word rather than a spelling variant.

The line wrapping is also awkward now that the text has shrunk, and would benefit from a reflow in the same patch.

Re-scoping

Updating the summary from "Document comment references loading OSFile, but OSFile is no longer loaded" to reflect what remains. Closing it outright would lose the verification above, which is the more useful half.

This is comment-only, no behaviour change, and self-contained — a reasonable good-first-bug if someone wants to tag it as one.