1819570 - Netmonitor no longer shows fetch requests from workers

Status: RESOLVED FIXED

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

Description

[Julian Descottes [:jdescottes]]

The test browser_net_worker_stacks.js was disabled on Nightly when enabling the preference dom.workers.pFetch.enabled.

This test checks if we show fetch requests issued from workers, so we should definitely fix it and restore it.

Comment 1

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

Set release status flags based on info from the regressing bug 1812039

:edenchuang, since you are the author of the regressor, bug 1812039, could you take a look?

For more information, please visit auto_nag documentation.

Comment 2

[Julian Descottes [:jdescottes]]

There's already a needinfo at https://bugzilla.mozilla.org/show_bug.cgi?id=1819307

Comment 3

[Julian Descottes [:jdescottes]]

Repurposing to fix the regression.

:edenchuang shared that the cause of the regression is that the channel is now created in the parent process instead. Copying my answer from phabricator at https://phabricator.services.mozilla.com/D171449#5640782

we already monitor channels from the parent process, it's just that we can't link this channel to the tab we are debugging.
Looking quickly we fail to match around https://searchfox.org/mozilla-central/rev/f7edb0b474a1a922f3285107620e802c6e19914d/devtools/shared/network-observer/NetworkUtils.sys.mjs#358-371
There are two things we try to use to match the channel, first channel.loadInfo.browsingContext (not set for this channel) and then the loadContext.

Is there a way we can set channel.loadInfo.browsingContexthere ? Or allow to get the loadContext from the channel via loadGroup.notificationCallbacks.getInterface(Ci.nsILoadContext);?
If none of those work, then what could be a good way to match this channel to a given BrowsingContext?

Comment 4

[Dianna Smith [:diannaS]]

:edenchuang as a reminder that code freeze starts tomorrow... is there a plan to fix this before Fx112 goes to beta?

Comment 5

[Suhaib Mujahid [:suhaib]]

