1992348 - Duplicated request ids for data URLs or cached images resources

Status: RESOLVED FIXED

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

Description

[Julian Descottes [:jdescottes]]

Noticed while testing something else, it seems that the ids created for data channels can be identical to ids created for existing (non-data) channels?

Eg I had for the same session:

{
  "type": "event",
  "method": "network.responseCompleted",
  "params": {
    "context": "2c9b18ff-a7f5-43bc-aec6-563002b7d9df",
    "isBlocked": false,
    "navigation": null,
    "redirectCount": 0,
    "request": {
      "request": "52",
      "url": "data:text/plain,1",
      [...]
    },
    [...]
  }
}

and

{
  "type": "event",
  "method": "network.responseCompleted",
  "params": {
    "context": "2c9b18ff-a7f5-43bc-aec6-563002b7d9df",
    "isBlocked": false,
    "navigation": "3fe27ada-3f1d-4a73-8e82-bdd9e211f6b3",
    "redirectCount": 0,
    "request": {
      "request": "52",
      "url": "https://www.wikipedia.org/",
      [...]
    },
    [...]
  }
}

So both events have the same id "52".

Comment 1

[Julian Descottes [:jdescottes]]

The request id is set at https://searchfox.org/firefox-main/rev/2c8e9bccfd6ce951ceda8d46ab619f461b7fd0bc/remote/shared/NetworkRequest.sys.mjs#79-84

this.#wrappedChannel = ChannelWrapper.get(channel);

this.#redirectCount = this.#timedChannel.redirectCount;
// The wrappedChannel id remains identical across redirects, whereas
// nsIChannel.channelId is different for each and every request.
this.#requestId = this.#wrappedChannel.id.toString();

So it seems that the wrappedChannel might be reusing ids for data channels which have already been attributed to other channels?

Comment 2

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

mozilla::extensions::ChannelWrapper::mId is initialized completely different from mozilla::net::nsHttpHandler::NewChannelId

As far as I can tell, the ChannelWrapper could return the same ID for two different channels if you use them in different processes.

Comment 3

[Julian Descottes [:jdescottes]]

Good point thanks. ChannelWrapper is preferable for us in a few cases (mostly it stays the same across redirects) but then we need to make sure handle the case where the ids come from different processes.

Data urls and cached resources (images/styles) will be handled in content processes, while the rest will be handled in the parent. We could prefix it with a process id. Ideally in each process we could build a ChannelWrapper mId -> UUID, but I'm not sure when we could cleanup the entries so I'm concerned it would grow indefinitely.

Comment 4

[Julian Descottes [:jdescottes]]

This potentially impacts any channel handled in content processes.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1992348 - [bidi] Use unique ids for requests handled in different processes

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1992348 - [wdspec] Add simple test to check that various requests have unique request ids

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8f679077da89
https://hg.mozilla.org/integration/autoland/rev/b77bbd8a24a2
[bidi] Use unique ids for requests handled in different processes r=webdriver-reviewers,whimboo
https://github.com/mozilla-firefox/firefox/commit/b6e55d9685d9
https://hg.mozilla.org/integration/autoland/rev/801c3e7a4ad3
[wdspec] Add simple test to check that various requests have unique request ids r=webdriver-reviewers,Sasha

Comment 8

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

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

Comment 9

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/b77bbd8a24a2
https://hg.mozilla.org/mozilla-central/rev/801c3e7a4ad3

Comment 10

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

Upstream PR merged by moz-wptsync-bot