1790368 - Implement "network.beforeRequestSent" event

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, task, P1)

Comment 1

[James Graham [:jgraham]]

Attachment: Bug 1790368 - Update pytest-asyncio to 0.19.0

Comment 2

[James Graham [:jgraham]]

Attachment: Bug 1790368 - Mark async fixtures explicitly

Comment 3

[James Graham [:jgraham]]

Attachment: Bug 1790368 - Update websockets library to 10.3

Comment 4

[James Graham [:jgraham]]

Attachment: Bug 1790368 - Update creating websockets connection in BiDi client

Comment 5

[James Graham [:jgraham]]

Attachment: Bug 1790368 - Move action module to remote/shared/webdriver

Comment 6

[James Graham [:jgraham]]

Attachment: Bug 1790368 - Move event module to remote/shared/webdriver

Comment 7

[James Graham [:jgraham]]

Attachment: Bug 1790368 - Add input module support to WebDriver BiDi

Comment 8

[James Graham [:jgraham]]

Attachment: Bug 1790368 - Add input module to bidi test client

Comment 9

[James Graham [:jgraham]]

Attachment: Bug 1790368 - Add basic tests for BiDi key input

Comment 10

[Phabricator Automation]

Comment on attachment 9295861 [details]
Bug 1790368 - Update pytest-asyncio to 0.19.0

Revision D157966 was moved to bug 1792090. Setting attachment 9295861 [details] to obsolete.

Comment 11

[Phabricator Automation]

Comment on attachment 9295862 [details]
Bug 1790368 - Mark async fixtures explicitly

Revision D157967 was moved to bug 1792090. Setting attachment 9295862 [details] to obsolete.

Comment 12

[Phabricator Automation]

Comment on attachment 9295863 [details]
Bug 1790368 - Update websockets library to 10.3

Revision D157968 was moved to bug 1792090. Setting attachment 9295863 [details] to obsolete.

Comment 13

[Phabricator Automation]

Comment on attachment 9295864 [details]
Bug 1790368 - Update creating websockets connection in BiDi client

Revision D157969 was moved to bug 1792090. Setting attachment 9295864 [details] to obsolete.

Comment 14

[Phabricator Automation]

Comment on attachment 9295865 [details]
Bug 1790368 - Move action module to remote/shared/webdriver

Revision D157970 was moved to bug 1792090. Setting attachment 9295865 [details] to obsolete.

Comment 15

[Phabricator Automation]

Comment on attachment 9295866 [details]
Bug 1790368 - Move event module to remote/shared/webdriver

Revision D157971 was moved to bug 1792090. Setting attachment 9295866 [details] to obsolete.

Comment 16

[Phabricator Automation]

Comment on attachment 9295867 [details]
Bug 1790368 - Add input module support to WebDriver BiDi

Revision D157972 was moved to bug 1792090. Setting attachment 9295867 [details] to obsolete.

Comment 17

[Phabricator Automation]

Comment on attachment 9295868 [details]
Bug 1790368 - Add input module to bidi test client

Revision D157973 was moved to bug 1792090. Setting attachment 9295868 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9295869 [details]
Bug 1790368 - Add basic tests for BiDi key input

Revision D157974 was moved to bug 1792090. Setting attachment 9295869 [details] to obsolete.

Comment 19

[Julian Descottes [:jdescottes]]

Attachment: Bug 1790368 - [bidi] Implement basic support for network.beforeRequestSent event

Comment 20

[Julian Descottes [:jdescottes]]

Attachment: Bug 1790368 - [wdspec] Add basic tests for network.beforeRequestSent event

Depends on D162037

Comment 21

[Julian Descottes [:jdescottes]]

Hi Henrik, quick ping about a topic we discussed several times: creating an alias to DevTools' NetworkObserver.sys.mjs in remote's jar.mn. While this worked fine for httpd.js I think we should not use this pattern for NetworkObserver.sys.mjs.

The main reason is that NetworkObserver.sys.mjs is not a "standalone" module, it imports other modules from devtools/shared/network-observer. So if we were concerned about situations where devtools would not be included in the build, this actually doesn't help because NetworkObserver.sys.mjs will not be able to import its dependencies.

Another reason is that this pattern make things harder to understand. Usually you expect that the paths used in our imports are somewhat mapped to what is in the filesystem, and we should not break this convention lightly. I think httpd.js is a special case because it is not shipped in most builds. But here I would rather trust that devtools' shared folder is part of all builds (which is the case AFAIK) or if that's not good enough, start thinking about another location (toolkit?).

Let me know if you agree/disagree !

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

The main reason is that NetworkObserver.sys.mjs is not a "standalone" module, it imports other modules from devtools/shared/network-observer. So if we were concerned about situations where devtools would not be included in the build, this actually doesn't help because NetworkObserver.sys.mjs will not be able to import its dependencies.

Oh I see. We initially didn't talk about multiple files, so this is unfortunate but understandable.

Another reason is that this pattern make things harder to understand. Usually you expect that the paths used in our imports are somewhat mapped to what is in the filesystem, and we should not break this convention lightly. I think httpd.js is a special case because it is not shipped in most builds. But here I would rather trust that devtools' shared folder is part of all builds (which is the case AFAIK) or if that's not good enough, start thinking about another location (toolkit?).

Is there a build option which let someone disable building devtools? If yes I would see the following options:

