1805176 - Support network events using data URLs

Status: RESOLVED FIXED

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

Description

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

Similar to DevTools NetMonitor (bug 1000540) but for our WebDriver BiDi implementation. When loading pages via data: or file:/// URLs we should support all the available network events.

Comment 1

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

As decided in one of our last meetings we are going to split off the file protocol case into its own bug.

Comment 2

[Julian Descottes [:jdescottes]]

Hi Valentin,

Do you know if requests to data: URIs are handled at all in Necko? Do we create a channel for those etc...
I think it's not the case but wanted to check.

Thanks!

Comment 3

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

Yes, we have nsDataHandler::NewChannel that creates data channels, and nsDataChannel::OpenContentStream that actually loads that channel into a stream.
We could have an approach similar to the one for file channels, where we send out a notification when one of these is loaded.

If we're only interested in navigations, we might also be able to use DocumentLoadListener::Open to send out the notification.

Comment 4

[Julian Descottes [:jdescottes]]

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

Yes, we have nsDataHandler::NewChannel that creates data channels, and nsDataChannel::OpenContentStream that actually loads that channel into a stream.
We could have an approach similar to the one for file channels, where we send out a notification when one of these is loaded.

If we're only interested in navigations, we might also be able to use DocumentLoadListener::Open to send out the notification.

Thanks! An observer notification would be great, and I confirm we want to monitor all data channels. I will file a bug for Necko.

Comment 5

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

There are 7 (maybe more) Puppeteer unit tests that are failing based on this missing feature. Lets re-evaluate.

Alex, would you consider this as a blocker?

Comment 6

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

We would consider it for addition to the M11 milestone given that a lot of tests might use data URLs.

Comment 7

[alexrudenko]

If it's possible on your side to support these events, it would be great. In Chromium, we only support the events but not interception for data URLs. If it is too difficult to support it, we could exclude it from the scope and document the difference.

Comment 8

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

Julian, would adding support for the network events also add support for interception, or would that need additional work (beside bug 1898158)? If that's the case we should get a bug filed.

Comment 9

[Julian Descottes [:jdescottes]]

I suspect interception will not work out of the box for data URIs. I can't imagine there's a strong case for supporting it either, but maybe that's something we should discuss on the spec.

Comment 10

[Alexandra Borovova [:Sasha]]

Hi Valentin,
I've started working on this bug and notification works fine, but I'm not able to get much information from the channel, maybe some of it doesn't make much sense compare to other types, but we probably need at least headers, and it looks like visitRequestHeaders and visitOriginalResponseHeaders are not available. Do you know if there is maybe another way to get this info, or should/could we just hardcode it?

Similar case with getting the final information about the response (e.g. status code), for regular HTTP requests we listen to stream events but data channel doesn't seem to implement nsITraceableChannel interface, I guess that's why setNewListener method doesn't exist in this case. Again, I guess we could hardcode 200, but maybe there is another way to retrieve this information dynamically?

Thanks!

Comment 11

[Julian Descottes [:jdescottes]]

FWIW, hardcoding some values seems fine spec-wise considering the way the response is built in scheme-fetch for data URI https://fetch.spec.whatwg.org/#concept-scheme-fetch

Return a new response whose status message is OK, header list is « (Content-Type, mimeType) », and body is dataURLStruct’s body as a body.

Seems like we can assume status 200 statusMessage OK and a single header Content-Type? Using a simple fetch await fetch('data:text/plain,42') I also seem to get a Content-Length header.

Comment 12

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

Agreed. Data channels don't have headers or status codes, so hardcoding is the way to go.

Comment 13

[Alexandra Borovova [:Sasha]]

Hi Valentin,

I have another question. :)
I've noticed that in some case (e.g. data URL as src for images on the page, as background images or in fetch requests inside the page) the data-channel-opened notification is sent only in the content process, where at the moment we listen to all the network related notifications in the parent process. Is it maybe possible to bubble them up?

Thank you in advance

Comment 14

[Alexandra Borovova [:Sasha]]

Looks like Valentin is already off. Kershaw, could you maybe help here?

(In reply to Alexandra Borovova [:Sasha] from comment #13)

Hi Valentin,

I have another question. :)
I've noticed that in some case (e.g. data URL as src for images on the page, as background images or in fetch requests inside the page) the data-channel-opened notification is sent only in the content process, where at the moment we listen to all the network related notifications in the parent process. Is it maybe possible to bubble them up?

Thank you in advance

Comment 15

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1805176 - Add support for data URLs in the shared network classes.

Comment 16

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1805176 - [bidi] Add support for network events using data URLs.

Comment 17

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1805176 - [wdspec] Add tests for network events with data URLs.

Comment 18

[Kershaw Chang [:kershaw]]

(In reply to Alexandra Borovova [:Sasha] from comment #13)

Hi Valentin,

I have another question. :)
I've noticed that in some case (e.g. data URL as src for images on the page, as background images or in fetch requests inside the page) the data-channel-opened notification is sent only in the content process, where at the moment we listen to all the network related notifications in the parent process. Is it maybe possible to bubble them up?

Thank you in advance

Yeah, I think that's possible, but it requires some amount of work.
I assume we need to forward the data URL to parent process and open it again.

Comment 19

[Alexandra Borovova [:Sasha]]

(In reply to Kershaw Chang [:kershaw] from comment #18)

(In reply to Alexandra Borovova [:Sasha] from comment #13)

Hi Valentin,

I have another question. :)
I've noticed that in some case (e.g. data URL as src for images on the page, as background images or in fetch requests inside the page) the data-channel-opened notification is sent only in the content process, where at the moment we listen to all the network related notifications in the parent process. Is it maybe possible to bubble them up?

Thank you in advance

Yeah, I think that's possible, but it requires some amount of work.
I assume we need to forward the data URL to parent process and open it again.

Thanks! I've updated the bug 1903060, which I created for this issue. Let me know if you have any problems or concerns with it.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e5c06b95aa0
Add support for data URLs in the shared network classes. r=devtools-reviewers,jdescottes,bomsy
https://hg.mozilla.org/integration/autoland/rev/10d9917879bf
[bidi] Add support for network events using data URLs. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/ccef2524223b
[wdspec] Add tests for network events with data URLs. r=webdriver-reviewers,jdescottes

Comment 21

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

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

Comment 22

[Aron Cseh]

https://hg.mozilla.org/mozilla-central/rev/3e5c06b95aa0
https://hg.mozilla.org/mozilla-central/rev/10d9917879bf
https://hg.mozilla.org/mozilla-central/rev/ccef2524223b

Comment 23

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

Upstream PR merged by moz-wptsync-bot