1657130 - Expose list of parent windowIDs on nsIWorkerDebugger for SharedWorkers

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement, P2)

Description

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

In bug 1607778 it would make it easier for devtools to map nsIWorkerDebugger instances to their owning windows if their list of window IDs was exposed. Right now ServiceWorker mappings are handled by unsigned long serviceWorkerID and dedicated Worker bindings are handled by mozIDOMWindow window.

When implementing, note that it's best to ensure that any existing XPCOM bindings that return arrays that are imitated are semi-recent, as there was a transition point when returned arrays got much prettier.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Hello Andrew, I'm ready to start working on Bug 1607778 and I was wondering if you had the cycle to work on this? If not, maybe you could mentor me doing it? I don't have much experience in C++ but I can hack around things :)

Comment 2

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

I'll be working in this space in the next ~2 weeks, I should be able to do this.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Note that it would be nice to re-use the same array for dedicated workers (it would then only contain 1 element), so consumers can only care about this array of ids, and not have to have different paths between shared and dedicated worker.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

NOTE: I just filed Bug 1673024 where I highlight an issue with the window property on WorkerDebugger while navigating away. This isn't directly impacting this bug, but we should be careful to not have this issue with the new property we'll have.

Comment 5

[Yaron Tausky [:ytausky]]

Attachment: Bug 1657130 - Expose worker's window IDs to nsIWorkerDebugger r=#dom-workers-and-storage-reviewers,asuth

Comment 6

[Yaron Tausky [:ytausky]]

I wrote a tentative patch, but I'm not sure how to test it yet.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Thanks Yaron! I'll try to test it today or Monday

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

I used the new windowIds property in DevTools code where we used to get through the window, and TRY seems fine with that (https://treeherder.mozilla.org/jobs?repo=try&revision=38a5a81cb7c5ac85d734b8552c6966de1259e58e)
It also allowed me to remove the code we had to workaround Bug 1673024, which is nice :)
I adapted https://phabricator.services.mozilla.com/D94599 to use windowIds as well, and I can't reproduce the bug anymore, so this is great.
You might want to take inspiration from the test in https://phabricator.services.mozilla.com/D94599 , and add test cases with shared worker to check windowIds holds the expected ids.
Let me know if I can help doing that, I know you're busy.

Thanks again :)

Comment 9

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:ytausky, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Hello Yaron, any update on this? Since you already did most of the work, it would be nice to push it over the finish line :)

Comment 11

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

The plan was to wait for direct test coverage for this, but Yaron has been juggling some higher priority bugs and given that devtools will give us indirect coverage, I've amended the testing flag in the review so this can land now. But we should file a follow-up to add a test.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4b644dd05b2
Expose worker's window IDs to nsIWorkerDebugger r=dom-workers-and-storage-reviewers,asuth

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d4b644dd05b2

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Thanks!

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/d62e46c787df
Backed out changeset d4b644dd05b2 for causing worker crashes. a=backout

Comment 16

[Alexandru Michis [:malexandru]]

Backed out changeset d4b644dd05b2 (Bug 1657130) for causing worker crashes. a=backout

Backout link: https://hg.mozilla.org/mozilla-central/rev/d62e46c787dfaa06ef650290011d8d72c2d056f7

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for crashes in latest Nightly.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57bb57b6bfb7
Expose worker's window IDs to nsIWorkerDebugger r=dom-workers-and-storage-reviewers,asuth

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/57bb57b6bfb7