Closed Bug 1826192 Opened 1 year ago Closed 9 months ago

Implement "network.addIntercept" command

Categories

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

Product:
Remote Protocol
Component:
WebDriver BiDi
Type:
task
Points:
5

Tracking

(firefox118 fixed)

Status:
RESOLVED FIXED
Milestone:
118 Branch
Tracking Flags:
Tracking Status
firefox118 --- fixed

People

(Reporter: whimboo, Assigned: jdescottes)

References

(Blocks 2 open bugs,
URL
)

Details

(Whiteboard: [webdriver:m8][wptsync upstream])

Attachments

(3 files, 4 obsolete files)

Bug 1826192 - [bidi] Implement "network.addIntercept" command
48 bytes, text/x-phabricator-request
Details | Review
Bug 1826192 - [wdspec] Add network module for wdspec tests
48 bytes, text/x-phabricator-request
Details | Review
Bug 1826192 - [wdspec] Add wdspec tests for invalid calls to network.addIntercept
48 bytes, text/x-phabricator-request
Details | Review

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

Whiteboard: [webdriver:m7]
Priority: P1 → P2
Whiteboard: [webdriver:m7] → [webdriver:m8]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
No longer blocks: 1826198

Depends on D185457

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?

Flags: needinfo?(valentin.gosu)

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

Flags: needinfo?(valentin.gosu)
See Also: → 1731418

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.

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? 🤷

Depends on: 1847736

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

Points: 8 → 5

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

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

Attachment #9347443 - Attachment is obsolete: true

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

Blocks: 1848156

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.

Attachment #9347449 - Attachment is obsolete: true

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.

Attachment #9347448 - Attachment is obsolete: true

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.

Attachment #9347447 - Attachment is obsolete: true
Pushed by jdescottes@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41492 for changes under testing/web-platform/tests
Whiteboard: [webdriver:m8] → [webdriver:m8], [wptsync upstream]

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

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
status-firefox118: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 118 Branch
Upstream PR merged by moz-wptsync-bot
Whiteboard: [webdriver:m8], [wptsync upstream] → [webdriver:m8][wptsync upstream][webdriver:relnote]

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

Whiteboard: [webdriver:m8][wptsync upstream][webdriver:relnote] → [webdriver:m8][wptsync upstream]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: