1848156 - Handle intercepted requests in beforeRequestSent and responseStarted phases

Status: RESOLVED FIXED

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

Description

[Julian Descottes [:jdescottes]]

Follow up to Bug 1826192

In Bug 1826192 we will implement a basic addIntercept command, but intercepts will not yet be processed during the beforeRequestSent and responseStarted events.

The goal of this bug is to handle this.

In order to achieve this we will need a parent process mechanism to check if there is currently a listener set for a given event + browsing context. When this is true, we should handle the intercept, suspend the requests, and update the network events to contain the isBlocked and intercepts properties.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1848156 - [wdspec] Add basic wdspec test for add intercept

Depends on D185457

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1848156 - [wdspec] Update BiDi network event tests to expect interception properties

Depends on D185456

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1848156 - [bidi] Suspend requests which match an intercept on BeforeRequestSent or ResponseStarted

Comment 4

[Julian Descottes [:jdescottes]]

Also going to block this on remove intercept, because that makes it much easier to write sane wdspec tests.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1848156 - [remote] Add API to EventsDispatcher to check if an event has listeners

Depends on D186004

Comment 6

[Julian Descottes [:jdescottes]]

Hi Valentin,

For network interception in webdriver bidi, we would need to be able to block requests before the request is actually sent.
This is a known issue for DevTools request blocking: Bug 1756770 and Bug 1803531.

DevTools is using nsIRequest:cancel, and I gave a try to suspend, but for both the requests still reach the server.

Is it possible to fully block a request - maybe we are intercepting them too late ? Or is this a limitation and would need changes on Necko side?

Comment 7

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

As far as I can tell, it's only cancelled in #createNetworkEvent, but that isn't getting called at the right time.
The correct time to cancel the blocked request would be in "http-on-modify-request" here.

Please let me know if that doesn't work.

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #7)

As far as I can tell, it's only cancelled in #createNetworkEvent, but that isn't getting called at the right time.
The correct time to cancel the blocked request would be in "http-on-modify-request" here.

Please let me know if that doesn't work.

Thanks! I can confirm that blocking requests on http-on-modify-request works. It prevents the requests from reaching the server.

In order to build a correct "network.beforeRequestSent" event, I currently need access to the raw headers, which I read from the extraStringData of the ACTIVITY_SUBTYPE_REQUEST_HEADER activity.
Valentin: do you know if there is an API to fetch the raw request headers from a channel as early as http-on-modify-request ?

More details about what needs to change

The main challenge is that we still want emit "beforeRequestSent" for intercepted requests. Which means we need to move the creation of the network event there, and ideally gather all the information required for DevTools and BiDi there.

At the moment we create the network event in different places:

• gActivityDistributor.ACTIVITY_SUBTYPE_REQUEST_HEADER: main path for regular requests
• http-on-examine-cached-response: for cached requests
• http-on-failed-opening-request: blocked (never hit in CI, not sure this codepath is valid)
• http-on-stop-request (if the request was not tracked already)
  • if isSuccessCode -> means we missed the beginning of the request
  • if has blocked reason -> means early blocked (eg CORS or webextension)

http-on-modify-request is handy because I think it is consistently involved in all the paths where we create network events right now, except for late successful http-on-stop-request. So in theory we could start creating the events there and remove a lot of logic. But we sometimes infer properties about the network event based on which codepath created it. For instance, channels detected via http-on-examine-cached-response are flagged as "cached". And it seems this information cannot be swapped with channel.isFromCache which seems to return false at this point, even for cached requests.

In total, the following data is missing when creating events from http-on-modify-request:

• information about raw headers (read from the extraStringData of ACTIVITY_SUBTYPE_REQUEST_HEADER)
• is from cache / service worker
• blocked reason / blocking extension

For cache & blocked, we can probably work around this, because we don't need to know this as early as possible. It just happens that today some of this is read at the network event creation time, but I don't think we strongly need to have it early. It means we'll have to revert some of the simplifications we did in the devtools NetworkObserver, to start collecting data from several spots instead of reading everything at creation time. Of the two, the most problematic is probably the "from cache" bit, because I think channel.isFromCache will still return false in some cases.

For devtools we can also expect some request ordering to change. I already spotted that CORS preflight requests were now detected AFTER the request that triggered it. Which makes sense I think (and matches Chrome) but that means we can expect some failures around that.

Comment 9

[Julian Descottes [:jdescottes]]

Doing more tests, it seems that we don't have access to all the headers in http-on-modify-request.
On one example request, I can see that Upgrade-Insecure-Requests, Pragma and Cache-Control are not returned when visiting headers, but they will be if I wait until ACTIVITY_SUBTYPE_REQUEST_HEADER.

Is this something that can be fixed? Is it possible to intercept a request early enough so that it doesn't reach the server, while still having access to all request headers?

Comment 10

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

Yes, so the last notification we send before connecting is "http-on-before-connect". I think if you listen for that one all of the headers "should" be there.

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

In total, the following data is missing when creating events from http-on-modify-request:

• information about raw headers (read from the extraStringData of ACTIVITY_SUBTYPE_REQUEST_HEADER)

Since we don't end up creating a transaction, we never end up calling ObserveHttpActivityWithArgs.

• is from cache / service worker

I don't think you can have that info before we start the connection.
See the code here

We send the on-before-connect, decide whether to use a SW, then actually connect.

• blocked reason / blocking extension
  For devtools we can also expect some request ordering to change. I already spotted that CORS preflight requests were now detected AFTER the request that triggered it. Which makes sense I think (and matches Chrome) but that means we can expect some failures around that.

Not sure where this happens, and I haven't looked yet. Let me know if this still happens with on-before-connect - I expect it might.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe7f193567fe
[remote] Add API to EventsDispatcher to check if an event has listeners r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/ea4564eaeba7
[bidi] Suspend requests which match an intercept on BeforeRequestSent or ResponseStarted r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/2e19f5156c3f
[wdspec] Update BiDi network event tests to expect interception properties r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/3cc32ff42ef7
[wdspec] Add basic wdspec test for add intercept r=webdriver-reviewers,whimboo

Comment 12

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

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

Comment 13

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/fe7f193567fe
https://hg.mozilla.org/mozilla-central/rev/ea4564eaeba7
https://hg.mozilla.org/mozilla-central/rev/2e19f5156c3f
https://hg.mozilla.org/mozilla-central/rev/3cc32ff42ef7

Comment 14

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

Upstream PR was closed without merging

Comment 15

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

(In reply to Web Platform Test Sync Bot (Matrix: #interop:mozilla.org) from comment #14)

Upstream PR was closed without merging

The PR was closed and immediately reopened. Finally it got merged.

Comment 16

[Rick Byers]

Heads up that the new test webdriver/tests/bidi/network/add_intercept/url_patterns_tentative.py is intermittently timing out in Chromium infrastructure: https://bugs.chromium.org/p/chromium/issues/detail?id=1481752. That looks different from the regression mentioned above and could be a chromium-infrastructure-specific issue so I've assigned it to Chrome's webdriver team to investigate.