Skip to content

Commit

Permalink
[Site Isolation] Owner element should be preserved when switching bet…
Browse files Browse the repository at this point in the history
…ween LocalFrame and RemoteFrame

https://bugs.webkit.org/show_bug.cgi?id=273809
rdar://127638179

Reviewed by Charlie Wolfe.

Otherwise it suddenly becomes null from the point of view of the Frame and DOMWindow,
which prevents things like the load event from flowing to the event listeners.

I also rename createSubframeHostedInAnotherProcess to createProvisionalSubframe because
the contents aren't necessarily hosted in another process.

This test hit a case where sometimes a frame was created and destroyed before the frame
we transitioned from was destroyed, so I updated the assertion to not assert in that case.

* LayoutTests/http/tests/site-isolation/load-event-after-transition-expected.txt: Added.
* LayoutTests/http/tests/site-isolation/load-event-after-transition.html: Added.
* Source/WebCore/page/Frame.cpp:
(WebCore::Frame::setOwnerElement):
* Source/WebCore/page/Frame.h:
* Source/WebCore/page/LocalFrame.cpp:
(WebCore::LocalFrame::createProvisionalSubframe):
(WebCore::LocalFrame::createSubframeHostedInAnotherProcess): Deleted.
* Source/WebCore/page/LocalFrame.h:
* Source/WebKit/WebProcess/WebPage/WebFrame.cpp:
(WebKit::WebFrame::createProvisionalFrame):
(WebKit::WebFrame::commitProvisionalFrame):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm:

Canonical link: https://commits.webkit.org/278472@main
  • Loading branch information
achristensen07 committed May 7, 2024
1 parent 86f6d64 commit 6f62e90
Show file tree
Hide file tree
Showing 8 changed files with 39 additions and 6 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
PASS successfullyParsed is true

TEST COMPLETE
PASS Load event fired after switching to a local frame.

Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<!-- webkit-test-runner [ SiteIsolationEnabled=true ] -->
<script src="/js-test-resources/js-test.js"></script>
<script>
if (window.testRunner) {
testRunner.dumpAsText();
testRunner.waitUntilDone();
}

onload = () => {
let i = document.getElementById('testiframe');
i.src = 'http://127.0.0.1:8000/site-isolation/resources/green-background.html';
i.addEventListener('load', function() {
testPassed("Load event fired after switching to a local frame.");
testRunner.notifyDone();
})
}
</script>
<iframe id='testiframe' src="http://localhost:8000/site-isolation/resources/green-background.html"></iframe>
12 changes: 11 additions & 1 deletion Source/WebCore/page/Frame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ Frame::~Frame()

#if ASSERT_ENABLED
auto it = allFrames().find(m_frameID);
ASSERT(it != allFrames().end());
if (it == allFrames().end())
return;
if (it->value.ptr() == this)
allFrames().remove(it);
else
Expand Down Expand Up @@ -197,4 +198,13 @@ CheckedRef<HistoryController> Frame::checkedHistory() const
return m_history.get();
}

void Frame::setOwnerElement(HTMLFrameOwnerElement* element)
{
m_ownerElement = element;
if (element) {
element->clearContentFrame();
element->setContentFrame(*this);
}
}

} // namespace WebCore
1 change: 1 addition & 0 deletions Source/WebCore/page/Frame.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ class Frame : public ThreadSafeRefCounted<Frame, WTF::DestructionThread::Main>,

WEBCORE_EXPORT void detachFromPage();

WEBCORE_EXPORT void setOwnerElement(HTMLFrameOwnerElement*);
inline HTMLFrameOwnerElement* ownerElement() const; // Defined in HTMLFrameOwnerElement.h.
inline RefPtr<HTMLFrameOwnerElement> protectedOwnerElement() const; // Defined in HTMLFrameOwnerElement.h.

Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/page/LocalFrame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ Ref<LocalFrame> LocalFrame::createSubframe(Page& page, ClientCreator&& clientCre
return adoptRef(*new LocalFrame(page, WTFMove(clientCreator), identifier, &ownerElement, ownerElement.document().frame(), nullptr));
}

Ref<LocalFrame> LocalFrame::createSubframeHostedInAnotherProcess(Page& page, ClientCreator&& clientCreator, FrameIdentifier identifier, Frame& parent)
Ref<LocalFrame> LocalFrame::createProvisionalSubframe(Page& page, ClientCreator&& clientCreator, FrameIdentifier identifier, Frame& parent)
{
return adoptRef(*new LocalFrame(page, WTFMove(clientCreator), identifier, nullptr, &parent, nullptr));
}
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/page/LocalFrame.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class LocalFrame final : public Frame {
using ClientCreator = CompletionHandler<UniqueRef<LocalFrameLoaderClient>(LocalFrame&)>;
WEBCORE_EXPORT static Ref<LocalFrame> createMainFrame(Page&, ClientCreator&&, FrameIdentifier, Frame* opener);
WEBCORE_EXPORT static Ref<LocalFrame> createSubframe(Page&, ClientCreator&&, FrameIdentifier, HTMLFrameOwnerElement&);
WEBCORE_EXPORT static Ref<LocalFrame> createSubframeHostedInAnotherProcess(Page&, ClientCreator&&, FrameIdentifier, Frame& parent);
WEBCORE_EXPORT static Ref<LocalFrame> createProvisionalSubframe(Page&, ClientCreator&&, FrameIdentifier, Frame& parent);

WEBCORE_EXPORT void init();
#if PLATFORM(IOS_FAMILY)
Expand Down
3 changes: 2 additions & 1 deletion Source/WebKit/WebProcess/WebPage/WebFrame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ void WebFrame::createProvisionalFrame(ProvisionalFrameCreationParameters&& param
auto clientCreator = [this, protectedThis = Ref { *this }] (auto& localFrame) mutable {
return makeUniqueRef<WebLocalFrameLoaderClient>(localFrame, WTFMove(protectedThis), makeInvalidator());
};
auto localFrame = parent ? LocalFrame::createSubframeHostedInAnotherProcess(*corePage, WTFMove(clientCreator), m_frameID, *parent) : LocalFrame::createMainFrame(*corePage, WTFMove(clientCreator), m_frameID, remoteFrame->opener());
auto localFrame = parent ? LocalFrame::createProvisionalSubframe(*corePage, WTFMove(clientCreator), m_frameID, *parent) : LocalFrame::createMainFrame(*corePage, WTFMove(clientCreator), m_frameID, remoteFrame->opener());
m_provisionalFrame = localFrame.ptr();
localFrame->init();

Expand Down Expand Up @@ -465,6 +465,7 @@ void WebFrame::commitProvisionalFrame()
m_coreFrame = localFrame.get();
remoteFrame->setView(nullptr);
localFrame->tree().setSpecifiedName(remoteFrame->tree().specifiedName());
localFrame->setOwnerElement(ownerElement.get());
if (remoteFrame->isMainFrame())
corePage->setMainFrame(*localFrame);
localFrame->takeWindowProxyFrom(*remoteFrame);
Expand Down
2 changes: 0 additions & 2 deletions Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm
Original file line number Diff line number Diff line change
Expand Up @@ -2531,8 +2531,6 @@ HTTPServer server({
// FIXME: Make a test that tries to access its parent that used to be remote during a provisional navigation of
// the parent to that domain to verify that even the main frame uses provisional frames.

// FIXME: <rdar://127638179> Call load event on iframe after switching from RemoteFrame to LocalFrame.

TEST(SiteIsolation, OpenThenClose)
{
HTTPServer server({
Expand Down

0 comments on commit 6f62e90

Please sign in to comment.