Allow named frame navigation in RemoteFrames

https://bugs.webkit.org/show_bug.cgi?id=264846
rdar://118363128

Reviewed by Pascoe.

A few places where we made it only work with LocalFrames can be generalized
to work with any type of Frame.  Doing so a little more makes an existing test
start working like it did before site isolation.  We already have a large
task to audit all uses of dynamicDowncast<LocalFrame>, so the parts of this PR
that might not work correctly with site isolation yet will at some point.

No changes affect behavior with site isolation not enabled, and no changes cause
nullptr crashes with site isolation enabled.

* LayoutTests/http/tests/site-isolation/window-open-with-name-expected.txt:
* LayoutTests/http/tests/site-isolation/window-open-with-name.html:
* Source/WebCore/html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::submitIfPossible):
* Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:
(WebCore::InspectorFrontendClientLocal::openURLExternally):
* Source/WebCore/loader/FrameLoader.cpp:
(WebCore::FrameLoader::detachFromAllOpenedFrames):
(WebCore::FrameLoader::loadURL):
(WebCore::FrameLoader::load):
(WebCore::FrameLoader::loadPostRequest):
(WebCore::FrameLoader::findFrameForNavigation):
(WebCore::createWindow):
* Source/WebCore/loader/FrameLoader.h:
* Source/WebCore/page/Frame.h:
* Source/WebCore/page/LocalDOMWindow.cpp:
(WebCore::LocalDOMWindow::opener const):
(WebCore::LocalDOMWindow::createWindow):
* Source/WebCore/page/LocalFrame.cpp:
(WebCore::LocalFrame::setOpener):
* Source/WebCore/page/LocalFrame.h:
* Source/WebCore/page/RemoteFrame.h:

Canonical link: https://commits.webkit.org/270789@main

LayoutTests/http/tests/site-isolation/window-open-with-name-expected.txt

@@ -3,9 +3,8 @@ Verifies window.open with a target name correctly reuses a main frame
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-FAIL first opened window should have been reused
+PASS event.data is 'second opened window received: sent to first window, which should be reused'
 PASS successfullyParsed is true
-Some tests failed.
 
 TEST COMPLETE
 click to run test manually in a browser

LayoutTests/http/tests/site-isolation/window-open-with-name.html

