WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
227924
Add initial support for BroadcastChannel behind a runtime flag
https://bugs.webkit.org/show_bug.cgi?id=227924
Summary
Add initial support for BroadcastChannel behind a runtime flag
Chris Dumez
Reported
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.
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
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2021-07-13 17:02:21 PDT
Created
attachment 433457
[details]
WIP Patch
Chris Dumez
Comment 2
2021-07-13 17:19:22 PDT
Created
attachment 433459
[details]
WIP Patch
Chris Dumez
Comment 3
2021-07-13 17:29:31 PDT
Created
attachment 433461
[details]
WIP Patch
Chris Dumez
Comment 4
2021-07-13 18:15:54 PDT
Created
attachment 433467
[details]
WIP Patch
Chris Dumez
Comment 5
2021-07-14 09:01:25 PDT
Created
attachment 433504
[details]
Patch
Chris Dumez
Comment 6
2021-07-14 11:57:15 PDT
Created
attachment 433515
[details]
Patch
Alex Christensen
Comment 7
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?
Chris Dumez
Comment 8
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"
Alex Christensen
Comment 9
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.
Chris Dumez
Comment 10
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().
Sam Weinig
Comment 11
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.
Chris Dumez
Comment 12
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?).
Sam Weinig
Comment 13
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.
Chris Dumez
Comment 14
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)?
Alex Christensen
Comment 15
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.
Chris Dumez
Comment 16
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.
Sam Weinig
Comment 17
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.
Chris Dumez
Comment 18
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).
Sam Weinig
Comment 19
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.
Chris Dumez
Comment 20
2021-07-14 18:26:15 PDT
Created
attachment 433551
[details]
Patch
Chris Dumez
Comment 21
2021-07-14 18:36:29 PDT
Created
attachment 433552
[details]
Patch
Chris Dumez
Comment 22
2021-07-14 19:02:47 PDT
Created
attachment 433555
[details]
Patch
Chris Dumez
Comment 23
2021-07-14 21:16:39 PDT
Created
attachment 433559
[details]
Patch
Chris Dumez
Comment 24
2021-07-15 08:20:44 PDT
Created
attachment 433585
[details]
Patch
Chris Dumez
Comment 25
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.
Chris Dumez
Comment 26
2021-07-15 17:08:45 PDT
Comment on
attachment 433585
[details]
Patch Thanks for reviewing.
EWS
Comment 27
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]
.
Radar WebKit Bug Importer
Comment 28
2021-07-15 17:37:19 PDT
<
rdar://problem/80660270
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug