Bug 184467

<rdar://problem/39011267>

Created attachment 337643 [details] Patch

Created attachment 337650 [details] Patch

Created attachment 337651 [details] Patch

Created attachment 337653 [details] Patch

Created attachment 337655 [details] Patch

Created attachment 337729 [details] Patch

Comment on attachment 337729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337729&action=review > Source/WebCore/ChangeLog:10 > + Introduce remote variants of Frame / DOMWindow classes, for when these frames / windows > + are hosted on another WebProcess. Those will be used in a follow-up patch. Given what a fundamental change this is, I think this could use a more fleshed out ChangeLog. Specifically, I like to see an overview of how these remote variants will work, especially for things like the Frame/DOMWindow tree, openers, etc. > Source/WebCore/ChangeLog:66 > + * page/RemoteDOMWindow.cpp: Added. > + (WebCore::RemoteDOMWindow::RemoteDOMWindow): > + (WebCore::RemoteDOMWindow::~RemoteDOMWindow): > + (WebCore::RemoteDOMWindow::self const): > + (WebCore::RemoteDOMWindow::location const): > + (WebCore::RemoteDOMWindow::close): > + (WebCore::RemoteDOMWindow::closed const): > + (WebCore::RemoteDOMWindow::focus): > + (WebCore::RemoteDOMWindow::blur): > + (WebCore::RemoteDOMWindow::length const): > + (WebCore::RemoteDOMWindow::top const): > + (WebCore::RemoteDOMWindow::opener const): > + (WebCore::RemoteDOMWindow::parent const): > + (WebCore::RemoteDOMWindow::postMessage): > + * page/RemoteDOMWindow.h: Added. > + (isType): It would be nice to have an explanation of why this subset of the DOMWindow API is being exposed (presumably these are the functions/properties exposed cross origin). > Source/WebCore/loader/ContentFilter.cpp:234 > - static_assert(std::is_base_of<ThreadSafeRefCounted<Frame>, Frame>::value, "Frame must be ThreadSafeRefCounted."); > + static_assert(std::is_base_of<ThreadSafeRefCounted<AbstractFrame>, Frame>::value, "Frame must be ThreadSafeRefCounted."); Should probably update the comment too. > Source/WebCore/page/GlobalFrameIdentifier.h:32 > +struct GlobalFrameIdentifier { I think a comment explaining what this is for would be helpful. > Source/WebCore/page/GlobalWindowIdentifier.h:37 > +struct GlobalWindowIdentifier { I think a comment explaining what this is for would be helpful. > Source/WebCore/page/GlobalWindowIdentifier.h:58 > + unsigned hashes[2]; > + hashes[0] = WTF::intHash(processIdentifier.toUInt64()); > + hashes[1] = WTF::intHash(windowIdentifier.toUInt64()); > + > + return StringHasher::hashMemory(hashes, sizeof(hashes)); I believe we traditionally try not to hash hashes, though I do not remember the math behind that. > Source/WebCore/page/RemoteDOMWindow.cpp:44 > + m_frame->setWindow(nullptr); I'm not clear how/if m_frame can become null, but since you null check it in the functions below, it seems like you should either null check it here, or remove the null checks below and convert m_frame to Ref<RemoteFrame>. > Source/WebCore/page/RemoteDOMWindow.h:57 > + // DOM API. An explanation of why this subset is here would be useful.

Comment on attachment 337729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337729&action=review >> Source/WebCore/ChangeLog:10 >> + are hosted on another WebProcess. Those will be used in a follow-up patch. > > Given what a fundamental change this is, I think this could use a more fleshed out ChangeLog. Specifically, I like to see an overview of how these remote variants will work, especially for things like the Frame/DOMWindow tree, openers, etc. Ok, I'll flesh out the changelog and upload the follow-up patch to a separate bug so you can see how it is used.

Comment on attachment 337729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337729&action=review >>> Source/WebCore/ChangeLog:10 >>> + are hosted on another WebProcess. Those will be used in a follow-up patch. >> >> Given what a fundamental change this is, I think this could use a more fleshed out ChangeLog. Specifically, I like to see an overview of how these remote variants will work, especially for things like the Frame/DOMWindow tree, openers, etc. > > Ok, I'll flesh out the changelog and upload the follow-up patch to a separate bug so you can see how it is used. Note that short-term I only plan to implement window.open() cross origin, not out of process iframes. This makes things a bit simpler as only a top-level window/frame can be remote for now. >> Source/WebCore/ChangeLog:66 >> + (isType): > > It would be nice to have an explanation of why this subset of the DOMWindow API is being exposed (presumably these are the functions/properties exposed cross origin). Yes, this is the API subset that is exposed cross origin. It becomes more obvious in the next patch but I'll improve the changelog. >> Source/WebCore/loader/ContentFilter.cpp:234 >> + static_assert(std::is_base_of<ThreadSafeRefCounted<AbstractFrame>, Frame>::value, "Frame must be ThreadSafeRefCounted."); > > Should probably update the comment too. As you wish, it is still true though :) >> Source/WebCore/page/RemoteDOMWindow.cpp:44 >> + m_frame->setWindow(nullptr); > > I'm not clear how/if m_frame can become null, but since you null check it in the functions below, it seems like you should either null check it here, or remove the null checks below and convert m_frame to Ref<RemoteFrame>. Similarly as for regular DOMWindows, RemoteDOMWindows' frame can become null when that frame gets destroyed. Frameless windows behave very differently.

Comment on attachment 337729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337729&action=review >>>> Source/WebCore/ChangeLog:10 >>>> + are hosted on another WebProcess. Those will be used in a follow-up patch. >>> >>> Given what a fundamental change this is, I think this could use a more fleshed out ChangeLog. Specifically, I like to see an overview of how these remote variants will work, especially for things like the Frame/DOMWindow tree, openers, etc. >> >> Ok, I'll flesh out the changelog and upload the follow-up patch to a separate bug so you can see how it is used. > > Note that short-term I only plan to implement window.open() cross origin, not out of process iframes. This makes things a bit simpler as only a top-level window/frame can be remote for now. I uploaded a more complete patch at https://bugs.webkit.org/show_bug.cgi?id=184515 where this code is used. Feedback is most welcome.

Created attachment 337738 [details] Patch

Comment on attachment 337738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337738&action=review > Source/WebCore/page/DOMWindow.cpp:404 > + : AbstractDOMWindow(GlobalWindowIdentifier { Process::identifier(), generateObjectIdentifier<WindowIdentifierType>() }) Process::identifier() grabs a lock... Can we cache the result in a static local variable for the main thread? > Source/WebCore/page/GlobalFrameIdentifier.h:32 > +// Frame identifier that is unique across all WebContent processes. If this is the case, can't we just make DOMWindow identifier generated from frame ID & some monotonically increasing number? We can't have a DOMWindow without a frame, right? > Source/WebCore/page/GlobalWindowIdentifier.h:39 > + ProcessIdentifier processIdentifier; Can't we just use pageID here too?

(In reply to Ryosuke Niwa from comment #13) > Comment on attachment 337738 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=337738&action=review > > > Source/WebCore/page/DOMWindow.cpp:404 > > + : AbstractDOMWindow(GlobalWindowIdentifier { Process::identifier(), generateObjectIdentifier<WindowIdentifierType>() }) > > Process::identifier() grabs a lock... Can we cache the result in a static > local variable for the main thread? > > > Source/WebCore/page/GlobalFrameIdentifier.h:32 > > +// Frame identifier that is unique across all WebContent processes. > > If this is the case, can't we just make DOMWindow identifier generated from > frame ID & some monotonically increasing number? > We can't have a DOMWindow without a frame, right? > We can totally have a window without a Frame. When a Frame gets navigated, the previous window gets detached from the frame and a new window gets created and attached to the frame. > > Source/WebCore/page/GlobalWindowIdentifier.h:39 > > + ProcessIdentifier processIdentifier; > > Can't we just use pageID here too?

(In reply to Chris Dumez from comment #14) > (In reply to Ryosuke Niwa from comment #13) > > Comment on attachment 337738 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=337738&action=review > > > > > Source/WebCore/page/DOMWindow.cpp:404 > > > + : AbstractDOMWindow(GlobalWindowIdentifier { Process::identifier(), generateObjectIdentifier<WindowIdentifierType>() }) > > > > Process::identifier() grabs a lock... Can we cache the result in a static > > local variable for the main thread? > > > > > Source/WebCore/page/GlobalFrameIdentifier.h:32 > > > +// Frame identifier that is unique across all WebContent processes. > > > > If this is the case, can't we just make DOMWindow identifier generated from > > frame ID & some monotonically increasing number? > > We can't have a DOMWindow without a frame, right? > > > > We can totally have a window without a Frame. When a Frame gets navigated, > the previous window gets detached from the frame and a new window gets > created and attached to the frame. That's fine. We have a frame when we create DOMWindow, and that detached DOMWindow can't inserted back into some other frame, right?

(In reply to Ryosuke Niwa from comment #15) > (In reply to Chris Dumez from comment #14) > > (In reply to Ryosuke Niwa from comment #13) > > > Comment on attachment 337738 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=337738&action=review > > > > > > > Source/WebCore/page/DOMWindow.cpp:404 > > > > + : AbstractDOMWindow(GlobalWindowIdentifier { Process::identifier(), generateObjectIdentifier<WindowIdentifierType>() }) > > > > > > Process::identifier() grabs a lock... Can we cache the result in a static > > > local variable for the main thread? > > > > > > > Source/WebCore/page/GlobalFrameIdentifier.h:32 > > > > +// Frame identifier that is unique across all WebContent processes. > > > > > > If this is the case, can't we just make DOMWindow identifier generated from > > > frame ID & some monotonically increasing number? > > > We can't have a DOMWindow without a frame, right? > > > > > > > We can totally have a window without a Frame. When a Frame gets navigated, > > the previous window gets detached from the frame and a new window gets > > created and attached to the frame. > > That's fine. We have a frame when we create DOMWindow, and that detached > DOMWindow can't inserted back into some other frame, right? That's right. However, since a Window can exist without a Frame / Page. If I omit the Process Identifier, how do you suggest which WebProcess the DOMWindow is in? We cannot rely on the WebPage / WebFrame identifiers because they may no longer exist and therefore, I wouldn't be able to find them in the HashMaps on the UIProcess side. If anything, I think we may have to reconsider the GlobalFrameIdentifier at some point to start using a ProcessIdentifier too (rather than relying on the PageID), as a Frame can be detached from its page.

Comment on attachment 337738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337738&action=review >>>>> Source/WebCore/page/DOMWindow.cpp:404 >>>>> + : AbstractDOMWindow(GlobalWindowIdentifier { Process::identifier(), generateObjectIdentifier<WindowIdentifierType>() }) >>>> >>>> Process::identifier() grabs a lock... Can we cache the result in a static local variable for the main thread? >>> >>> We can totally have a window without a Frame. When a Frame gets navigated, the previous window gets detached from the frame and a new window gets created and attached to the frame. >> >> That's fine. We have a frame when we create DOMWindow, and that detached DOMWindow can't inserted back into some other frame, right? > > That's right. However, since a Window can exist without a Frame / Page. If I omit the Process Identifier, how do you suggest which WebProcess the DOMWindow is in? We cannot rely on the WebPage / WebFrame identifiers because they may no longer exist and therefore, I wouldn't be able to find them in the HashMaps on the UIProcess side. > > If anything, I think we may have to reconsider the GlobalFrameIdentifier at some point to start using a ProcessIdentifier too (rather than relying on the PageID), as a Frame can be detached from its page. Regarding the locking, I'll talk with Brady since he wrote the Process Identifier code. As far as I can tell, there is no need for such locking and we could rewrite the code like so: void setIdentifier(ProcessIdentifier processIdentifier) { ASSERT(isMainThread()); globalIdentifier = processIdentifier; } ProcessIdentifier identifier() { static std::once_flag onceFlag; std::call_once(onceFlag, [] { if (!globalIdentifier) globalIdentifier = generateThreadSafeObjectIdentifier<ProcessIdentifierType>(); }); return *globalIdentifier; } Process::setIdentifier() gets called once, upon ChildProcess initialization, on the main thread. Then, it shouldn't matter that later on, Process::identifier() gets called on background thread since the variable has already been initialized. If it hasn't, then we can rely on the std::call_once() and generateThreadSafeObjectIdentifier for this uncommon case. At least, the common case (identifier has been initialized) would not do any locking.

Okay, then your current approach seems fine to me. We may want to add some DEBUG assertions to make sure the timing of setProcessIdentifier doesn't get changed e.g. to make process startup more lazy.

Comment on attachment 337738 [details] Patch Clearing flags on attachment: 337738 Committed r230613: <https://trac.webkit.org/changeset/230613>

All reviewed patches have been landed. Closing bug.

*** Bug 184151 has been marked as a duplicate of this bug. ***