1826192 - Implement "network.addIntercept" command

Status: RESOLVED FIXED

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

Description

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

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

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1826192 - [remote] Add URLPattern module

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1826192 - [bidi] Implement "network.addIntercept" command

Depends on D185452

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1826192 - [wdspec] Add network module for wdspec tests

Depends on D185453

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1826192 - [wdspec] Add wdspec tests for invalid calls to network.addIntercept

Depends on D185454

Comment 6

[Julian Descottes [:jdescottes]]

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

Depends on D185455

Comment 7

[Julian Descottes [:jdescottes]]

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

Depends on D185456

Comment 8

[Julian Descottes [:jdescottes]]

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

Depends on D185457

Comment 9

[Julian Descottes [:jdescottes]]

Hi Valentin,

For WebDriver BiDi request interception, the specifications rely on BasicURLParser to match intercepted URLs with expected patterns (spec PR preview).

Since BasicURLParser is not really implemented or exposed in gecko yet, and new URL does not provide enough insights in some cases (eg it's unable to differentiate between and "no search string" and "empty search string"), I have been using nsIURLParser instead (patch). It's not as complete as the js package https://github.com/jsdom/whatwg-url, but I would like to avoid using a third party JS library if possible.

Do you have any concern with this? Also with the URL work planned for interop 2023, do you think there is a chance BasicURLParser would be implemented in Gecko and exposed to JS?

Comment 10

[Valentin Gosu [:valentin]]

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

Since BasicURLParser is not really implemented or exposed in gecko yet, and new URL does not provide enough insights in some cases (eg it's unable to differentiate between and "no search string" and "empty search string"), I have been using nsIURLParser instead (patch). It's not as complete as the js package https://github.com/jsdom/whatwg-url, but I would like to avoid using a third party JS library if possible.

I would advise against reimplementing a URL parser.
Instead you can use the gecko nsIURI.hasRef (we can add a hasQuery bool if necessary) - you can use NetUtil.newURI directly.

We'll try to look into bug 1731418 soon.

Comment 11

[Julian Descottes [:jdescottes]]

Thanks for the feedback, I'll give a try to NetUtil.newURI.

I would advise against reimplementing a URL parser.

Just to be clear, I did not intend to reimplement a parser, I only used nsIURLParser. There is a bit of code in the patch around that, but that's mostly to wrap the return values in a way which makes sense for us.

Comment 12

[Valentin Gosu [:valentin]]

URL parsing has a lot of edge cases, which would be difficult to mirror in your implementation.
We could end up with a case where new URL(input) throws, but your matchURLPattern(pattern, input) doesn't.

But looking more at the implementation, AFAICT we won't be using this for general urlpattern matching, instead just for isolated matching of URLs that we want to intercept?
If that's the case, this is probably fine. I'll leave the decision whether to proceed to you.

Otherwise I would have suggested the following:
Use new URL to parse the input.
You can then use the DOM attributes or access the inner nsIURI if needed. To figure out whether there's a query, we can implement a GetHasQuery (let me know if you want it, and I can get it to you in 20 minutes).
I see you're already using URL.canParse, so why not make sure we're using the same parsing steps? 🤷

Comment 13

[Julian Descottes [:jdescottes]]

(reducing from 8 points to 5 because the initial work on the URL pattern moves to Bug 1847736).

Comment 14

[Phabricator Automation]

Comment on attachment 9347443 [details]
Bug 1826192 - [remote] Add URLPattern module

Revision D185452 was moved to bug 1847736. Setting attachment 9347443 [details] to obsolete.

Comment 15

[Julian Descottes [:jdescottes]]

(In reply to Valentin Gosu [:valentin] (he/him) {{ PTO 3 July - 6 August }} from comment #12)

Otherwise I would have suggested the following:
Use new URL to parse the input.
You can then use the DOM attributes or access the inner nsIURI if needed. To figure out whether there's a query, we can implement a GetHasQuery (let me know if you want it, and I can get it to you in 20 minutes).
I see you're already using URL.canParse, so why not make sure we're using the same parsing steps? 🤷

Thanks! So I tried out something based on new URL. It works fine for the most part, the only issues are with non-special schemes (eg webpack://) which new URL really doesn't understand, and also differentiating between missing query and empty string query. In theory differentiating between missing path and empty path would also be an issue, but for special schemes, the path is always at least [""], so that's a non-issue.

Provided we can have an API for the query on nsIURI, I think we're not too concerned about non-special schemes because in practice we never should have any request to intercept with such a scheme. I'll file a bug for that.

Comment 16

[Phabricator Automation]

Comment on attachment 9347449 [details]
Bug 1826192 - [wdspec] Add basic wdspec test for add intercept

Revision D185458 was moved to bug 1848156. Setting attachment 9347449 [details] to obsolete.

Comment 17

[Phabricator Automation]

Comment on attachment 9347448 [details]
Bug 1826192 - [wdspec] Update BiDi network event tests to expect interception properties

Revision D185457 was moved to bug 1848156. Setting attachment 9347448 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9347447 [details]
Bug 1826192 - [bidi] Suspend requests which match an intercept on BeforeRequestSent or ResponseStarted

Revision D185456 was moved to bug 1848156. Setting attachment 9347447 [details] to obsolete.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98b285f4bdbd
[bidi] Implement "network.addIntercept" command r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/daaa1164a5ad
[wdspec] Add network module for wdspec tests r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/ca49a99264a1
[wdspec] Add wdspec tests for invalid calls to network.addIntercept 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/41492 for changes under testing/web-platform/tests

Comment 21

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/98b285f4bdbd
https://hg.mozilla.org/mozilla-central/rev/daaa1164a5ad
https://hg.mozilla.org/mozilla-central/rev/ca49a99264a1

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]]

We cannot add this command to the release notes yet given that it is still experimental.