1903060 - Expose an observer notification when a data channel is created in the content process

Status: RESOLVED FIXED

(Core :: Networking, task, P3)

Description

[Alexandra Borovova [:Sasha]]

While adding support for data URLs in the scope of bug 1805176, we've noticed that in the cases when data URLs requested in the content process (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.
There are two solutions here:

• the notification is propagated to parent process on the platform side;
• we set up an observer in the content process on WebDriver BiDi and DevTools side.

Comment 1

[Alexandra Borovova [:Sasha]]

Moving the comment from the discussion on the bug 1805176:
(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.

Comment 2

[Julian Descottes [:jdescottes]]

Just want to reiterate what Sasha mentioned in the description: we can also observe the notifications in content processes if needed. It's a bit more plumbing work to do on our side, but it's feasible. So if it is problematic for necko to bubble those notifications from content to parent (for performance reasons or others), that is still an option here.

Comment 3

[Kershaw Chang [:kershaw]]

For every DataChannel created in content proces, we create a DataChannelChild here, which means there is also a DataChannelParent created in parent process.
I think we can reuse PDataChannel to forward the notification.

Comment 4

[Julian Descottes [:jdescottes]]

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

For every DataChannel created in content proces, we create a DataChannelChild here, which means there is also a DataChannelParent created in parent process.
I think we can reuse PDataChannel to forward the notification.

I was trying to have a go at this, and I managed to get the IPC working (I think) but my issue is that not all child data channels seem connected to their parent. Meaning DataChannelChild::ConnectParent was not called. Any suggestion on how that should be done?

Comment 5

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

This is no longer blocking the Puppeteer announcement.

Comment 6

[Kershaw Chang [:kershaw]]

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

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

For every DataChannel created in content proces, we create a DataChannelChild here, which means there is also a DataChannelParent created in parent process.
I think we can reuse PDataChannel to forward the notification.

I was trying to have a go at this, and I managed to get the IPC working (I think) but my issue is that not all child data channels seem connected to their parent. Meaning DataChannelChild::ConnectParent was not called. Any suggestion on how that should be done?

How about moving gNeckoChild->SendPDataChannelConstructor to the constructor of DataChannelChild to ensure that DataChannelParent is always created for every DataChannel in the child process? We can send the necessary information for devtools in gNeckoChild->SendPDataChannelConstructor.
For handling redirection, I think we can send the channel ID to the parent process only when DataChannelChild::ConnectParent is called.

Comment 7

[Julian Descottes [:jdescottes]]

How about moving gNeckoChild->SendPDataChannelConstructor to the constructor of DataChannelChild

Thanks, that seems to work fine!

To emit and handle the observer notification correctly, it would be nice if the subject of the notification could be an instance of nsIChannel and nsIDataChannel.

Do you think we should introduce some kind of BaseDataChannel class and create it from DataChannelParent based on constructor arguments provided by DataChannelChild?

Comment 8

[Kershaw Chang [:kershaw]]

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

How about moving gNeckoChild->SendPDataChannelConstructor to the constructor of DataChannelChild

Thanks, that seems to work fine!

To emit and handle the observer notification correctly, it would be nice if the subject of the notification could be an instance of nsIChannel and nsIDataChannel.

Do you think we should introduce some kind of BaseDataChannel class and create it from DataChannelParent based on constructor arguments provided by DataChannelChild?

This sounds doable, but it might be easier if we just create the same DataChannel in DataChannelParent. This would mean we need to pass loadInfo and the data URI from content process to parent process.

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1903060 - Expose an observer notification when a data channel is created in the content process

Comment 10

[Julian Descottes [:jdescottes]]

Thanks for the tips! I have something which almost works, I also had to expose the contentType, which we use in BiDi at the moment.
However I still have one issue for the data: channels created for a navigation.

Eg entering data:text/html,test in the URL bar.

If I followed correctly, this will first lead to create a data channel in the parent process (regular nsDataChannel) and then will be redirected to a channel in the content process (DataChannelChild). With my current implementation, I get 2 notifications: a first one for the original nsDataChannel, a second one for the parent actor paired to the DataChannelChild.

I'm not sure what's the best way to address this? Either never notify from nsDataChannel? Or check in DataChannelChild if we are a redirect (ie ConnectToParent was called) and avoid asking the parent actor to notify in this case?

I attached my current patch at https://phabricator.services.mozilla.com/D216165 if you want to take a look.

Thanks!

Comment 11

[Kershaw Chang [:kershaw]]

I'm not sure what's the best way to address this? Either never notify from nsDataChannel? Or check in DataChannelChild if we are a redirect (ie ConnectToParent was called) and avoid asking the parent actor to notify in this case?

Sorry for the late reply.

I might prefer never notifying from nsDataChannel, because it's not easy to inform DataChannelChild that we've already notified (assuming we don't want to add extra parameters when creating a new data channel).

I assume devtools only cares about the DataChannel created from content processes, right? If so, we can always send the notification from DataChannelParent.

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1903060 - [puppeteer] Update test expectations for content data uri support

Depends on D216165

Comment 13

[Julian Descottes [:jdescottes]]

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

I'm not sure what's the best way to address this? Either never notify from nsDataChannel? Or check in DataChannelChild if we are a redirect (ie ConnectToParent was called) and avoid asking the parent actor to notify in this case?

Sorry for the late reply.

I might prefer never notifying from nsDataChannel, because it's not easy to inform DataChannelChild that we've already notified (assuming we don't want to add extra parameters when creating a new data channel).

I assume devtools only cares about the DataChannel created from content processes, right? If so, we can always send the notification from DataChannelParent.

Thanks! Yes that works for me. For DevTools it's probably better to have a consistent source for the notifications anyway. So always going the route Child -> Parent sounds good to me too. Updated the patch.

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: Bug 1903060 - [wdspec] Check for navigation id in data url BiDi network tests

Depends on D217096

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86bb03672ace
Expose an observer notification when a data channel is created in the content process r=necko-reviewers,webdriver-reviewers,Sasha,kershaw
https://hg.mozilla.org/integration/autoland/rev/55a468953a9a
[puppeteer] Update test expectations for content data uri support r=webdriver-reviewers,Sasha
https://hg.mozilla.org/integration/autoland/rev/594b30fb3ea9
[wdspec] Check for navigation id in data url BiDi network tests r=webdriver-reviewers,Sasha

Comment 16

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

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

Comment 17

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/86bb03672ace
https://hg.mozilla.org/mozilla-central/rev/55a468953a9a
https://hg.mozilla.org/mozilla-central/rev/594b30fb3ea9

Comment 18

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

Upstream PR merged by moz-wptsync-bot

Comment 19

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

Julian, for those Puppeteer tests that are passing now we would also need an upstream PR. Shall we maybe somewhere list all the changes that need to be applied for a specific Firefox release? Maybe it could be a meta issue on the Puppeteer repository?

Comment 20

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #19)

Julian, for those Puppeteer tests that are passing now we would also need an upstream PR. Shall we maybe somewhere list all the changes that need to be applied for a specific Firefox release? Maybe it could be a meta issue on the Puppeteer repository?

good point, https://github.com/puppeteer/puppeteer/pull/12850

For the rest let's as puppeteer folks how they want to handle those