1673024 - WorkerDebugger#window can refer to the wrong window

Status: NEW

(Core :: DOM: Workers, defect, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

If you have a page where a dedicated worker lives in, the worker debugger provides a useful window access (See nsIWorkerDebugger.idl#36-38)

But if the tab reloads, it can happen that the worker would still be retrievable through the worker debugger manager enumerator, and window points to the new window (the one we're navigating to).

My guess is that the window is a getter, and we don't clean it soon enough (or ever maybe?).

I'll attach a mochitest that reproduce the issue everytime on my machine

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1673024 - Test to demonstrate the issue with WorkerDebugger#window

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

That's something that got into my way while working on Bug 1633712 (DevTools worker debugging support for Fission)

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Apply https://phabricator.services.mozilla.com/D94599 and run browser_worker_debugger_window.js to see the issue.

(tagging Andrew as he might work in this area those days)

Comment 4

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

The inner window is explicitly latched in the Worker constructor and the getter returns that.

My first suspicion would be that the inner window you're being returned ends up being used in an outer window manner, possibly via some kind of binding magic. For example getting the WindowUtils for an inner window forwards to its outer window. But I see windowGlobalChild seems to only be defined on Window.webidl (next to the windowUtils getter), but its binding clearly is calling nsPIDOMWindowInner::GetWindowGlobalChild which seems to correspond exactly to the window and have the right life-cycle. That said, some kind of coercion could be happening when the inner window passes through XPConnect and the binding magic happens.

If you happen to have a pernosco trace of this, that could help simplify things, but if not (and/or you're on the wrong platform) I should be able to generate one at some point.

Comment 5

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

One thing to sanity check in the test is that the output indicates that windowGlobalChild is non-null, as the:

      if (
        workerDbg.window &&
        initialInnerWindowId !==
          workerDbg.window.windowGlobalChild?.innerWindowId
      )

check I believe could catch the situation where workerDbg.window is non-null but workerDbg.window.windowGlobalChild is null.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

I pushed to try with --pernosco : https://treeherder.mozilla.org/#/jobs?repo=try&revision=79be25d6a60ea9a2d6a5393e25e341f7eafc5e71&selectedTaskRun=WI2z2dmfSAuRAlPyFvyIPQ.0

not sure where I should find the link, or if that even worked (I used fuzzy so maybe it didn't work?)

Comment 7

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

I believe you'd get an email. When I construct a self-service API link of https://pernos.co/self-service-api/mozilla/WI2z2dmfSAuRAlPyFvyIPQ/self-service.html per https://github.com/Pernosco/pernosco/wiki/Mozilla-Tips#reproducing-arbitrary-taskcluster-job-failures the automation doesn't seem to see the failure, so I suspect the issue is that pernosco doesn't understand TV jobs.

Comment 8

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

Confirmed with :khuey that pernosco has trouble with test verify jobs because they don't make it clear to the pernosco automation what test framework or test is being run from the logs. I triggered all the browser mochitest jobs in the hope of finding the job that has the new test. (In theory I think I could find the first base revision that has full browser mochitest runs for the directory you added the test and use the test-groups test to find the manifest that includes it? Maybe there's a better way now?)

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Here's a pernosco link for the failing test: https://pernos.co/debug/JXLli-WihBw7GDqXQPaHJA/index.html

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jstutte, could you have a look please?

For more information, please visit auto_nag documentation.