Regression(270308@main) WebPageProxy objects are leaking

https://bugs.webkit.org/show_bug.cgi?id=266803 Reviewed by Ben Nham. 270308@main added a couple of data members which hold a strong reference to the WebPageProxy. This introduced a cycle and we are now leaking WebPageProxy objects. The cycle in question is: WebPageProxy::m_lastMouseMoveHitTestResult <-> API::HitTestResult::m_page Switch to weak pointers to break the cycle. I don't think hit test results should keep the page alive anyway. Thanks to Ben Nham for figuring out which commit caused the memory leak. * Source/WebKit/Shared/API/Cocoa/_WKHitTestResult.mm: (-[_WKHitTestResult frameInfo]): * Source/WebKit/UIProcess/API/APIContextMenuElementInfoMac.h: * Source/WebKit/UIProcess/API/APIHitTestResult.cpp: (API::HitTestResult::create): * Source/WebKit/UIProcess/API/APIHitTestResult.h: (API::HitTestResult::page): (API::HitTestResult::HitTestResult): Canonical link: https://commits.webkit.org/272448@main
WebKit · Dec 22, 2023 · dcca261 · dcca261
1 parent 1a8243f
commit dcca261
Show file tree

Hide file tree

Showing 9 changed files with 17 additions and 15 deletions.
diff --git a/Source/WebKit/Shared/API/Cocoa/_WKHitTestResult.mm b/Source/WebKit/Shared/API/Cocoa/_WKHitTestResult.mm
@@ -133,7 +133,7 @@ - (_WKHitTestResultElementType)elementType
 - (WKFrameInfo *)frameInfo
 {
     if (auto frameInfo = _hitTestResult->frameInfo())
-        return wrapper(API::FrameInfo::create(WTFMove(*frameInfo), &_hitTestResult->page())).autorelease();
+        return wrapper(API::FrameInfo::create(WTFMove(*frameInfo), _hitTestResult->page())).autorelease();
     return nil;
 }
 

diff --git a/Source/WebKit/UIProcess/API/APIContextMenuElementInfoMac.h b/Source/WebKit/UIProcess/API/APIContextMenuElementInfoMac.h
@@ -30,6 +30,7 @@
 #include "APIObject.h"
 #include "ContextMenuContextData.h"
 #include "WebHitTestResultData.h"
+#include <wtf/WeakPtr.h>
 
 namespace WebKit {
 class WebPageProxy;
@@ -45,7 +46,7 @@ class ContextMenuElementInfoMac final : public ObjectImpl<Object::Type::ContextM
     }
 
     const WebKit::WebHitTestResultData& hitTestResultData() const { return m_hitTestResultData; }
-    WebKit::WebPageProxy& page() { return m_page.get(); }
+    WebKit::WebPageProxy* page() { return m_page.get(); }
     const WTF::String& qrCodePayloadString() const { return m_qrCodePayloadString; }
     bool hasEntireImage() const { return m_hasEntireImage; }
 
@@ -57,7 +58,7 @@ class ContextMenuElementInfoMac final : public ObjectImpl<Object::Type::ContextM
         , m_hasEntireImage(data.hasEntireImage()) { }
 
     WebKit::WebHitTestResultData m_hitTestResultData;
-    Ref<WebKit::WebPageProxy> m_page;
+    WeakPtr<WebKit::WebPageProxy> m_page;
     WTF::String m_qrCodePayloadString;
     bool m_hasEntireImage { false };
 };

