1646541 - Cleanup/remove nsGlobalWindowInner::ShouldReportForServiceWorkerScope

Status: NEW

(Core :: DOM: Service Workers, task, P3)

Comment 1

[Chris Peterson [:cpeterson]]

Tracking "Figure out GetInProcessTop usage" bugs for Fission M6b.

Comment 2

[Neha Kochar [:neha]]

Andrew, can you review this too?

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

In short: The current logic is part of devtools' handling of console messages of ServiceWorkers which has slowly been getting cleaned up. The necessary cleanup is currently tracked as part of bug 1592584 which is targeted at Fission M7 but might be impacted by staffing level changes. The current implementation without that bug fixed will result in a slightly degraded experience for the specific logging mechanism, but that mechanism isn't sufficient for a good experience on its own under parent-intercept. That said, what did work should continue to work for cases where the ServiceWorker of interest to be debugged is the current devtools frame.

I'll prepare a patch to update the comment at the location that indicates the current limitations, references this bug for more depth, and references the M7 bug devtools bugs tracking the modern implementation end state.

PENDING: I still need to better clarify the non-fission status quo for ServiceWorker debugging without fission enabled since we switched to parent intercept and the enhancements that may be necessary post-bug 1592584 or that should be considered as part of it. In particular, as things relate to "devtools.debugger.features.windowless-service-workers".

Restating overview:

• Console messages have 2 general paths:
  1. Modern: Be reported directly on a global's Console actor.
  2. Legacy: Be reported to the process-wide "@mozilla.org/consoleAPI-storage;1" JS XPCOM service on the main thread where a parameterized ConsoleAPIListener instance filters the global log soup back down to the window of interest.
• We've been slowly moving towards the modern approach.
  • Bug 1613147 most recently made it possible to disable this legacy path entirely.
  • https://bugzilla.mozilla.org/show_bug.cgi?id=1592584#c10 seemed to demonstrate that at least the broad strokes of this already works for workers when disabling the legacy path and https://bugzilla.mozilla.org/show_bug.cgi?id=1592584#c13 was a refinement of that demonstration patch.
  • The primary correctness concern is ensuring that devtools is able to properly associate all ServiceWorkers of interest with the page in question. See the "in depth" section below for more, but there's a series of recursive checks.
• The GetInProcessScriptableTop() limitation under Fission means that:
  • The nsGlobalWindowInner::ShouldReportForServiceWorkerScope check will only see a portion of the entire docshell/browsing context tree that it saw pre-fission. Specifically, it will only see direct descendant same-origin iframes.
  • With Bug 1568597 fixed (landing soon), the ServiceWorker will end up being in-process with the (potentially) intercepted/registering page.

Restating in depth:

• The use-case for nsGlobalWindowInner::ShouldReportForServiceWorkerScope to be exposed via Window.webidl for chrome-only purposes to be consumed by devtools' console-api.js's isMessageRelevant. That method is used as a filter over "console-api-log-event" observer notifications with the intent to filter out ServiceWorker messages that are not relevant to the current window. The message is also used to process cached messages
• The currently logic primarily stems from bug 1425316 and the patches had excellent commit messages, with the bug adding some extra context.
  • Of particular relevance is the point at https://bugzilla.mozilla.org/show_bug.cgi?id=1425316#c18 about how this is actually a fallback path. The searchfox link in the comment is bit-rotted, but manually adjusting it, we get CopyASCIItoUTF16(mWorkerPrivate->ServiceWorkerScope(), id); which is still there with a more current permalink.
  • The above logic is invoked on the main thread as part of a sync runnable dispatched from the worker thread to the main thread, this is part of the legacy but currently used console logging mechanism.
• The pre-fission logic cares about 4 scenarios:
  1. The page is controlled by the ServiceWorker registration in question as identified by the scope.
  2. The page has started registering a ServiceWorker registration with the given scope.
  3. The page may be navigating and may not yet be controlled but its navigation matches the given scope.
  4. Recursively contained iframes.

Comment 4

[Jens Stutte [:jstutte]]

I assume we can move this to M7 then?

Comment 5

[Neha Kochar [:neha]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #3)

In short: The current logic is part of devtools' handling of console messages of ServiceWorkers which has slowly been getting cleaned up. The necessary cleanup is currently tracked as part of bug 1592584 which is targeted at Fission M7

Moving to M7 based on this.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

:cpeterson pinged me about this externally. The current state is:

• With the devtools bug 1592584 fixed the devtools experience is likely fine and there's no need to block Fission.
• The use of GetInProcessScriptableTop in nsGlobalWindowInner::ShouldReportForServiceWorkerScope is basically mooted by the above and can probably be ripped out. Its current usage is fine given that it's legacy code.

The next steps re: this bug are:

• Audit nsGlobalWindowInner::ShouldReportForServiceWorkerScope now in the context of bug 1592584 being fixed and the higher level devtools bug on that, bug 1429805. If we can't remove ShouldReportForServiceWorkerScope then bug 1429805 should be updated and/or have new dependencies filed and a comment added to the use of GetInProcessScriptableTop.

Accordingly, I'm removing this bug from blocking fission.