1. With our webdriver build config flag set we should require devtools to build as well. Only that way we can ensure that the necessary modules are available. If devtools has been disabled it would need to be enabled.

2. Duplicating the relevant modules but that is clearly not an option regarding maintainability.

3. A shared folder outside of devtools which is always included when devtools or/and webdriver build option flags are set. We should discuss where a good place for that folder would be.

Comment 23

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #22)

Is there a build option which let someone disable building devtools? If yes I would see the following options:

The only build option related to devtools is MOZ_DEVTOOLS, which can either be set to server or all. No other value is accepted, and would result in a build error.

The names are a bit misleading, but if you specify server as the value, then the following folders will be included:

• devtools/platform
• devtools/server
• devtools/shared (that's where the network-observer lives)
• devtools/startup

If you specify all, then all the folders above are included, as well as devtools/client.
I think we are guaranteed that the network-observer files will always be available.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That sounds promising then. In this case lets go ahead with using the devtools specific chrome URLs then. I'm fine with that. We can consider moving them out later if really needed.

Comment 25

[Julian Descottes [:jdescottes]]

Hi Valentin,

For this bug we want to expose some fetch timings, and one of them is described as

Let |fetch start| be [=convert fetch timestamp=] given |timings|' [=post-redirect start time=] and |global|.

Looking at https://searchfox.org/mozilla-central/source/netwerk/base/nsITimedChannel.idl , but I am not sure which timing would be a good fit for "post-redirect start time". Do you have any suggestion?

Comment 26

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

According to the diagram I would expect it to be either dispatchFetchEventStart, cacheReadStart or domainLookupStart. The first one that is non-zero.

Comment 27

[Julian Descottes [:jdescottes]]

Based on the patches I attached here, there are a few things which are still a bit unclear / challenging and that I would like to see move to follow up bugs if possible:

• request cookies: at the moment, the DevTools network observer simply parses the cookies headers and provides that information. Which basically means you only get names and values. This was slightly discussed at https://github.com/w3c/webdriver-bidi/pull/204/files#r978520683. But overall it seems we should send full details about the cookies which were sent, and also add the cookies which were not sent but with a flag. Since that's quite far from what we handle today and the spec is not fully finished around this I would prefer to move this to a follow up
• initiator: resolving the initiator is not entirely clear in the PR yet, beyond "try to match what CDP is doing". So for now I only set "other".
• initiator's stacktrace: the spec PR currently expects the stacktrace for some initiators to be emitted as early as beforeRequestSent. At the moment this is an information we would have to retrieve in content processes (which was not in scope of the initial work), so I would prefer to leave this for a follow up.
• start time timing: I am not sure what to use for start time, as the current spec seems to refer to something which is only relevant for navigation requests? Added a question in the spec PR for this, maybe I am misunderstanding it.

Also, but not new to this patch, but navigation id is not something we can easily generate for now.

Comment 28

[Julian Descottes [:jdescottes]]

Attachment: Bug 1790368 - [devtools] Expose the nsIChannel in NetworkObserver's onNetworkEvent

We need to expose the nsIChannel in order to retrieve all the information needed by BiDi

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6df64c2927f1
[devtools] Expose the nsIChannel in NetworkObserver's onNetworkEvent r=bomsy
https://hg.mozilla.org/integration/autoland/rev/8b438fe8bd72
[bidi] Implement basic support for network.beforeRequestSent event r=webdriver-reviewers,Sasha,whimboo
https://hg.mozilla.org/integration/autoland/rev/a50e9467f30b
[wdspec] Add basic tests for network.beforeRequestSent event r=webdriver-reviewers,whimboo,jgraham
https://hg.mozilla.org/integration/autoland/rev/b04c179c53fb
1790368, 1790368: apply code formatting via Lando

Comment 30

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37546 for changes under testing/web-platform/tests

Comment 31

[Serban Stanca [:SerbanS]]

Backed out for causing mochitests failures in browser_networkobserver_invalid_constructor.js.

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | devtools/shared/network-observer/test/browser/browser_networkobserver_invalid_constructor.js | A promise chain failed to handle a rejection: can't access property "addSecurityInfo", this[#httpActivity].owner is undefined - stack: #getSecurityInfo@resource://devtools/shared/network-observer/NetworkResponseListener.sys.mjs:380:5

Comment 32

[Julian Descottes [:jdescottes]]

I don't see what could trigger this issue in my patch and I also don't have it locally. Will investigate.

Comment 33

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/245ca37205bb
[devtools] Expose the nsIChannel in NetworkObserver's onNetworkEvent r=bomsy
https://hg.mozilla.org/integration/autoland/rev/f12504eb6b43
[bidi] Implement basic support for network.beforeRequestSent event r=webdriver-reviewers,Sasha,whimboo
https://hg.mozilla.org/integration/autoland/rev/d2de6d375dd3
[wdspec] Add basic tests for network.beforeRequestSent event r=webdriver-reviewers,whimboo,jgraham
https://hg.mozilla.org/integration/autoland/rev/1b3d211c3599
1790368, 1790368: apply code formatting via Lando

Comment 35

[Sandor Molnar]

https://hg.mozilla.org/mozilla-central/rev/245ca37205bb
https://hg.mozilla.org/mozilla-central/rev/f12504eb6b43
https://hg.mozilla.org/mozilla-central/rev/d2de6d375dd3
https://hg.mozilla.org/mozilla-central/rev/1b3d211c3599

Comment 36

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 37

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by jgraham