1850680 - Add support for headers, cookies, method and body arguments for "network.continueRequest" command

Status: RESOLVED FIXED

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

Description

[Julian Descottes [:jdescottes]]

This bug will take care of implementing the network.continueRequest command.

Comment 1

[Julian Descottes [:jdescottes]]

When implementing this command we should also update the interception code in the supported phases to create a promise when a request is intercepted so that we can cleanup the blockedRequests map once the request is resumed and completed.

Comment 2

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

With bug 1874206 the plan is to cover the required arguments only for now.

Comment 3

[Julian Descottes [:jdescottes]]

Hi Valentin,

I know we haven't landed the new observer notification yet (Bug 1849686), but I wanted to check with you if the current requirements for modifying requests with WebDriver BiDi sound feasible. Maybe some of them will actually be more challenging if we intercept the request "late".

The spec bit is covered in https://w3c.github.io/webdriver-bidi/#command-network-continueRequest

For this command, we expect the request to be blocked in the beforeRequestSent phase, which should be done when we receive the notification from Bug 1849686. We want to allow users to modify the following properties of the request:

• body
• headers
• cookies (in that case it's probably just a subset of headers)
• method
• url

Do you think it should be feasible, maybe you have concerns for some of those properties?
I can see some APIs on nsIHttpChannel eg to set headers, but maybe there are other which can be useful here?

Comment 4

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

Changing the URL at that point is probably not very straightforward. HttpBaseChannel::mURI is set in ::Init and stays the same for the entire lifetime of the channel. By the time we send out beforeRequestSent we've already done all the security checks, set the path in the requestHead, etc.
Changing the URL at this point would require us to make additional changes to the channel (not impossible, but right now it wouldn't work - maybe instead of changing the URL we can do an internal redirect instead).
The method can probably be changed at this point - we have HttpBaseChannel::SetRequestMethod - but some checks are performed before this - so the request might not be valid. And I seem to remember we had some assertions that get/head requests must not have a body - but can't find them right now.
Changing the body and headers (including cookies) is probably fine. You can use SetUploadStream to change the body - note that it also sets the methdod.

Comment 5

[Julian Descottes [:jdescottes]]

I have been testing this using another observer notifications: http-on-before-connect, which in theory should be raised earlier than the one we are trying to add.

I could successfully update request headers / cookies thanks to setRequestHeader.

However for both the method and the body, the APIs I used all threw NS_ERROR_IN_PROGRESS. Setting the method for instance uses the ENSURE_CALLED_BEFORE_CONNECT macro.

Valentin, do you think about this? Is it too late to try to update the method/body of the channel?

Comment 6

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

I think that check exists because some security headers/flags/decisions are made after onModifyRequest, and changing them wouldn't be great, especially for your random webextension.
However, in this case I'm thinking we might be able to temporarily relax this constraint?
We can StoreRequestObserversCalled(false); before calling the observer notification, and set it back to true afterwards.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1850680 - [bidi] Create BiDi network events earlier

Depends on D209524

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1850680 - [bidi] Add support for headers, cookies, method and body parameters in continueRequest

Depends on D209538

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1850680 - [bidi] Retrieve request bodysize dynamically

Depends on D209539

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1850680 - [wdspec] Add wdspec tests for network.continueRequest

Depends on D209540

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1850680 - [puppeteer] Update test expectations for continueRequest support

Depends on D209541

Comment 12

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

Hi Julian, I cannot find the support for the url argument. Did you miss that one, or does it need to be added with a follow-up bug due to some complexity?

Comment 13

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

Actually I've asked for an update in the related Phabricator revision.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aeeda14ca643
[bidi] Create BiDi network events earlier r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/1c4084febc9c
[bidi] Add support for headers, cookies, method and body parameters in continueRequest r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/8ff3dc16fcf9
[bidi] Retrieve request bodysize dynamically r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/9a98ace82049
[wdspec] Add wdspec tests for network.continueRequest r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/3147811b0446
[puppeteer] Update test expectations for continueRequest support r=webdriver-reviewers,whimboo

Comment 15

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

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

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/aeeda14ca643
https://hg.mozilla.org/mozilla-central/rev/1c4084febc9c
https://hg.mozilla.org/mozilla-central/rev/8ff3dc16fcf9
https://hg.mozilla.org/mozilla-central/rev/9a98ace82049
https://hg.mozilla.org/mozilla-central/rev/3147811b0446

Comment 17

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

Upstream PR merged by moz-wptsync-bot

Comment 18

[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 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2bbe2132d874
[bidi] Create BiDi network events earlier r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/bcccf3ed3aea
[bidi] Add support for headers, cookies, method and body parameters in continueRequest r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/a09656081892
[bidi] Retrieve request bodysize dynamically r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/7fe452e20b6d
[wdspec] Add wdspec tests for network.continueRequest r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/aa0a319bd0bc
[puppeteer] Update test expectations for continueRequest support r=webdriver-reviewers,whimboo

Comment 20

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

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

Comment 21

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/2bbe2132d874
https://hg.mozilla.org/mozilla-central/rev/bcccf3ed3aea
https://hg.mozilla.org/mozilla-central/rev/a09656081892
https://hg.mozilla.org/mozilla-central/rev/7fe452e20b6d
https://hg.mozilla.org/mozilla-central/rev/aa0a319bd0bc

Comment 22

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

Upstream PR merged by moz-wptsync-bot

Comment 23

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

FYI https://github.com/puppeteer/puppeteer/pull/12483 is the upstream PR to get more tests enabled.