(In reply to Dianna Smith [:diannaS] from comment #4)

:edenchuang as a reminder that code freeze starts tomorrow... is there a plan to fix this before Fx112 goes to beta?

:edenchuang has a WIP patch, but it will target 113, and if everything goes fine, uplift it to beta 112.

Comment 6

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

Set release status flags based on info from the regressing bug 1812039

Comment 7

[Dianna Smith [:diannaS]]

just checking in for an update on this patch, is the plan still to uplift this to 112 beta?

Comment 9

[Eden Chuang[:edenchuang]]

I am testing the local patch with some try runs to make sure it would not make any side-effect.
Will request a review in these two days.
I think it still targets 113. However, if it goes fine, I think we can still uplift it to 112 beta.

Comment 10

[Eden Chuang[:edenchuang]]

Attachment: Bug 1819570 - P1 Propagate worker's assocaited BrowsingContextID through PFetch to the parent process. r=#dom-worker-reviewers

Comment 11

[Eden Chuang[:edenchuang]]

Attachment: Bug 1819570 - P2 Save worker's associated BrowsingContextID in channel's LoadInfo for the netmonitor. r=#necko-reviewers

After PFetch is enabled, fetch() call in workers will not create a channel in the content process anymore.
Although netmonitor also watches the channels in the parent process, the created channel still loses the BrowsingContext information for netmonitor to connect the network event and the channel.

In P1, https://phabricator.services.mozilla.com/D174249, we propagate the BrowsingContext ID through PFetch.
In this patch, we need to save it in channel's LoadInfo for netmonitor.

In order not to confuse with nsILoadInfo's BrowsingContextID, we create a new attribute WorkerAssociatedBrowsingContextID in nsILoadInfo.

Depends on D174249

Comment 12

[Eden Chuang[:edenchuang]]

Attachment: Bug 1819570 - P3 using nsILoadInfo.WorkerAssociatedBrowsingContext for netmonitor. r=jdescottes

After PFetch is enabled, fetch() call in workers will not create a channel in the content process anymore.
Although netmonitor also watches the channels in the parent process, the created channel still loses the BrowsingContext information for netmonitor to connect the network event and the channel.

Depends on D174441

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Bug 1819570 - P4 Propagte stack trace notification back to the content process for PFetch. r=#dom-worker-reviewers

After PFetch is enabled, fetch() call in workers will not create a channel in the content process anymore.
Although netmonitor watches channels and NetEvents, stack traces are only caught in the content process.
That means PFetch should notify the netmonitor about the stack trace of the fetch at the proper moment.

In original fetch steps, FetchDriver would notify the netmonitor the fetch stack trace at
https://searchfox.org/mozilla-central/rev/cdddec7fd690700efa4d6b48532cf70155e0386b/dom/fetch/FetchDriver.cpp#834

When PFetch is enabled, PFetch needs also to propagate this notification back to the content process.

Depends on D174442

Comment 14

[Eden Chuang[:edenchuang]]

Attachment: Bug 1819570 - P5 Let netmonitor to watch stack trace events on nsIWorkerChannleInfo. r=jdescottes

Depends on D174443

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this ready to land?

Comment 18

[Julian Descottes [:jdescottes]]

For info, we are getting reports of users missing requests in devtools, it would be great to land this soon (or to uplift in 114 after the freeze)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d73ec5415a4
P1 Propagate worker's assocaited BrowsingContextID through PFetch to the parent process. r=asuth
https://hg.mozilla.org/integration/autoland/rev/81e926cf92dc
P2 Save worker's associated BrowsingContextID in channel's LoadInfo for the netmonitor. r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/c8534cf58f86
P3 using nsILoadInfo.WorkerAssociatedBrowsingContext for netmonitor. r=jdescottes,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/addc41903c2f
P4 Propagte stack trace notification back to the content process for PFetch. r=asuth
https://hg.mozilla.org/integration/autoland/rev/e73379145f9a
P5 Let netmonitor to watch stack trace events on nsIWorkerChannleInfo. r=jdescottes,devtools-reviewers

Comment 20

[Iulian Moraru]

Backed out for causing dt failures on browser_net_worker_stacks.js.

• Push with failures

• Failure log

• Backout link

[task 2023-05-04T04:06:18.146Z] 04:06:18     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_worker_stacks.js | The requestItem exists - 
[task 2023-05-04T04:06:18.147Z] 04:06:18     INFO - Visible index of item: 5
[task 2023-05-04T04:06:18.148Z] 04:06:18     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_worker_stacks.js | The attached method is correct. - 
[task 2023-05-04T04:06:18.148Z] 04:06:18     INFO - Buffered messages finished
[task 2023-05-04T04:06:18.150Z] 04:06:18     INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_worker_stacks.js | The attached url is correct. - Got "https://example.com/browser/devtools/client/netmonitor/test/missing.txt", expected "https://example.com/browser/devtools/client/netmonitor/test/missing.json"
[task 2023-05-04T04:06:18.150Z] 04:06:18     INFO - Stack trace:
[task 2023-05-04T04:06:18.151Z] 04:06:18     INFO - chrome://mochikit/content/browser-test.js:test_is:1612
[task 2023-05-04T04:06:18.151Z] 04:06:18     INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/head.js:verifyRequestItemTarget:596
[task 2023-05-04T04:06:18.151Z] 04:06:18     INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/head.js:validateRequests/<:1187
[task 2023-05-04T04:06:18.151Z] 04:06:18     INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/head.js:validateRequests:1183
[task 2023-05-04T04:06:18.151Z] 04:06:18     INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_worker_stacks.js:null:116
[task 2023-05-04T04:06:18.151Z] 04:06:18     INFO - chrome://mochikit/content/browser-test.js:handleTask:1133
[task 2023-05-04T04:06:18.151Z] 04:06:18     INFO - chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1205
[task 2023-05-04T04:06:18.151Z] 04:06:18     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1347
[task 2023-05-04T04:06:18.151Z] 04:06:18     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1122
[task 2023-05-04T04:06:18.151Z] 04:06:18     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1056
[task 2023-05-04T04:06:18.152Z] 04:06:18     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_worker_stacks.js | The displayed method is correct. - 
[task 2023-05-04T04:06:18.153Z] 04:06:18     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_worker_stacks.js | The displayed file is correct. - 
[task 2023-05-04T04:06:18.154Z] 04:06:18     INFO - Not taking screenshot here: see the one that was previously logged
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_worker_stacks.js | The tooltip file is correct. - Got "https://example.com/browser/devtools/client/netmonitor/test/missing.txt", expected "https://example.com/browser/devtools/client/netmonitor/test/missing.json"
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - Stack trace:
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - chrome://mochikit/content/browser-test.js:test_is:1612
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/head.js:verifyRequestItemTarget:625
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/head.js:validateRequests/<:1187
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/head.js:validateRequests:1183
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_worker_stacks.js:null:116
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - chrome://mochikit/content/browser-test.js:handleTask:1133
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1205
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1347
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1122
[task 2023-05-04T04:06:18.156Z] 04:06:18     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1056
[task 2023-05-04T04:06:18.157Z] 04:06:18     INFO - TEST-PASS | devtools/client/netmonitor/test/browser_net_worker_stacks.js | The displayed protocol is correct. - 

Comment 21

[Julian Descottes [:jdescottes]]

This might be one of the tests where the netmonitor assumes a request order which is not necessarily guaranteed. I don't think this really highlights an implementation issue. I can help to update this test so that it no longer expects the missing.txt request to be listed before the missing.json request.

Comment 22

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/335ef5d9bbb7

Comment 23

[Julian Descottes [:jdescottes]]

The following patch should avoid the intermittent: https://hg.mozilla.org/try/rev/197aa6c5cb20b4b750443e872cd9a2a4f6d86d18

Feel free to fold that in, I can ping another devtools peer for review.
Hubert: can you take a look at this patch, I think it's fine to relax the expectation on request order here?

Comment 24

[Hubert Boma Manilla (:bomsy)]

(In reply to Julian Descottes [:jdescottes] from comment #23)

The following patch should avoid the intermittent: https://hg.mozilla.org/try/rev/197aa6c5cb20b4b750443e872cd9a2a4f6d86d18

Feel free to fold that in, I can ping another devtools peer for review.
Hubert: can you take a look at this patch, I think it's fine to relax the expectation on request order here?

Yes that sounds fine. The patch LGTM!

Comment 25

[Eden Chuang[:edenchuang]]

Attachment: Bug 1819570 - P6 Allow a different request order in netmonitor tests. r=jdescottes

Depends on D174444

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/674cfac561b6
P1 Propagate worker's assocaited BrowsingContextID through PFetch to the parent process. r=asuth
https://hg.mozilla.org/integration/autoland/rev/6a5e5ebc576c
P2 Save worker's associated BrowsingContextID in channel's LoadInfo for the netmonitor. r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/5d99cbb6461a
P3 using nsILoadInfo.WorkerAssociatedBrowsingContext for netmonitor. r=jdescottes,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/c6fa624678e8
P4 Propagte stack trace notification back to the content process for PFetch. r=asuth
https://hg.mozilla.org/integration/autoland/rev/33a18bb015ff
P5 Let netmonitor to watch stack trace events on nsIWorkerChannleInfo. r=jdescottes,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/6307e9e20326
P6 Allow a different request order in netmonitor tests. r=jdescottes,devtools-reviewers

Comment 27

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/674cfac561b6
https://hg.mozilla.org/mozilla-central/rev/6a5e5ebc576c
https://hg.mozilla.org/mozilla-central/rev/5d99cbb6461a
https://hg.mozilla.org/mozilla-central/rev/c6fa624678e8
https://hg.mozilla.org/mozilla-central/rev/33a18bb015ff
https://hg.mozilla.org/mozilla-central/rev/6307e9e20326