Use a HashSet<CheckedRef<LocalFrame>> for Page::m_rootFrames

https://bugs.webkit.org/show_bug.cgi?id=262721 rdar://116565539 Reviewed by Ryosuke Niwa. Use a HashSet<CheckedRef<LocalFrame>> for Page::m_rootFrames instead of a WeakHashSet<LocalFrame>. This set is used in a hot code path and using a WeakHashSet is unnecessarily inefficient. * Source/WebCore/page/LocalFrame.cpp: (WebCore::LocalFrame::LocalFrame): (WebCore::LocalFrame::isRootFrame const): * Source/WebCore/page/LocalFrame.h: * Source/WebCore/page/Page.cpp: (WebCore::Page::~Page): (WebCore::Page::removeRootFrame): * Source/WebCore/page/Page.h: (WebCore::Page::rootFrames const): * Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm: (WebKit::RemoteLayerTreeDrawingArea::updateRendering): * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::hasRootFrames): Canonical link: https://commits.webkit.org/269009@main
WebKit · Oct 6, 2023 · 75d236e · 75d236e
1 parent 6c81acd
commit 75d236e
Show file tree

Hide file tree

Showing 6 changed files with 22 additions and 6 deletions.
diff --git a/Source/WebCore/page/LocalFrame.cpp b/Source/WebCore/page/LocalFrame.cpp
@@ -169,6 +169,9 @@ LocalFrame::LocalFrame(Page& page, UniqueRef<LocalFrameLoaderClient>&& frameLoad
     // Pause future ActiveDOMObjects if this frame is being created while the page is in a paused state.
     if (LocalFrame* parent = dynamicDowncast<LocalFrame>(tree().parent()); parent && parent->activeDOMObjectsAndAnimationsSuspended())
         suspendActiveDOMObjectsAndAnimations();
+
+    if (auto page = this->page(); page && isRootFrame())
+        page->removeRootFrame(*this);
 }
 
 void LocalFrame::init()
@@ -217,6 +220,14 @@ LocalFrame::~LocalFrame()
         localMainFrame->selfOnlyDeref();
 }
 
+bool LocalFrame::isRootFrame() const
+{
+    if (auto* parent = tree().parent())
+        return is<RemoteFrame>(parent);
+    ASSERT(&mainFrame() == this);
+    return true;
+}
+
 void LocalFrame::addDestructionObserver(FrameDestructionObserver& observer)
 {
     m_destructionObservers.add(observer);

diff --git a/Source/WebCore/page/LocalFrame.h b/Source/WebCore/page/LocalFrame.h
@@ -32,6 +32,7 @@
 #include "Frame.h"
 #include "ScrollTypes.h"
 #include "UserScriptTypes.h"
+#include <wtf/CheckedRef.h>
 #include <wtf/HashSet.h>
 #include <wtf/UniqueRef.h>
 
@@ -111,7 +112,7 @@ enum OverflowScrollAction { DoNotPerformOverflowScroll, PerformOverflowScroll };
 using NodeQualifier = Function<Node* (const HitTestResult&, Node* terminationNode, IntRect* nodeBounds)>;
 #endif
 
-class LocalFrame final : public Frame {
+class LocalFrame final : public Frame, public CanMakeCheckedPtr {
 public:
     WEBCORE_EXPORT static Ref<LocalFrame> createMainFrame(Page&, UniqueRef<LocalFrameLoaderClient>&&, FrameIdentifier);
     WEBCORE_EXPORT static Ref<LocalFrame> createSubframe(Page&, UniqueRef<LocalFrameLoaderClient>&&, FrameIdentifier, HTMLFrameOwnerElement&);
@@ -152,6 +153,8 @@ class LocalFrame final : public Frame {
     const ScriptController& script() const { return m_script; }
     void resetScript();
 
+    WEBCORE_EXPORT bool isRootFrame() const;
+
     WEBCORE_EXPORT RenderView* contentRenderer() const; // Root of the render tree for the document contained in this frame.
     WEBCORE_EXPORT RenderWidget* ownerRenderer() const; // Renderer for the element that contains this frame.
 

diff --git a/Source/WebCore/page/Page.cpp b/Source/WebCore/page/Page.cpp
@@ -433,6 +433,7 @@ Page::~Page()
     m_validationMessageClient = nullptr;
     m_diagnosticLoggingClient = nullptr;
     m_performanceLoggingClient = nullptr;
+    m_rootFrames.clear();
     if (auto* localMainFrame = dynamicDowncast<LocalFrame>(m_mainFrame.get()))
         localMainFrame->setView(nullptr);
     setGroupName(String());
@@ -4506,7 +4507,6 @@ void Page::addRootFrame(LocalFrame& frame)
 void Page::removeRootFrame(LocalFrame& frame)
 {
     ASSERT(frame.isRootFrame());
-    ASSERT(m_rootFrames.contains(frame));
     m_rootFrames.remove(frame);
 }
 

diff --git a/Source/WebCore/page/Page.h b/Source/WebCore/page/Page.h
@@ -1052,7 +1052,7 @@ class Page : public Supplementable<Page>, public CanMakeWeakPtr<Page>, public Ca
     void willBeginScrolling();
     void didFinishScrolling();
 
-    const WeakHashSet<LocalFrame>& rootFrames() const { return m_rootFrames; }
+    const HashSet<CheckedRef<LocalFrame>>& rootFrames() const { return m_rootFrames; }
     WEBCORE_EXPORT void addRootFrame(LocalFrame&);
     WEBCORE_EXPORT void removeRootFrame(LocalFrame&);
 
@@ -1144,7 +1144,7 @@ class Page : public Supplementable<Page>, public CanMakeWeakPtr<Page>, public Ca
     UniqueRef<ProgressTracker> m_progress;
 
     UniqueRef<BackForwardController> m_backForwardController;
-    WeakHashSet<LocalFrame> m_rootFrames;
+    HashSet<CheckedRef<LocalFrame>> m_rootFrames;
     UniqueRef<EditorClient> m_editorClient;
     Ref<Frame> m_mainFrame;
 

diff --git a/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm b/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm
@@ -316,7 +316,7 @@
         return;
 
     Ref webPage = m_webPage.get();
-    if (auto* page = webPage->corePage(); page && !page->rootFrames().computeSize())
+    if (!webPage->hasRootFrames())
         return;
 
     scaleViewToFitDocumentIfNeeded();

diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
@@ -4746,11 +4746,13 @@ void WebPage::updateRendering()
 
 bool WebPage::hasRootFrames()
 {
-    bool result = m_page && !m_page->rootFrames().isEmptyIgnoringNullReferences();
+    bool result = m_page && !m_page->rootFrames().isEmpty();
+#if ASSERT_ENABLED
     if (!result) {
         ASSERT(m_page->settings().processSwapOnCrossSiteWindowOpenEnabled());
         ASSERT(!m_page->settings().siteIsolationEnabled());
     }
+#endif
     return result;
 }