Closed Bug 1850680 Opened 9 months ago Closed 6 hours ago

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

Categories

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

task
Points:
5

Tracking

(firefox128 fixed)

RESOLVED FIXED
128 Branch
Tracking Status
firefox128 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 3 open bugs, )

Details

(Whiteboard: [webdriver:m11:blocked], [wptsync upstream])

Attachments

(5 files)

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

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.

Summary: Implement "network.continueRequest" event → Implement "network.continueRequest" command
Whiteboard: [webdriver:m9]
Priority: -- → P3
Priority: P3 → P2
Points: --- → 5
Depends on: 1849686
Whiteboard: [webdriver:m9] → [webdriver:m9:blocked]
Whiteboard: [webdriver:m9:blocked] → [webdriver:m10:blocked]
Depends on: 1874206

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

Summary: Implement "network.continueRequest" command → Add support for optional arguments for "network.continueRequest" command
No longer blocks: 1845345
Depends on: 1880803
No longer depends on: 1849686

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?

Flags: needinfo?(valentin.gosu)

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.

Flags: needinfo?(valentin.gosu)
Whiteboard: [webdriver:m10:blocked] → [webdriver:m11:blocked]

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?

Flags: needinfo?(valentin.gosu)

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.

Flags: needinfo?(valentin.gosu)
Depends on: 1892440
Depends on: 1893117
Depends on: 1895198
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Blocks: 1898158
Summary: Add support for optional arguments for "network.continueRequest" command → Add support for headers, cookies, method and body arguments for "network.continueRequest" command
Pushed by jdescottes@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46456 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m11:blocked] → [webdriver:m11:blocked], [wptsync upstream]
Blocks: 1898565
Upstream PR merged by moz-wptsync-bot
Pushed by jdescottes@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46510 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: