Add initial support for BroadcastChannel behind a runtime flag

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Add initial support for BroadcastChannel in Window contexts only and behind an off-by-default experimental feature flag.

There is a privacy issue that prevents shipping as is (lack of partitioning: https://github.com/whatwg/html/issues/5803). However, a lot of Web-Platform-Tests rely on BroadcastChannel to test other Web features so having a working implementation is very beneficial when running the layout tests. This is also hopefully a good step towards eventually shipping this since other major browser engines already support it.

BroadcastChannel is supposed to work in Worker environments too but this is not implemented yet.

[Chris Dumez]

Created attachment 433457 [details]
WIP Patch

[Chris Dumez]

Created attachment 433459 [details]
WIP Patch

[Chris Dumez]

Created attachment 433461 [details]
WIP Patch

[Chris Dumez]

Created attachment 433467 [details]
WIP Patch

[Chris Dumez]

Created attachment 433504 [details]
Patch

[Chris Dumez]

Created attachment 433515 [details]
Patch

[Alex Christensen]

Comment on attachment 433515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433515&action=review

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:96
> +      default: false

It might be nice to add a comment saying that it should stay off until https://github.com/whatwg/html/issues/5803 is resolved.

> Source/WebCore/dom/BroadcastChannelRegistry.cpp:35
> +static BroadcastChannelRegistry* globalRegistry;

Is this null-initialized? I can never remember.

> Source/WebCore/dom/BroadcastChannelRegistry.cpp:94
> +        globalRegistry = new LocalBroadcastChannelRegistry;

When would this be called?  WebKitLegacy?

> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.h:32
> +#include "SecurityOriginData.h"

Aren't unified source files fun?

> Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.cpp:76
> +        IPC::Connection::send(globalIdentifier.connectionIdentifier, Messages::WebBroadcastChannelRegistry::PostMessageToRemote(globalIdentifier.channelIndentifierInProcess, message), 0);

If I'm reading this correctly, this makes a communication channel between pages using ephemeral and non-ephemeral data stores.  There's a lot of globals that should probably be owned by something, and we should probably add a test verifying that pages with different data stores cannot communicate with each other.

> Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.h:64
> +    using NameToChannelIdentifiersMap = HashMap<String, Vector<GlobalBroadcastChannelIdentifier>>;

Why a Vector?  Don't we iterate this or remove from it?  Aren't they all unique?  Do we need order?

[Chris Dumez]

Comment on attachment 433515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433515&action=review

>> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:96
>> +      default: false
> 
> It might be nice to add a comment saying that it should stay off until https://github.com/whatwg/html/issues/5803 is resolved.

Sure, I will add a comment here too (I have one in the code and the changelog already).

>> Source/WebCore/dom/BroadcastChannelRegistry.cpp:35
>> +static BroadcastChannelRegistry* globalRegistry;
> 
> Is this null-initialized? I can never remember.

Yes, it is static so 0/null initialized.

>> Source/WebCore/dom/BroadcastChannelRegistry.cpp:94
>> +        globalRegistry = new LocalBroadcastChannelRegistry;
> 
> When would this be called?  WebKitLegacy?

Yes, WebKit1 uses this local registry.

>> Source/WebCore/dom/DeviceOrientationAndMotionAccessController.h:32
>> +#include "SecurityOriginData.h"
> 
> Aren't unified source files fun?

Yes :( quite a bit of it...

>> Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.cpp:76
>> +        IPC::Connection::send(globalIdentifier.connectionIdentifier, Messages::WebBroadcastChannelRegistry::PostMessageToRemote(globalIdentifier.channelIndentifierInProcess, message), 0);
> 
> If I'm reading this correctly, this makes a communication channel between pages using ephemeral and non-ephemeral data stores.  There's a lot of globals that should probably be owned by something, and we should probably add a test verifying that pages with different data stores cannot communicate with each other.

That is a good point. This is likely an issue with my current implementation.

>> Source/WebKit/NetworkProcess/NetworkBroadcastChannelRegistry.h:64
>> +    using NameToChannelIdentifiersMap = HashMap<String, Vector<GlobalBroadcastChannelIdentifier>>;
> 
> Why a Vector?  Don't we iterate this or remove from it?  Aren't they all unique?  Do we need order?

We need ordering as per spec:
- https://html.spec.whatwg.org/multipage/web-messaging.html#dom-broadcastchannel-postmessage

"all BroadcastChannel objects whose relevant agents are the same are sorted in creation order, oldest first"

[Alex Christensen]

ListHashSet might be better than Vector in this case.
I think you might also need to do special cleanup if the web process crashes.

[Chris Dumez]

(In reply to Alex Christensen from comment #9)
> ListHashSet might be better than Vector in this case.

This seems very heavy handed. We don't expect these to be very large vectors.
I am going to end up using more memory.

> I think you might also need to do special cleanup if the web process crashes.

This is already done, see NetworkBroadcastChannelRegistry::removeConnection().

[Sam Weinig]

Comment on attachment 433515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433515&action=review

>>> Source/WebCore/dom/BroadcastChannelRegistry.cpp:35
>>> +static BroadcastChannelRegistry* globalRegistry;
>> 
>> Is this null-initialized? I can never remember.
> 
> Yes, it is static so 0/null initialized.

Can we please try to avoid a singleton for this? I'd much prefer if this owned by the page and allowed WebKit to decide on how the registry is grouped. We almost always regret these singletons. It also seems likely that this will need to work for workers, which makes this design very error prone.

[Chris Dumez]

(In reply to Sam Weinig from comment #11)
> Comment on attachment 433515 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433515&action=review
> 
> >>> Source/WebCore/dom/BroadcastChannelRegistry.cpp:35
> >>> +static BroadcastChannelRegistry* globalRegistry;
> >> 
> >> Is this null-initialized? I can never remember.
> > 
> > Yes, it is static so 0/null initialized.
> 
> Can we please try to avoid a singleton for this? I'd much prefer if this
> owned by the page and allowed WebKit to decide on how the registry is
> grouped. We almost always regret these singletons. It also seems likely that
> this will need to work for workers, which makes this design very error prone.

Sam, this already works with workers in this patch so I don't think that's a concern.
If I move this to the Page, how does it work for workers? And what about when there is more than one Page (do all pages in the same process share the same registry somehow?).

[Sam Weinig]

(In reply to Chris Dumez from comment #12)
> (In reply to Sam Weinig from comment #11)
> > Comment on attachment 433515 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=433515&action=review
> > 
> > >>> Source/WebCore/dom/BroadcastChannelRegistry.cpp:35
> > >>> +static BroadcastChannelRegistry* globalRegistry;
> > >> 
> > >> Is this null-initialized? I can never remember.
> > > 
> > > Yes, it is static so 0/null initialized.
> > 
> > Can we please try to avoid a singleton for this? I'd much prefer if this
> > owned by the page and allowed WebKit to decide on how the registry is
> > grouped. We almost always regret these singletons. It also seems likely that
> > this will need to work for workers, which makes this design very error prone.
> 
> Sam, this already works with workers in this patch so I don't think that's a
> concern.

Fair.

> If I move this to the Page, how does it work for workers? And what about
> when there is more than one Page (do all pages in the same process share the
> same registry somehow?).

The model we use elsewhere for this type of thing is to have an object on Page, and then for workers, use a proxy object, on the worker, that safely forwards to the page.

As for whether you can register the same one with different pages, yes, if that is what the WebKit layer wants to do it can, but isn't required to. By avoiding global state, we give more flexibility and control here. As an example, if we wanted to use the same web process for two different page groups, we could. We probably need to do this for WebKitLegacy anyway, since WebView's with different groupNames should probably not contaminate other WebViews with different groupNames.

I'd really like to see singletons reduced in WebKit to a bear minimum, as they make the code much harder to factor and share, in my experience, and often build in bad assumptions about how WebCore should be used as a library.

[Chris Dumez]

So it seems we want to:
1. Forbid communication between pages that belong to different sessions (I think that makes a lot of sense)
2. Forbid communication between pages that belong to different page groups

I am not very familiar with page groups, looking at the code, it seems like in theory, a page group could have pages from different sessions? It also seems like pages from the same session can belong to different groups. As a result, I don't think I should have a registry per page group or per session. I think we still want a global registry (doesn't necessarily have to be a singleton) but we probably want to use (pageGroupID + sessionID + origin) as key instead of just (origin)?

[Alex Christensen]

Don't worry about page groups.  There's just 2 legacy internal clients using it, and one is about to be removed and the other only uses it for preferences and is also about to be removed.  I'm removing page groups in bug 225611 and another similar bug.

[Chris Dumez]

(In reply to Alex Christensen from comment #15)
> Don't worry about page groups.  There's just 2 legacy internal clients using
> it, and one is about to be removed and the other only uses it for
> preferences and is also about to be removed.  I'm removing page groups in
> bug 225611 and another similar bug.

Great thanks. I'll just add partitioning based on sessionID then. I already have a test to validate that.

I am also looking into moving ownership of the registry to the Page.

[Sam Weinig]

(In reply to Chris Dumez from comment #14)
> So it seems we want to:
> 1. Forbid communication between pages that belong to different sessions (I
> think that makes a lot of sense)
> 2. Forbid communication between pages that belong to different page groups
> 
> I am not very familiar with page groups, looking at the code, it seems like
> in theory, a page group could have pages from different sessions? It also
> seems like pages from the same session can belong to different groups. As a
> result, I don't think I should have a registry per page group or per
> session. I think we still want a global registry (doesn't necessarily have
> to be a singleton) but we probably want to use (pageGroupID + sessionID +
> origin) as key instead of just (origin)?

My experience is that using singletons in WebCore is an anti-pattern we should avoid, and treating WebCore as library makes understanding and working with the code easier. We have existing patterns this can follow, which makes it more similar to existing code and better encapsulates behavior.

[Chris Dumez]

(In reply to Sam Weinig from comment #17)
> (In reply to Chris Dumez from comment #14)
> > So it seems we want to:
> > 1. Forbid communication between pages that belong to different sessions (I
> > think that makes a lot of sense)
> > 2. Forbid communication between pages that belong to different page groups
> > 
> > I am not very familiar with page groups, looking at the code, it seems like
> > in theory, a page group could have pages from different sessions? It also
> > seems like pages from the same session can belong to different groups. As a
> > result, I don't think I should have a registry per page group or per
> > session. I think we still want a global registry (doesn't necessarily have
> > to be a singleton) but we probably want to use (pageGroupID + sessionID +
> > origin) as key instead of just (origin)?
> 
> My experience is that using singletons in WebCore is an anti-pattern we
> should avoid, and treating WebCore as library makes understanding and
> working with the code easier. We have existing patterns this can follow,
> which makes it more similar to existing code and better encapsulates
> behavior.

Not sure how this is a response to my comment? My comment did not argue for the use of a singleton and ask a concrete question (which thankfully Alex answered).

However, since you mention existing patterns, I'll say that this was implemented similarly to MessagePort (which is a super similar feature) and which uses a singleton (MessagePortChannelProvider).

[Sam Weinig]

(In reply to Chris Dumez from comment #18)
> (In reply to Sam Weinig from comment #17)
> > (In reply to Chris Dumez from comment #14)
> > > So it seems we want to:
> > > 1. Forbid communication between pages that belong to different sessions (I
> > > think that makes a lot of sense)
> > > 2. Forbid communication between pages that belong to different page groups
> > > 
> > > I am not very familiar with page groups, looking at the code, it seems like
> > > in theory, a page group could have pages from different sessions? It also
> > > seems like pages from the same session can belong to different groups. As a
> > > result, I don't think I should have a registry per page group or per
> > > session. I think we still want a global registry (doesn't necessarily have
> > > to be a singleton) but we probably want to use (pageGroupID + sessionID +
> > > origin) as key instead of just (origin)?
> > 
> > My experience is that using singletons in WebCore is an anti-pattern we
> > should avoid, and treating WebCore as library makes understanding and
> > working with the code easier. We have existing patterns this can follow,
> > which makes it more similar to existing code and better encapsulates
> > behavior.
> 
> Not sure how this is a response to my comment? My comment did not argue for
> the use of a singleton and ask a concrete question (which thankfully Alex
> answered).
> 
> However, since you mention existing patterns, I'll say that this was
> implemented similarly to MessagePort (which is a super similar feature) and
> which uses a singleton (MessagePortChannelProvider).

Yeah, sorry, didn't really respond to your comment, I was just trying to clarify myself. Apologies.

[Chris Dumez]

Created attachment 433551 [details]
Patch

[Chris Dumez]

Created attachment 433552 [details]
Patch

[Chris Dumez]

Created attachment 433555 [details]
Patch

[Chris Dumez]

Created attachment 433559 [details]
Patch

[Chris Dumez]

Created attachment 433585 [details]
Patch

[Chris Dumez]

I believe I took into consideration both the feedback from Alex and Sam. How does this look? 

I'd like this to start working on the COOP/COEP headers support since their tests 100% rely on BroadcastChannel.

[Chris Dumez]

Comment on attachment 433585 [details]
Patch

Thanks for reviewing.

[EWS]

Committed r279971 (239714@main): <https://commits.webkit.org/239714@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433585 [details].

[Radar WebKit Bug Importer]

<rdar://problem/80660270>