diff --git a/Source/WebKit/UIProcess/API/APIHitTestResult.cpp b/Source/WebKit/UIProcess/API/APIHitTestResult.cpp
@@ -22,7 +22,7 @@
 
 namespace API {
 
-Ref<HitTestResult> HitTestResult::create(const WebKit::WebHitTestResultData& hitTestResultData, WebKit::WebPageProxy& page)
+Ref<HitTestResult> HitTestResult::create(const WebKit::WebHitTestResultData& hitTestResultData, WebKit::WebPageProxy* page)
 {
     return adoptRef(*new HitTestResult(hitTestResultData, page));
 }

diff --git a/Source/WebKit/UIProcess/API/APIHitTestResult.h b/Source/WebKit/UIProcess/API/APIHitTestResult.h
@@ -29,6 +29,7 @@
 #include <WebCore/PageOverlay.h>
 #include <wtf/Forward.h>
 #include <wtf/RefPtr.h>
+#include <wtf/WeakPtr.h>
 #include <wtf/text/WTFString.h>
 
 namespace IPC {
@@ -46,7 +47,7 @@ class WebFrame;
 
 class HitTestResult : public API::ObjectImpl<API::Object::Type::HitTestResult> {
 public:
-    static Ref<HitTestResult> create(const WebKit::WebHitTestResultData&, WebKit::WebPageProxy&);
+    static Ref<HitTestResult> create(const WebKit::WebHitTestResultData&, WebKit::WebPageProxy*);
 
     WTF::String absoluteImageURL() const { return m_data.absoluteImageURL; }
     WTF::String absolutePDFURL() const { return m_data.absolutePDFURL; }
@@ -76,19 +77,19 @@ class HitTestResult : public API::ObjectImpl<API::Object::Type::HitTestResult> {
 
     WebKit::WebHitTestResultData::ElementType elementType() const { return m_data.elementType; }
 
-    WebKit::WebPageProxy& page() { return m_page.get(); }
+    WebKit::WebPageProxy* page() { return m_page.get(); }
 
     const std::optional<WebKit::FrameInfoData>& frameInfo() const { return m_data.frameInfo; }
 
 private:
-    explicit HitTestResult(const WebKit::WebHitTestResultData& hitTestResultData, WebKit::WebPageProxy& page)
+    explicit HitTestResult(const WebKit::WebHitTestResultData& hitTestResultData, WebKit::WebPageProxy* page)
         : m_data(hitTestResultData)
         , m_page(page)
     {
     }
 
     WebKit::WebHitTestResultData m_data;
-    Ref<WebKit::WebPageProxy> m_page;
+    WeakPtr<WebKit::WebPageProxy> m_page;
 };
 
 } // namespace API
diff --git a/Source/WebKit/UIProcess/API/C/WKPage.cpp b/Source/WebKit/UIProcess/API/C/WKPage.cpp
@@ -981,7 +981,7 @@ void WKPageSetPageContextMenuClient(WKPageRef pageRef, const WKPageContextMenuCl
         {
             if (m_client.base.version >= 4 && m_client.getContextMenuFromProposedMenuAsync) {
                 auto proposedMenuItems = toAPIObjectVector(proposedMenuVector);
-                Ref webHitTestResult = API::HitTestResult::create(hitTestResultData, page);
+                Ref webHitTestResult = API::HitTestResult::create(hitTestResultData, &page);
                 m_client.getContextMenuFromProposedMenuAsync(toAPI(&page), toAPI(API::Array::create(WTFMove(proposedMenuItems)).ptr()), toAPI(&contextMenuListener), toAPI(webHitTestResult.ptr()), toAPI(userData), m_client.base.clientInfo);
                 return;
             }
@@ -1000,7 +1000,7 @@ void WKPageSetPageContextMenuClient(WKPageRef pageRef, const WKPageContextMenuCl
 
             WKArrayRef newMenu = nullptr;
             if (m_client.base.version >= 2) {
-                Ref webHitTestResult = API::HitTestResult::create(hitTestResultData, page);
+                Ref webHitTestResult = API::HitTestResult::create(hitTestResultData, &page);
                 m_client.getContextMenuFromProposedMenu(toAPI(&page), toAPI(API::Array::create(WTFMove(proposedMenuItems)).ptr()), &newMenu, toAPI(webHitTestResult.ptr()), toAPI(userData), m_client.base.clientInfo);
             } else
                 m_client.getContextMenuFromProposedMenu_deprecatedForUseWithV0(toAPI(&page), toAPI(API::Array::create(WTFMove(proposedMenuItems)).ptr()), &newMenu, toAPI(userData), m_client.base.clientInfo);
@@ -1806,7 +1806,7 @@ void WKPageSetPageUIClient(WKPageRef pageRef, const WKPageUIClientBase* wkClient
                 return;
             }
 
-            Ref apiHitTestResult = API::HitTestResult::create(data, page);
+            Ref apiHitTestResult = API::HitTestResult::create(data, &page);
             m_client.mouseDidMoveOverElement(toAPI(&page), toAPI(apiHitTestResult.ptr()), toAPI(modifiers), toAPI(userData), m_client.base.clientInfo);
         }
 

diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKNavigationAction.mm b/Source/WebKit/UIProcess/API/Cocoa/WKNavigationAction.mm
@@ -253,7 +253,7 @@ - (_WKHitTestResult *)_hitTestResult
     if (!page)
         return nil;
 
-    auto apiHitTestResult = API::HitTestResult::create(webHitTestResultData.value(), *page);
+    auto apiHitTestResult = API::HitTestResult::create(webHitTestResultData.value(), page.get());
     return retainPtr(wrapper(apiHitTestResult)).autorelease();
 #else
     return nil;

diff --git a/Source/WebKit/UIProcess/Cocoa/UIDelegate.mm b/Source/WebKit/UIProcess/Cocoa/UIDelegate.mm
@@ -298,7 +298,7 @@
     if (!delegate)
         return;
 
-    auto apiHitTestResult = API::HitTestResult::create(data, page);
+    auto apiHitTestResult = API::HitTestResult::create(data, &page);
 #if PLATFORM(MAC)
     auto modifierFlags = WebEventFactory::toNSEventModifierFlags(modifiers);
 #else

diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
@@ -7448,7 +7448,7 @@ void WebPageProxy::setStatusText(const String& text)
 void WebPageProxy::mouseDidMoveOverElement(WebHitTestResultData&& hitTestResultData, OptionSet<WebEventModifier> modifiers, UserData&& userData)
 {
 #if PLATFORM(MAC)
-    m_lastMouseMoveHitTestResult = API::HitTestResult::create(hitTestResultData, *this);
+    m_lastMouseMoveHitTestResult = API::HitTestResult::create(hitTestResultData, this);
 #endif
     m_uiClient->mouseDidMoveOverElement(*this, hitTestResultData, modifiers, protectedProcess()->transformHandlesToObjects(userData.protectedObject().get()).get());
     setToolTip(hitTestResultData.toolTipText);

diff --git a/Source/WebKit/UIProcess/mac/WKImmediateActionController.mm b/Source/WebKit/UIProcess/mac/WKImmediateActionController.mm
@@ -254,7 +254,7 @@ - (void)immediateActionRecognizerDidCompleteAnimation:(NSImmediateActionGestureR
 {
     RefPtr<API::HitTestResult> hitTestResult;
     if (_state == WebKit::ImmediateActionState::Ready)
-        hitTestResult = API::HitTestResult::create(_hitTestResultData, *_page);
+        hitTestResult = API::HitTestResult::create(_hitTestResultData, _page.get());
     else
         hitTestResult = RefPtr { _page.get() }->lastMouseMoveHitTestResult();