Bug 227924

Created attachment 433457 [details] WIP Patch

Created attachment 433459 [details] WIP Patch

Created attachment 433461 [details] WIP Patch

Created attachment 433467 [details] WIP Patch

Created attachment 433504 [details] Patch

Created attachment 433515 [details] Patch

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 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"

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

(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 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.

(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?).

(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.

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)?

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.

(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.

(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.

(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).

(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.

Created attachment 433551 [details] Patch

Created attachment 433552 [details] Patch

Created attachment 433555 [details] Patch

Created attachment 433559 [details] Patch

Created attachment 433585 [details] Patch

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 on attachment 433585 [details] Patch Thanks for reviewing.

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].

<rdar://problem/80660270>