@@ -17,7 +17,7 @@
     } else if (event.data == 'second ping') {
         firstOpenedWindow.postMessage('sent to first window, which should be reused', '*')
     } else if (event.data.startsWith('opened window received')) {
-        testFailed("first opened window should have been reused"); // FIXME: <rdar://118363128> Reuse the main frame and make this test not fail with site isolation enabled.
+        testFailed("first opened window should have been reused");
         finishJSTest();
     } else if (event.data.startsWith('second opened window received')) {
         shouldBe("event.data", "'second opened window received: sent to first window, which should be reused'");

Source/WebCore/html/HTMLFormElement.cpp

@@ -282,7 +282,8 @@ void HTMLFormElement::submitIfPossible(Event* event, HTMLFormControlElement* sub
     if (!targetFrame)
         targetFrame = frame.get();
     auto formState = FormState::create(*this, textFieldValues(), document(), NotSubmittedByJavaScript);
-    targetFrame->loader().client().dispatchWillSendSubmitEvent(WTFMove(formState));
+    if (RefPtr localTargetFrame = dynamicDowncast<LocalFrame>(targetFrame))
+        localTargetFrame->loader().client().dispatchWillSendSubmitEvent(WTFMove(formState));
 
     Ref protectedThis { *this };
 

Source/WebCore/inspector/InspectorFrontendClientLocal.cpp

@@ -290,11 +290,11 @@ void InspectorFrontendClientLocal::openURLExternally(const String& url)
 
     bool created;
     WindowFeatures features;
-    auto frame = WebCore::createWindow(mainFrame, mainFrame, WTFMove(frameLoadRequest), features, created);
+    RefPtr frame = dynamicDowncast<LocalFrame>(WebCore::createWindow(mainFrame, mainFrame, WTFMove(frameLoadRequest), features, created));
     if (!frame)
         return;
 
-    frame->loader().setOpener(mainFrame.copyRef());
+    frame->loader().setOpener(mainFrame.ptr());
     frame->page()->setOpenedByDOM();
 
     // FIXME: Why do we compute the absolute URL with respect to |frame| instead of |mainFrame|?

Source/WebCore/loader/FrameLoader.cpp

@@ -353,9 +353,10 @@ Ref<LocalFrame> FrameLoader::protectedFrame() const
 
 void FrameLoader::detachFromAllOpenedFrames()
 {
-    for (auto& frame : m_openedFrames)
+    for (auto& frame : m_openedFrames) {
         if (RefPtr localFrame = dynamicDowncast<LocalFrame>(frame))
             localFrame->loader().m_opener = nullptr;
+    }
     m_openedFrames.clear();
 }
 
@@ -1424,7 +1425,7 @@ void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& ref
     bool isFormSubmission = formState;
 
     // The search for a target frame is done earlier in the case of form submission.
-    RefPtr targetFrame = isFormSubmission ? nullptr : findFrameForNavigation(effectiveFrameName);
+    RefPtr targetFrame = isFormSubmission ? nullptr : dynamicDowncast<LocalFrame>(findFrameForNavigation(effectiveFrameName));
     if (targetFrame && targetFrame != frame.ptr()) {
         frameLoadRequest.setFrameName(selfTargetFrameName());
         targetFrame->checkedLoader()->loadURL(WTFMove(frameLoadRequest), referrer, newLoadType, event, WTFMove(formState), WTFMove(privateClickMeasurement), completionHandlerCaller.release());
@@ -1535,7 +1536,7 @@ void FrameLoader::load(FrameLoadRequest&& request)
         return;
 
     if (!request.frameName().isEmpty()) {
-        if (RefPtr frame = findFrameForNavigation(request.frameName())) {
+        if (RefPtr frame = dynamicDowncast<LocalFrame>(findFrameForNavigation(request.frameName()))) {
             request.setShouldCheckNewWindowPolicy(false);
             if (&frame->loader() != this) {
                 frame->checkedLoader()->load(WTFMove(request));
@@ -3247,7 +3248,7 @@ void FrameLoader::loadPostRequest(FrameLoadRequest&& request, const String& refe
     workingResourceRequest.setHTTPBody(inRequest.httpBody());
     workingResourceRequest.setHTTPContentType(contentType);
 
-    RefPtr targetFrame = formState || frameName.isEmpty() ? nullptr : findFrameForNavigation(frameName);
+    RefPtr targetFrame = formState || frameName.isEmpty() ? nullptr : dynamicDowncast<LocalFrame>(findFrameForNavigation(frameName));
 
     auto willOpenInNewWindow = !frameName.isEmpty() && !targetFrame ? WillOpenInNewWindow::Yes : WillOpenInNewWindow::No;
     updateRequestAndAddExtraFields(workingResourceRequest, IsMainResource::Yes, loadType, ShouldUpdateAppInitiatedValue::Yes, FrameLoader::IsServiceWorkerNavigationLoad::No, willOpenInNewWindow, &request.requester());
@@ -3986,7 +3987,7 @@ bool FrameLoader::shouldTreatURLAsSrcdocDocument(const URL& url) const
     return ownerElement->hasAttributeWithoutSynchronization(srcdocAttr);
 }
 
-RefPtr<LocalFrame> FrameLoader::findFrameForNavigation(const AtomString& name, Document* activeDocument)
+RefPtr<Frame> FrameLoader::findFrameForNavigation(const AtomString& name, Document* activeDocument)
 {
     // FIXME: Eventually all callers should supply the actual activeDocument so we can call canNavigate with the right document.
     if (!activeDocument)
@@ -3999,8 +4000,7 @@ RefPtr<LocalFrame> FrameLoader::findFrameForNavigation(const AtomString& name, D
     if (!activeDocument->canNavigate(dynamicDowncast<LocalFrame>(frame.get())))
         return nullptr;
 
-    // FIXME: <rdar://118363128> This should return a Frame. If a RemoteFrame with the given name exists, we should use that.
-    return dynamicDowncast<LocalFrame>(frame.get());
+    return frame;
 }
 
 void FrameLoader::loadSameDocumentItem(HistoryItem& item)
@@ -4368,7 +4368,7 @@ bool LocalFrameLoaderClient::hasHTMLView() const
     return true;
 }
 
-RefPtr<LocalFrame> createWindow(LocalFrame& openerFrame, LocalFrame& lookupFrame, FrameLoadRequest&& request, WindowFeatures& features, bool& created)
+RefPtr<Frame> createWindow(LocalFrame& openerFrame, LocalFrame& lookupFrame, FrameLoadRequest&& request, WindowFeatures& features, bool& created)
 {
     ASSERT(!features.dialog || request.frameName().isEmpty());
     ASSERT(request.resourceRequest().httpMethod() == "GET"_s);
@@ -4424,12 +4424,12 @@ RefPtr<LocalFrame> createWindow(LocalFrame& openerFrame, LocalFrame& lookupFrame
     if (!page)
         return nullptr;
 
-    RefPtr frame = dynamicDowncast<LocalFrame>(page->mainFrame());
-    if (!frame)
-        return nullptr;
+    Ref frame = page->mainFrame();
 
-    if (isDocumentSandboxed(openerFrame, SandboxPropagatesToAuxiliaryBrowsingContexts))
-        frame->checkedLoader()->forceSandboxFlags(openerFrame.document()->sandboxFlags());
+    if (isDocumentSandboxed(openerFrame, SandboxPropagatesToAuxiliaryBrowsingContexts)) {
+        if (RefPtr localFrame = dynamicDowncast<LocalFrame>(frame))
+            localFrame->checkedLoader()->forceSandboxFlags(openerFrame.document()->sandboxFlags());
+    }
 
     if (!isBlankTargetFrameName(request.frameName()))
         frame->tree().setSpecifiedName(request.frameName());
@@ -4497,7 +4497,8 @@ RefPtr<LocalFrame> createWindow(LocalFrame& openerFrame, LocalFrame& lookupFrame
         arguments.width = *features.width;
     if (features.height && *features.height)
         arguments.height = *features.height;
-    frame->setViewportArguments(arguments);
+    if (RefPtr localFrame = dynamicDowncast<LocalFrame>(frame))
+        localFrame->setViewportArguments(arguments);
 #endif
 
     if (!frame->page())

Source/WebCore/loader/FrameLoader.h

@@ -284,7 +284,7 @@ class FrameLoader final : public CanMakeCheckedPtr {
 
     void advanceStatePastInitialEmptyDocument();
 
-    WEBCORE_EXPORT RefPtr<LocalFrame> findFrameForNavigation(const AtomString& name, Document* activeDocument = nullptr);
+    WEBCORE_EXPORT RefPtr<Frame> findFrameForNavigation(const AtomString& name, Document* activeDocument = nullptr);
 
     void applyUserAgentIfNeeded(ResourceRequest&);
 
@@ -533,6 +533,6 @@ class FrameLoader final : public CanMakeCheckedPtr {
 //
 // FIXME: Consider making this function part of an appropriate class (not FrameLoader)
 // and moving it to a more appropriate location.
-RefPtr<LocalFrame> createWindow(LocalFrame& openerFrame, LocalFrame& lookupFrame, FrameLoadRequest&&, WindowFeatures&, bool& created);
+RefPtr<Frame> createWindow(LocalFrame& openerFrame, LocalFrame& lookupFrame, FrameLoadRequest&&, WindowFeatures&, bool& created);
 
 } // namespace WebCore

Source/WebCore/page/Frame.h

@@ -86,6 +86,7 @@ class Frame : public ThreadSafeRefCounted<Frame, WTF::DestructionThread::Main>,
 
     virtual FrameView* virtualView() const = 0;
     virtual void disconnectView() = 0;
+    virtual void setOpener(Frame*) = 0;
     virtual const Frame* opener() const = 0;
     virtual Frame* opener() = 0;
 

Source/WebCore/page/LocalDOMWindow.cpp

@@ -1508,6 +1508,7 @@ void LocalDOMWindow::setStatus(const String& string)
 
 WindowProxy* LocalDOMWindow::opener() const
 {
+    // FIXME: <rdar://118263278> Move LocalDOMWindow::opener and RemoteDOMWindow::opener to DOMWindow.
     RefPtr frame = this->frame();
     if (!frame)
         return nullptr;
@@ -2611,23 +2612,24 @@ ExceptionOr<RefPtr<LocalFrame>> LocalDOMWindow::createWindow(const String& urlSt
 
     bool noopener = windowFeatures.wantsNoOpener();
     if (!noopener)
-        newFrame->checkedLoader()->setOpener(&openerFrame);
+        newFrame->setOpener(&openerFrame);
 
     if (created)
         newFrame->checkedPage()->setOpenedByDOM();
 
-    if (newFrame->document()->domWindow()->isInsecureScriptAccess(activeWindow, completedURL.string()))
-        return noopener ? RefPtr<LocalFrame> { nullptr } : newFrame;
+    RefPtr localNewFrame = dynamicDowncast<LocalFrame>(newFrame);
+    if (localNewFrame && localNewFrame->document()->domWindow()->isInsecureScriptAccess(activeWindow, completedURL.string()))
+        return noopener ? RefPtr<LocalFrame> { nullptr } : localNewFrame;
 
-    if (prepareDialogFunction)
-        prepareDialogFunction(*newFrame->document()->protectedWindow());
+    if (prepareDialogFunction && localNewFrame)
+        prepareDialogFunction(*localNewFrame->document()->protectedWindow());
 
     if (created) {
         ResourceRequest resourceRequest { completedURL, referrer, ResourceRequestCachePolicy::UseProtocolCachePolicy };
         FrameLoader::addSameSiteInfoToRequestIfNeeded(resourceRequest, openerFrame.protectedDocument().get());
         FrameLoadRequest frameLoadRequest { activeWindow.protectedDocument().releaseNonNull(), activeWindow.document()->securityOrigin(), WTFMove(resourceRequest), selfTargetFrameName(), initiatedByMainFrame };
         frameLoadRequest.setShouldOpenExternalURLsPolicy(activeDocument->shouldOpenExternalURLsPolicyToPropagate());
-        newFrame->checkedLoader()->changeLocation(WTFMove(frameLoadRequest));
+        newFrame->changeLocation(WTFMove(frameLoadRequest));
     } else if (!urlString.isEmpty()) {
         LockHistory lockHistory = UserGestureIndicator::processingUserGesture() ? LockHistory::No : LockHistory::Yes;
         newFrame->checkedNavigationScheduler()->scheduleLocationChange(*activeDocument, activeDocument->securityOrigin(), completedURL, referrer, lockHistory, LockBackForwardList::No);
@@ -2637,7 +2639,7 @@ ExceptionOr<RefPtr<LocalFrame>> LocalDOMWindow::createWindow(const String& urlSt
     if (!newFrame->page())
         return RefPtr<LocalFrame> { nullptr };
 
-    return noopener ? RefPtr<LocalFrame> { nullptr } : newFrame;
+    return noopener ? RefPtr<LocalFrame> { nullptr } : localNewFrame;
 }
 
 ExceptionOr<RefPtr<WindowProxy>> LocalDOMWindow::open(LocalDOMWindow& activeWindow, LocalDOMWindow& firstWindow, const String& urlStringToOpen, const AtomString& frameName, const String& windowFeaturesString)

Source/WebCore/page/LocalFrame.cpp

@@ -971,6 +971,11 @@ void LocalFrame::disconnectView()
     setView(nullptr);
 }
 
+void LocalFrame::setOpener(Frame* opener)
+{
+    loader().setOpener(opener);
+}
+
 const Frame* LocalFrame::opener() const
 {
     return loader().opener();

Source/WebCore/page/LocalFrame.h

@@ -322,6 +322,7 @@ class LocalFrame final : public Frame {
     FrameView* virtualView() const final;
     void disconnectView() final;
     DOMWindow* virtualWindow() const final;
+    void setOpener(Frame*) final;
     const Frame* opener() const final;
     Frame* opener();
 

Source/WebCore/page/RemoteFrame.h

@@ -49,8 +49,7 @@ class RemoteFrame final : public Frame {
 
     RemoteDOMWindow& window() const;
 
-    // FIXME: <rdar://118263278> Move this to a pure virtual function on Frame or just a function on Frame, move LocalDOMWindow::opener to DOMWindow.
-    void setOpener(Frame* opener) { m_opener = opener; }
+    void setOpener(Frame* opener) final { m_opener = opener; }
     Frame* opener() final { return m_opener.get(); }
     const Frame* opener() const final { return m_opener.get(); }