Skip to content

Commit

Permalink
Break RemoteFrame/RemoteFrameView reference cycle
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=264862
rdar://116200737

Reviewed by Pascoe.

This required some work in 3 circumstances:

1. When a LocalFrame is removed in another process and we receive a message to destroy the
   RemoteFrame that represents it in this process
2. When a LocalFrame transitions to a RemoteFrame because a load has committed in another process
3. When a RemoteFrame transitions to a LocalFrame to begin a provisional load in this process

In these circumstances we needed some more teardown logic to break the Frame/FrameView reference
cycle.  Believe it or not, WebKit has never seen a RemoteFrame destructor before today.

To make assertions not fire, I had to make the assertion in
Page::mainFrameDidChangeToNonInitialEmptyDocument allow the main frame to be a RemoteFrame.

In the LocalFrame constructor, having an HTMLFrameOwnerElement always happened in the exact
same circumstances as having a parent frame before site isolation, but with site isolation
we can have a parent RemoteFrame and have no HTMLFrameOwnerElement in this process.  I updated
the conditions for calling selfOnlyRef ot match the conditions for calling selfOnlyDeref.

* Source/WebCore/page/LocalFrame.cpp:
(WebCore::LocalFrame::LocalFrame):
* Source/WebCore/page/Page.cpp:
(WebCore::Page::mainFrameDidChangeToNonInitialEmptyDocument):
* Source/WebCore/page/RemoteFrame.cpp:
(WebCore::m_layerHostingContextIdentifier):
* Source/WebKit/WebProcess/WebPage/WebFrame.cpp:
(WebKit::WebFrame::removeFromTree):
(WebKit::WebFrame::transitionToLocal):

Canonical link: https://commits.webkit.org/270776@main
  • Loading branch information
achristensen07 committed Nov 15, 2023
1 parent cc67c85 commit 8f58d68
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 5 deletions.
2 changes: 1 addition & 1 deletion Source/WebCore/page/LocalFrame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ LocalFrame::LocalFrame(Page& page, UniqueRef<LocalFrameLoaderClient>&& frameLoad
ProcessWarming::initializeNames();
StaticCSSValuePool::init();

if (auto* localMainFrame = dynamicDowncast<LocalFrame>(mainFrame()); localMainFrame && ownerElement)
if (auto* localMainFrame = dynamicDowncast<LocalFrame>(mainFrame()); localMainFrame && parent)
localMainFrame->selfOnlyRef();

#ifndef NDEBUG
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/page/Page.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4294,7 +4294,7 @@ void Page::removeInjectedUserStyleSheet(UserStyleSheet& userStyleSheet)
void Page::mainFrameDidChangeToNonInitialEmptyDocument()
{
auto* localMainFrame = dynamicDowncast<LocalFrame>(m_mainFrame.get());
ASSERT_UNUSED(localMainFrame, localMainFrame && !localMainFrame->loader().stateMachine().isDisplayingInitialEmptyDocument());
ASSERT_UNUSED(localMainFrame, !localMainFrame || !localMainFrame->loader().stateMachine().isDisplayingInitialEmptyDocument());
for (auto& userStyleSheet : m_userStyleSheetsPendingInjection)
injectUserStyleSheet(userStyleSheet);
m_userStyleSheetsPendingInjection.clear();
Expand Down
1 change: 0 additions & 1 deletion Source/WebCore/page/RemoteFrame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ RemoteFrame::RemoteFrame(Page& page, UniqueRef<RemoteFrameClient>&& client, Fram
, m_client(WTFMove(client))
, m_layerHostingContextIdentifier(layerHostingContextIdentifier)
{
// FIXME: We need a corresponding setView(nullptr) during teardown to break the ref cycle. <rdar://116200737>
setView(RemoteFrameView::create(*this));
}

Expand Down
8 changes: 6 additions & 2 deletions Source/WebKit/WebProcess/WebPage/WebFrame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,7 @@ void WebFrame::removeFromTree()
corePage->removeRootFrame(*localFrame);
if (RefPtr parent = coreFrame->tree().parent())
parent->tree().removeChild(*coreFrame);
coreFrame->disconnectView();
}

void WebFrame::transitionToLocal(std::optional<WebCore::LayerHostingContextIdentifier> layerHostingContextIdentifier)
Expand All @@ -431,10 +432,13 @@ void WebFrame::transitionToLocal(std::optional<WebCore::LayerHostingContextIdent
auto client = makeUniqueRef<WebLocalFrameLoaderClient>(*this, WTFMove(invalidator));
auto localFrame = parent ? LocalFrame::createSubframeHostedInAnotherProcess(*corePage, WTFMove(client), m_frameID, *parent) : LocalFrame::createMainFrame(*corePage, WTFMove(client), m_frameID);
m_coreFrame = localFrame.ptr();
if (localFrame->isMainFrame())
corePage->setMainFrame(localFrame);
remoteFrame->setView(nullptr);
localFrame->init();
localFrame->tree().setSpecifiedName(remoteFrame->tree().specifiedName());
if (localFrame->isMainFrame()) {
corePage->setMainFrame(localFrame);
localFrame->takeWindowProxyFrom(*remoteFrame);
}

if (layerHostingContextIdentifier)
setLayerHostingContextIdentifier(*layerHostingContextIdentifier);
Expand Down

0 comments on commit 8f58d68

Please sign in to comment.