Closed Bug 1790368 Opened 2 years ago Closed 1 year ago

Implement "network.beforeRequestSent" event

Categories

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

task
Points:
5

Tracking

(firefox110 fixed)

RESOLVED FIXED
110 Branch
Tracking Status
firefox110 --- fixed

People

(Reporter: whimboo, Assigned: jdescottes)

References

(Blocks 3 open bugs, )

Details

(Whiteboard: [webdriver:m5][wptsync upstream][webdriver:relnote])

Attachments

(3 files, 9 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
No description provided.
Points: --- → 5
Assignee: nobody → james
Status: NEW → ASSIGNED

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.

Attachment #9295861 - Attachment is obsolete: true

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

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

Attachment #9295862 - Attachment is obsolete: true

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.

Attachment #9295863 - Attachment is obsolete: true

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.

Attachment #9295864 - Attachment is obsolete: true

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.

Attachment #9295865 - Attachment is obsolete: true

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.

Attachment #9295866 - Attachment is obsolete: true

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.

Attachment #9295867 - Attachment is obsolete: true

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.

Attachment #9295868 - Attachment is obsolete: true

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.

Attachment #9295869 - Attachment is obsolete: true
Assignee: james → nobody
Status: ASSIGNED → NEW
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

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 !

Flags: needinfo?(hskupin)

(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.

Flags: needinfo?(hskupin) → needinfo?(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.

Flags: needinfo?(jdescottes)

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.

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?

Flags: needinfo?(valentin.gosu)

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

Flags: needinfo?(valentin.gosu)
Attachment #9303372 - Attachment description: Bug 1790368 - [bidi] WIP - Use network observer from WebDriver BiDi → Bug 1790368 - [bidi] Implement basic support for network.beforeRequestSent event
Attachment #9303372 - Attachment description: Bug 1790368 - [bidi] Implement basic support for network.beforeRequestSent event → Bug 1790368 - [bidi] WIP - Use network observer from WebDriver BiDi
Attachment #9303372 - Attachment description: Bug 1790368 - [bidi] WIP - Use network observer from WebDriver BiDi → Bug 1790368 - [bidi] Implement basic support for network.beforeRequestSent event
Attachment #9303373 - Attachment description: Bug 1790368 - [wdspec] WIP - Add basic tests for network events → Bug 1790368 - [wdspec] Add basic tests for network.beforeRequestSent event

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.

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

Blocks: 1805405
Blocks: 1805478
Blocks: 1805479
Pushed by jdescottes@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37546 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m5] → [webdriver:m5], [wptsync upstream]

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
Flags: needinfo?(jdescottes)

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

Flags: needinfo?(jdescottes)
Upstream PR was closed without merging
Pushed by jdescottes@mozilla.com:
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
Regressions: 1806661
Upstream PR was closed without merging
Upstream PR merged by jgraham
Whiteboard: [webdriver:m5], [wptsync upstream] → [webdriver:m5][wptsync upstream][webdriver:relnote]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: