Bug 227924 - Add initial support for BroadcastChannel behind a runtime flag
Summary: Add initial support for BroadcastChannel behind a runtime flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 227919
Blocks: 161472 228080
  Show dependency treegraph
 
Reported: 2021-07-13 15:14 PDT by Chris Dumez
Modified: 2021-08-11 14:10 PDT (History)
19 users (show)

See Also:


Attachments
WIP Patch (105.85 KB, patch)
2021-07-13 17:02 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (110.43 KB, patch)
2021-07-13 17:19 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (110.94 KB, patch)
2021-07-13 17:29 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (129.07 KB, patch)
2021-07-13 18:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (157.67 KB, patch)
2021-07-14 09:01 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (158.66 KB, patch)
2021-07-14 11:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (187.44 KB, patch)
2021-07-14 18:26 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (188.01 KB, patch)
2021-07-14 18:36 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (188.01 KB, patch)
2021-07-14 19:02 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (188.01 KB, patch)
2021-07-14 21:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (188.54 KB, patch)
2021-07-15 08:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-07-13 15:14:24 PDT
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.
Comment 1 Chris Dumez 2021-07-13 17:02:21 PDT
Created attachment 433457 [details]
WIP Patch
Comment 2 Chris Dumez 2021-07-13 17:19:22 PDT
Created attachment 433459 [details]
WIP Patch
Comment 3 Chris Dumez 2021-07-13 17:29:31 PDT
Created attachment 433461 [details]
WIP Patch
Comment 4 Chris Dumez 2021-07-13 18:15:54 PDT
Created attachment 433467 [details]
WIP Patch
Comment 5 Chris Dumez 2021-07-14 09:01:25 PDT
Created attachment 433504 [details]
Patch
Comment 6 Chris Dumez 2021-07-14 11:57:15 PDT
Created attachment 433515 [details]
Patch
Comment 7 Alex Christensen 2021-07-14 13:52:14 PDT
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?
Comment 8 Chris Dumez 2021-07-14 13:57:16 PDT
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"
Comment 9 Alex Christensen 2021-07-14 13:59:01 PDT
ListHashSet might be better than Vector in this case.
I think you might also need to do special cleanup if the web process crashes.
Comment 10 Chris Dumez 2021-07-14 14:02:20 PDT
(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().
Comment 11 Sam Weinig 2021-07-14 14:08:07 PDT
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.
Comment 12 Chris Dumez 2021-07-14 14:10:58 PDT
(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?).
Comment 13 Sam Weinig 2021-07-14 14:22:39 PDT
(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.
Comment 14 Chris Dumez 2021-07-14 14:45:51 PDT
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)?
Comment 15 Alex Christensen 2021-07-14 14:49:45 PDT
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.
Comment 16 Chris Dumez 2021-07-14 14:53:05 PDT
(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.
Comment 17 Sam Weinig 2021-07-14 14:54:55 PDT
(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.
Comment 18 Chris Dumez 2021-07-14 15:01:09 PDT
(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).
Comment 19 Sam Weinig 2021-07-14 15:04:45 PDT
(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.
Comment 20 Chris Dumez 2021-07-14 18:26:15 PDT
Created attachment 433551 [details]
Patch
Comment 21 Chris Dumez 2021-07-14 18:36:29 PDT
Created attachment 433552 [details]
Patch
Comment 22 Chris Dumez 2021-07-14 19:02:47 PDT
Created attachment 433555 [details]
Patch
Comment 23 Chris Dumez 2021-07-14 21:16:39 PDT
Created attachment 433559 [details]
Patch
Comment 24 Chris Dumez 2021-07-15 08:20:44 PDT
Created attachment 433585 [details]
Patch
Comment 25 Chris Dumez 2021-07-15 16:15:40 PDT
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.
Comment 26 Chris Dumez 2021-07-15 17:08:45 PDT
Comment on attachment 433585 [details]
Patch

Thanks for reviewing.
Comment 27 EWS 2021-07-15 17:36:46 PDT
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].
Comment 28 Radar WebKit Bug Importer 2021-07-15 17:37:19 PDT
<rdar://problem/80660270>