1880803 - NetworkObserver should create network events earlier (http-on-before-connect)

Status: RESOLVED FIXED

(DevTools :: Netmonitor, task, P2)

Description

[Julian Descottes [:jdescottes]]

In order to enable proper request blocking and interception for both DevTools and WebDriver BiDi, the NetworkObserver should create network events when catching the http-on-dispatching-transaction observer.

Right now we use a collection of various entry points to create the events, hopefully this observer can replace them all. However the challenge will be to make sure we have enough information at this point in time in order to populate the DevTools UI and to emit the WebDriver BiDi network.beforeRequestSent event.

The new observer notification should be added in Bug 1849686. Note that there are currently issues with ServiceWorker requests when using this notification.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1880803 - [devtools] Cleanup unused network codepaths and helpers

The window property is never set on NetworkObserver, remove the related codepath.
Also remove unused helpers.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1880803 - [devtools] Add option to NetworkObserver to create events earlier

Depends on D209523

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d63b73b4125
[devtools] Cleanup unused network codepaths and helpers r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/fb750d3b365d
[devtools] Add option to NetworkObserver to create events earlier r=devtools-reviewers,perftest-reviewers,ochameau,afinder

Comment 4

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/4d63b73b4125
https://hg.mozilla.org/mozilla-central/rev/fb750d3b365d

Comment 5

[Atila Butkovits]

Backed out for causing high frequency failures at url_patterns.py

Backout link: https://hg.mozilla.org/integration/autoland/rev/c5f0d71de69cf598d009f69c9ddf56fb6058bc9e

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=aNUnl0SdSJSaPYpgdA1p9g.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=windows%2C11%2Cx64%2C22h2%2Cwebrender%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-windows11-64-2009-qr%2Fopt-web-platform-tests-wdspec%2Cwd4&revision=2e388b7f6f914a31e5dbf521525f390163191bb4

Failure log: https://treeherder.mozilla.org/logviewer?job_id=459502969&repo=autoland&lineNumber=128070

Comment 6

[Julian Descottes [:jdescottes]]

We have an issue with some channels using the same channelId. This confuses the ChannelMap used by the DevTools NetworkObserver. This ChannelMap attempts to implement an iterable WeakMap, but to do so it relies on the channelId. This relates to the issue spotted for Bug 1897143.

In this ChannelMap we store "activity" objects, which contain various data collected about the channel, as well as the channel itself (activity.channel). The goal of the patches here was to make more consistent the way we use channels and activity objects in the NetworkObserver, and since the activity contains activity.channel, in all spots where we already fetched the activity object we started reading the channel from activity.channel instead of passing the channel as another argument.

So if we have channel1 (stale) and channel2 (new) with the same channelId, when we retrieve the activity for channel2, activity.channel will still point to channel1, and this creates various issues for us. Most importantly, we fail to create the network event. I'm still investigating to understand which requests are causing this, because the tests where this fails simply perform regular fetch requests.

Potential solutions:

• We could revert part of the change and keep passing both channel and activity, but that's probably only hiding a potential issue.
• Another option is to always update the activity object with the latest channel when we retrieve it. This assumes that the previous channel we had stored is really no longer relevant and we should completely forget about it.
• Finally another approach would be to handle different channels independently (eg by creating another UUID), but that would probably lead to unexpected situations where we create new network events for the same request (at least that would be the case for the STRs identified in Bug 1897143).

I think I'm rather tempted to go for the second option here, and file a followup to understand in which situations we have channels which reuse the same channelId.

Comment 7

[Julian Descottes [:jdescottes]]

Hi Valentin,

I managed to identify two requests which share the same channelId on one job.

• https://treeherder.mozilla.org/logviewer?job_id=459898356&repo=try&lineNumber=122013 which is a fetch to "https://web-platform.test/path"
• https://treeherder.mozilla.org/logviewer?job_id=459898356&repo=try&lineNumber=127892 which is a fetch to "https://web-platform.test/path/continued"

The two requests are 6 seconds apart, but their channel is using the same channelId "21492016349187". This is something we have only seen on windows so far.

In this case it really seems that they are two completely unrelated requests, which seems different from the use case for service worker requests were the intercepted channel keeps the same channelId. In DevTools we have a "strong" assumption that we should always get a unique channelId for different requests (eg https://searchfox.org/mozilla-central/rev/893f350260faac2ee6bf2b14c627d55eb2babfb0/devtools/server/actors/resources/network-events.js#281-285), so do you know in which cases two different requests will still use the same channelId ? Is this a bug or something we need to work with?

Comment 8

[Julian Descottes [:jdescottes]]

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

I think I'm rather tempted to go for the second option here, and file a followup to understand in which situations we have channels which reuse the same channelId.

Considering the example I found above, I don't think I have a perfect solution at the moment. There are use cases where two channels with the same channelId correspond to one request (service worker example), and there are cases where they correspond to two different requests (example above). In the first case we should rather "update" the activity with the new channel, in the second case we should create a new activity, a new owner, a new event etc... I think we should handle this in a follow up.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

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

The two requests are 6 seconds apart, but their channel is using the same channelId "21492016349187". This is something we have only seen on windows so far.

As far as I know we shouldn't be reusing channelIDs unless a redirect is involved, or a service worker.
That doesn't seem to be the case here.
As Julian mentioned, the tests are performed in different tabs, which means it's possible that the channelID gets generated, process finishes and gets killed, and another process comes along and gets assigned the same ProcessID leading to potentially the same channelIDs coming from the new process.

The channelID causing the issue in the tests is 0x138C00000003 - so this theory seems likely.
If this turns out to be the case we can update the channelID generating code to include some other factor - like a counter of spawned processes?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34c1026a5bf7
[devtools] Cleanup unused network codepaths and helpers r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/3ae2284fd96f
[devtools] Add option to NetworkObserver to create events earlier r=devtools-reviewers,perftest-reviewers,ochameau,afinder

Comment 11

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/34c1026a5bf7
https://hg.mozilla.org/mozilla-central/rev/3ae2284fd96f