WebExtensionControllerProxy ASSERT in WebProcess when loading a page.

https://webkit.org/b/268511 rdar://problem/122051926 Reviewed by Jeff Miller. The check for HashMap `contains()` ASSERTs return `true`, even though the WeakPtr is null. Check if the pointer is null instead of just contained in the map. Change the `WeakPtr` to be `WeakRef`, to better signal that the map should be only non-null references. Also be explicit about the smart pointers. * Source/WebKit/UIProcess/Extensions/WebExtensionContext.cpp: (WebKit::webExtensionContexts): (WebKit::WebExtensionContext::get): (WebKit::WebExtensionContext::WebExtensionContext): * Source/WebKit/UIProcess/Extensions/WebExtensionController.cpp: (WebKit::webExtensionControllers): (WebKit::WebExtensionController::get): (WebKit::WebExtensionController::WebExtensionController): * Source/WebKit/WebProcess/Extensions/Cocoa/WebExtensionContextProxyCocoa.mm: (WebKit::webExtensionContextProxies): (WebKit::WebExtensionContextProxy::get): (WebKit::WebExtensionContextProxy::WebExtensionContextProxy): (WebKit::WebExtensionContextProxy::~WebExtensionContextProxy): (WebKit::WebExtensionContextProxy::getOrCreate): * Source/WebKit/WebProcess/Extensions/WebExtensionControllerProxy.cpp: (WebKit::webExtensionControllerProxies): (WebKit::WebExtensionControllerProxy::get): (WebKit::WebExtensionControllerProxy::getOrCreate): (WebKit::WebExtensionControllerProxy::WebExtensionControllerProxy): (WebKit::WebExtensionControllerProxy::~WebExtensionControllerProxy): Canonical link: https://commits.webkit.org/273880@main
WebKit · Feb 1, 2024 · ead1bd2 · ead1bd2
1 parent 6c4fe5c
commit ead1bd2
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 36 deletions.
diff --git a/Source/WebKit/UIProcess/Extensions/WebExtensionContext.cpp b/Source/WebKit/UIProcess/Extensions/WebExtensionContext.cpp
@@ -38,22 +38,22 @@ namespace WebKit {
 
 using namespace WebCore;
 
-static HashMap<WebExtensionContextIdentifier, WeakPtr<WebExtensionContext>>& webExtensionContexts()
+static HashMap<WebExtensionContextIdentifier, WeakRef<WebExtensionContext>>& webExtensionContexts()
 {
-    static NeverDestroyed<HashMap<WebExtensionContextIdentifier, WeakPtr<WebExtensionContext>>> contexts;
+    static NeverDestroyed<HashMap<WebExtensionContextIdentifier, WeakRef<WebExtensionContext>>> contexts;
     return contexts;
 }
 
 WebExtensionContext* WebExtensionContext::get(WebExtensionContextIdentifier identifier)
 {
-    return webExtensionContexts().get(identifier).get();
+    return webExtensionContexts().get(identifier);
 }
 
 WebExtensionContext::WebExtensionContext()
     : m_identifier(WebExtensionContextIdentifier::generate())
 {
-    ASSERT(!webExtensionContexts().contains(m_identifier));
-    webExtensionContexts().add(m_identifier, this);
+    ASSERT(!get(m_identifier));
+    webExtensionContexts().add(m_identifier, *this);
 }
 
 WebExtensionContextParameters WebExtensionContext::parameters() const

diff --git a/Source/WebKit/UIProcess/Extensions/WebExtensionController.cpp b/Source/WebKit/UIProcess/Extensions/WebExtensionController.cpp
@@ -39,23 +39,23 @@ namespace WebKit {
 
 constexpr auto freshlyCreatedTimeout = 5_s;
 
-static HashMap<WebExtensionControllerIdentifier, WeakPtr<WebExtensionController>>& webExtensionControllers()
+static HashMap<WebExtensionControllerIdentifier, WeakRef<WebExtensionController>>& webExtensionControllers()
 {
-    static MainThreadNeverDestroyed<HashMap<WebExtensionControllerIdentifier, WeakPtr<WebExtensionController>>> controllers;
+    static MainThreadNeverDestroyed<HashMap<WebExtensionControllerIdentifier, WeakRef<WebExtensionController>>> controllers;
     return controllers;
 }
 
 WebExtensionController* WebExtensionController::get(WebExtensionControllerIdentifier identifier)
 {
-    return webExtensionControllers().get(identifier).get();
+    return webExtensionControllers().get(identifier);
 }
 
 WebExtensionController::WebExtensionController(Ref<WebExtensionControllerConfiguration> configuration)
     : m_configuration(configuration)
     , m_identifier(WebExtensionControllerIdentifier::generate())
 {
-    ASSERT(!webExtensionControllers().contains(m_identifier));
-    webExtensionControllers().add(m_identifier, this);
+    ASSERT(!get(m_identifier));
+    webExtensionControllers().add(m_identifier, *this);
 
     // A freshly created extension controller will be used to determine if the startup event
     // should be fired for any loaded extensions during a brief time window. Start a timer

diff --git a/Source/WebKit/WebProcess/Extensions/Cocoa/WebExtensionContextProxyCocoa.mm b/Source/WebKit/WebProcess/Extensions/Cocoa/WebExtensionContextProxyCocoa.mm
@@ -45,29 +45,29 @@
 
 namespace WebKit {
 
-static HashMap<WebExtensionContextIdentifier, WeakPtr<WebExtensionContextProxy>>& webExtensionContextProxies()
+static HashMap<WebExtensionContextIdentifier, WeakRef<WebExtensionContextProxy>>& webExtensionContextProxies()
 {
-    static MainThreadNeverDestroyed<HashMap<WebExtensionContextIdentifier, WeakPtr<WebExtensionContextProxy>>> contexts;
+    static MainThreadNeverDestroyed<HashMap<WebExtensionContextIdentifier, WeakRef<WebExtensionContextProxy>>> contexts;
     return contexts;
 }
 
 RefPtr<WebExtensionContextProxy> WebExtensionContextProxy::get(WebExtensionContextIdentifier identifier)
 {
-    return webExtensionContextProxies().get(identifier).get();
+    return webExtensionContextProxies().get(identifier);
 }
 
 WebExtensionContextProxy::WebExtensionContextProxy(const WebExtensionContextParameters& parameters)
     : m_identifier(parameters.identifier)
 {
-    ASSERT(!webExtensionContextProxies().contains(m_identifier));
-    webExtensionContextProxies().add(m_identifier, this);
+    ASSERT(!get(m_identifier));
+    webExtensionContextProxies().add(m_identifier, *this);
 
     WebProcess::singleton().addMessageReceiver(Messages::WebExtensionContextProxy::messageReceiverName(), m_identifier, *this);
 }
 
 WebExtensionContextProxy::~WebExtensionContextProxy()
 {
-    WebProcess::singleton().removeMessageReceiver(Messages::WebExtensionContextProxy::messageReceiverName(), m_identifier);
+    WebProcess::singleton().removeMessageReceiver(*this);
 }
 
 Ref<WebExtensionContextProxy> WebExtensionContextProxy::getOrCreate(const WebExtensionContextParameters& parameters, WebPage* newPage)
@@ -84,7 +84,7 @@
         if (parameters.backgroundPageIdentifier) {
             if (newPage && parameters.backgroundPageIdentifier.value() == newPage->identifier())
                 context.setBackgroundPage(*newPage);
-            else if (auto* page = WebProcess::singleton().webPage(parameters.backgroundPageIdentifier.value()))
+            else if (RefPtr page = WebProcess::singleton().webPage(parameters.backgroundPageIdentifier.value()))
                 context.setBackgroundPage(*page);
         }
 
@@ -95,7 +95,7 @@
 
             if (newPage && pageIdentifier == newPage->identifier())
                 context.addPopupPage(*newPage, tabIdentifier, windowIdentifier);
-            else if (auto* page = WebProcess::singleton().webPage(pageIdentifier))
+            else if (RefPtr page = WebProcess::singleton().webPage(pageIdentifier))
                 context.addPopupPage(*page, tabIdentifier, windowIdentifier);
         }
 
@@ -106,19 +106,19 @@
 
             if (newPage && pageIdentifier == newPage->identifier())
                 context.addTabPage(*newPage, tabIdentifier, windowIdentifier);
-            else if (auto* page = WebProcess::singleton().webPage(pageIdentifier))
+            else if (RefPtr page = WebProcess::singleton().webPage(pageIdentifier))
                 context.addTabPage(*page, tabIdentifier, windowIdentifier);
         }
     };
 
-    if (auto context = webExtensionContextProxies().get(parameters.identifier)) {
+    if (RefPtr context = get(parameters.identifier)) {
         updateProperties(*context);
         return *context;
     }
 
-    auto result = adoptRef(new WebExtensionContextProxy(parameters));
-    updateProperties(*result);
-    return result.releaseNonNull();
+    Ref result = adoptRef(*new WebExtensionContextProxy(parameters));
+    updateProperties(result);
+    return result;
 }
 
 _WKWebExtensionLocalization *WebExtensionContextProxy::parseLocalization(API::Data& json, const URL& baseURL)

diff --git a/Source/WebKit/WebProcess/Extensions/WebExtensionControllerProxy.cpp b/Source/WebKit/WebProcess/Extensions/WebExtensionControllerProxy.cpp
@@ -39,15 +39,15 @@ namespace WebKit {
 
 using namespace WebCore;
 
-static HashMap<WebExtensionControllerIdentifier, WeakPtr<WebExtensionControllerProxy>>& webExtensionControllerProxies()
+static HashMap<WebExtensionControllerIdentifier, WeakRef<WebExtensionControllerProxy>>& webExtensionControllerProxies()
 {
-    static MainThreadNeverDestroyed<HashMap<WebExtensionControllerIdentifier, WeakPtr<WebExtensionControllerProxy>>> controllers;
+    static MainThreadNeverDestroyed<HashMap<WebExtensionControllerIdentifier, WeakRef<WebExtensionControllerProxy>>> controllers;
     return controllers;
 }
 
 RefPtr<WebExtensionControllerProxy> WebExtensionControllerProxy::get(WebExtensionControllerIdentifier identifier)
 {
-    return webExtensionControllerProxies().get(identifier).get();
+    return webExtensionControllerProxies().get(identifier);
 }
 
 Ref<WebExtensionControllerProxy> WebExtensionControllerProxy::getOrCreate(const WebExtensionControllerParameters& parameters, WebPage* newPage)
@@ -57,37 +57,37 @@ Ref<WebExtensionControllerProxy> WebExtensionControllerProxy::getOrCreate(const
         WebExtensionContextProxyBaseURLMap baseURLMap;
 
         for (auto& contextParameters : parameters.contextParameters) {
-            auto context = WebExtensionContextProxy::getOrCreate(contextParameters, newPage);
+            Ref context = WebExtensionContextProxy::getOrCreate(contextParameters, newPage);
             baseURLMap.add(contextParameters.baseURL.protocolHostAndPort(), context);
             contexts.add(context);
         }
 
-        controller.m_extensionContexts.swap(contexts);
-        controller.m_extensionContextBaseURLMap.swap(baseURLMap);
+        controller.m_extensionContexts = WTFMove(contexts);
+        controller.m_extensionContextBaseURLMap = WTFMove(baseURLMap);
     };
 
-    if (auto controller = webExtensionControllerProxies().get(parameters.identifier)) {
+    if (RefPtr controller = get(parameters.identifier)) {
         updateProperties(*controller);
         return *controller;
     }
 
-    auto result = adoptRef(new WebExtensionControllerProxy(parameters));
-    updateProperties(*result);
-    return result.releaseNonNull();
+    Ref result = adoptRef(*new WebExtensionControllerProxy(parameters));
+    updateProperties(result);
+    return result;
 }
 
 WebExtensionControllerProxy::WebExtensionControllerProxy(const WebExtensionControllerParameters& parameters)
     : m_identifier(parameters.identifier)
 {
-    ASSERT(!webExtensionControllerProxies().contains(m_identifier));
-    webExtensionControllerProxies().add(m_identifier, this);
+    ASSERT(!get(m_identifier));
+    webExtensionControllerProxies().add(m_identifier, *this);
 
     WebProcess::singleton().addMessageReceiver(Messages::WebExtensionControllerProxy::messageReceiverName(), m_identifier, *this);
 }
 
 WebExtensionControllerProxy::~WebExtensionControllerProxy()
 {
-    WebProcess::singleton().removeMessageReceiver(Messages::WebExtensionControllerProxy::messageReceiverName(), m_identifier);
+    WebProcess::singleton().removeMessageReceiver(*this);
 }
 
 void WebExtensionControllerProxy::load(const WebExtensionContextParameters& contextParameters)