Use WeakHashSet for WebUserContentControllerProxy::m_processes
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jul 2019 00:53:26 +0000 (00:53 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jul 2019 00:53:26 +0000 (00:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199591
<rdar://problem/52798721>

Reviewed by Youenn Fablet.

Source/WebKit:

Use WeakHashSet for WebUserContentControllerProxy::m_processses for safety. In theory, a WebProcessProxy could
stay in the map if we failed to call WebProcessProxy::shutDown() before destroying it.

* UIProcess/UserContent/WebUserContentControllerProxy.cpp:
(WebKit::WebUserContentControllerProxy::~WebUserContentControllerProxy):
(WebKit::WebUserContentControllerProxy::addProcess):
(WebKit::WebUserContentControllerProxy::removeProcess):
(WebKit::WebUserContentControllerProxy::addUserContentWorldUse):
(WebKit::WebUserContentControllerProxy::removeUserContentWorldUses):
(WebKit::WebUserContentControllerProxy::addUserScript):
(WebKit::WebUserContentControllerProxy::removeUserScript):
(WebKit::WebUserContentControllerProxy::removeAllUserScripts):
(WebKit::WebUserContentControllerProxy::addUserStyleSheet):
(WebKit::WebUserContentControllerProxy::removeUserStyleSheet):
(WebKit::WebUserContentControllerProxy::removeAllUserStyleSheets):
(WebKit::WebUserContentControllerProxy::addUserScriptMessageHandler):
(WebKit::WebUserContentControllerProxy::removeUserMessageHandlerForName):
(WebKit::WebUserContentControllerProxy::removeAllUserMessageHandlers):
(WebKit::WebUserContentControllerProxy::addContentRuleList):
(WebKit::WebUserContentControllerProxy::removeContentRuleList):
(WebKit::WebUserContentControllerProxy::removeAllContentRuleLists):
* UIProcess/UserContent/WebUserContentControllerProxy.h:
(WebKit::WebUserContentControllerProxy::addNetworkProcess):
(WebKit::WebUserContentControllerProxy::removeNetworkProcess):

Source/WTF:

Update WeakHashSet::add() to return an AddResult type, similarly to our other containers.

* wtf/WeakHashSet.h:
(WTF::WeakHashSet::add):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247243 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/WeakHashSet.h
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/UserContent/WebUserContentControllerProxy.cpp
Source/WebKit/UIProcess/UserContent/WebUserContentControllerProxy.h

index ca46662..9119267 100644 (file)
@@ -1,3 +1,16 @@
+2019-07-08  Chris Dumez  <cdumez@apple.com>
+
+        Use WeakHashSet for WebUserContentControllerProxy::m_processes
+        https://bugs.webkit.org/show_bug.cgi?id=199591
+        <rdar://problem/52798721>
+
+        Reviewed by Youenn Fablet.
+
+        Update WeakHashSet::add() to return an AddResult type, similarly to our other containers.
+
+        * wtf/WeakHashSet.h:
+        (WTF::WeakHashSet::add):
+
 2019-07-08  Christopher Reid  <chris.reid@sony.com>
 
         Implement MappedFileData for Windows
index 0728cf3..1bc3690 100644 (file)
@@ -44,6 +44,7 @@ template <typename T>
 class WeakHashSet {
 public:
     typedef HashSet<Ref<WeakPtrImpl>> WeakPtrImplSet;
+    typedef typename WeakPtrImplSet::AddResult AddResult;
 
     class WeakHashSetConstIterator : public std::iterator<std::forward_iterator_tag, T, std::ptrdiff_t, const T*, const T&> {
     private:
@@ -96,9 +97,9 @@ public:
     const_iterator end() const { return WeakHashSetConstIterator(m_set, m_set.end()); }
 
     template <typename U>
-    void add(const U& value)
+    AddResult add(const U& value)
     {
-        m_set.add(*makeWeakPtr<T>(const_cast<U&>(value)).m_impl);
+        return m_set.add(*makeWeakPtr<T>(const_cast<U&>(value)).m_impl);
     }
 
     template <typename U>
index 1ca0008..3d2cbb9 100644 (file)
@@ -1,3 +1,36 @@
+2019-07-08  Chris Dumez  <cdumez@apple.com>
+
+        Use WeakHashSet for WebUserContentControllerProxy::m_processes
+        https://bugs.webkit.org/show_bug.cgi?id=199591
+        <rdar://problem/52798721>
+
+        Reviewed by Youenn Fablet.
+
+        Use WeakHashSet for WebUserContentControllerProxy::m_processses for safety. In theory, a WebProcessProxy could
+        stay in the map if we failed to call WebProcessProxy::shutDown() before destroying it.
+
+        * UIProcess/UserContent/WebUserContentControllerProxy.cpp:
+        (WebKit::WebUserContentControllerProxy::~WebUserContentControllerProxy):
+        (WebKit::WebUserContentControllerProxy::addProcess):
+        (WebKit::WebUserContentControllerProxy::removeProcess):
+        (WebKit::WebUserContentControllerProxy::addUserContentWorldUse):
+        (WebKit::WebUserContentControllerProxy::removeUserContentWorldUses):
+        (WebKit::WebUserContentControllerProxy::addUserScript):
+        (WebKit::WebUserContentControllerProxy::removeUserScript):
+        (WebKit::WebUserContentControllerProxy::removeAllUserScripts):
+        (WebKit::WebUserContentControllerProxy::addUserStyleSheet):
+        (WebKit::WebUserContentControllerProxy::removeUserStyleSheet):
+        (WebKit::WebUserContentControllerProxy::removeAllUserStyleSheets):
+        (WebKit::WebUserContentControllerProxy::addUserScriptMessageHandler):
+        (WebKit::WebUserContentControllerProxy::removeUserMessageHandlerForName):
+        (WebKit::WebUserContentControllerProxy::removeAllUserMessageHandlers):
+        (WebKit::WebUserContentControllerProxy::addContentRuleList):
+        (WebKit::WebUserContentControllerProxy::removeContentRuleList):
+        (WebKit::WebUserContentControllerProxy::removeAllContentRuleLists):
+        * UIProcess/UserContent/WebUserContentControllerProxy.h:
+        (WebKit::WebUserContentControllerProxy::addNetworkProcess):
+        (WebKit::WebUserContentControllerProxy::removeNetworkProcess):
+
 2019-07-08  Daniel Bates  <dabates@apple.com>
 
         [iOS] Support select all in non-editable element
index 834db7a..107bb22 100644 (file)
@@ -74,19 +74,21 @@ WebUserContentControllerProxy::WebUserContentControllerProxy()
 WebUserContentControllerProxy::~WebUserContentControllerProxy()
 {
     webUserContentControllerProxies().remove(m_identifier);
-    for (auto* process : m_processes) {
-        process->removeMessageReceiver(Messages::WebUserContentControllerProxy::messageReceiverName(), identifier());
-        process->didDestroyWebUserContentControllerProxy(*this);
+    for (auto& process : m_processes) {
+        process.removeMessageReceiver(Messages::WebUserContentControllerProxy::messageReceiverName(), identifier());
+        process.didDestroyWebUserContentControllerProxy(*this);
     }
 #if ENABLE(CONTENT_EXTENSIONS)
-    for (auto* process : m_networkProcesses)
-        process->didDestroyWebUserContentControllerProxy(*this);
+    for (auto& process : m_networkProcesses)
+        process.didDestroyWebUserContentControllerProxy(*this);
 #endif
 }
 
 void WebUserContentControllerProxy::addProcess(WebProcessProxy& webProcessProxy, WebPageCreationParameters& parameters)
 {
-    if (m_processes.add(&webProcessProxy).isNewEntry)
+    ASSERT(!m_processes.hasNullReferences());
+
+    if (m_processes.add(webProcessProxy).isNewEntry)
         webProcessProxy.addMessageReceiver(Messages::WebUserContentControllerProxy::messageReceiverName(), identifier(), *this);
 
     ASSERT(parameters.userContentWorlds.isEmpty());
@@ -114,9 +116,10 @@ void WebUserContentControllerProxy::addProcess(WebProcessProxy& webProcessProxy,
 
 void WebUserContentControllerProxy::removeProcess(WebProcessProxy& webProcessProxy)
 {
-    ASSERT(m_processes.contains(&webProcessProxy));
+    ASSERT(m_processes.contains(webProcessProxy));
+    ASSERT(!m_processes.hasNullReferences());
 
-    m_processes.remove(&webProcessProxy);
+    m_processes.remove(webProcessProxy);
     webProcessProxy.removeMessageReceiver(Messages::WebUserContentControllerProxy::messageReceiverName(), identifier());
 }
 
@@ -127,8 +130,8 @@ void WebUserContentControllerProxy::addUserContentWorldUse(API::UserContentWorld
 
     auto addResult = m_userContentWorlds.add(&world);
     if (addResult.isNewEntry) {
-        for (WebProcessProxy* process : m_processes)
-            process->send(Messages::WebUserContentController::AddUserContentWorlds({ std::make_pair(world.identifier(), world.name()) }), identifier());
+        for (auto& process : m_processes)
+            process.send(Messages::WebUserContentController::AddUserContentWorlds({ std::make_pair(world.identifier(), world.name()) }), identifier());
     }
 }
 
@@ -151,8 +154,8 @@ bool WebUserContentControllerProxy::shouldSendRemoveUserContentWorldsMessage(API
 void WebUserContentControllerProxy::removeUserContentWorldUses(API::UserContentWorld& world, unsigned numberOfUsesToRemove)
 {
     if (shouldSendRemoveUserContentWorldsMessage(world, numberOfUsesToRemove)) {
-        for (WebProcessProxy* process : m_processes)
-            process->send(Messages::WebUserContentController::RemoveUserContentWorlds({ world.identifier() }), identifier());
+        for (auto& process : m_processes)
+            process.send(Messages::WebUserContentController::RemoveUserContentWorlds({ world.identifier() }), identifier());
     }
 }
 
@@ -164,8 +167,8 @@ void WebUserContentControllerProxy::removeUserContentWorldUses(HashCountedSet<Re
             worldsToRemove.append(worldUsePair.key->identifier());
     }
 
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::RemoveUserContentWorlds(worldsToRemove), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::RemoveUserContentWorlds(worldsToRemove), identifier());
 }
 
 void WebUserContentControllerProxy::addUserScript(API::UserScript& userScript, InjectUserScriptImmediately immediately)
@@ -176,16 +179,16 @@ void WebUserContentControllerProxy::addUserScript(API::UserScript& userScript, I
 
     m_userScripts->elements().append(&userScript);
 
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::AddUserScripts({ { userScript.identifier(), world->identifier(), userScript.userScript() } }, immediately), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::AddUserScripts({ { userScript.identifier(), world->identifier(), userScript.userScript() } }, immediately), identifier());
 }
 
 void WebUserContentControllerProxy::removeUserScript(API::UserScript& userScript)
 {
     Ref<API::UserContentWorld> world = userScript.userContentWorld();
 
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::RemoveUserScript(world->identifier(), userScript.identifier()), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::RemoveUserScript(world->identifier(), userScript.identifier()), identifier());
 
     m_userScripts->elements().removeAll(&userScript);
 
@@ -194,8 +197,8 @@ void WebUserContentControllerProxy::removeUserScript(API::UserScript& userScript
 
 void WebUserContentControllerProxy::removeAllUserScripts(API::UserContentWorld& world)
 {
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::RemoveAllUserScripts({ world.identifier() }), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::RemoveAllUserScripts({ world.identifier() }), identifier());
 
     unsigned userScriptsRemoved = m_userScripts->removeAllOfTypeMatching<API::UserScript>([&](const auto& userScript) {
         return &userScript->userContentWorld() == &world;
@@ -215,8 +218,8 @@ void WebUserContentControllerProxy::removeAllUserScripts()
     for (const auto& worldCountPair : worlds)
         worldIdentifiers.uncheckedAppend(worldCountPair.key->identifier());
 
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::RemoveAllUserScripts(worldIdentifiers), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::RemoveAllUserScripts(worldIdentifiers), identifier());
 
     m_userScripts->elements().clear();
 
@@ -231,16 +234,16 @@ void WebUserContentControllerProxy::addUserStyleSheet(API::UserStyleSheet& userS
 
     m_userStyleSheets->elements().append(&userStyleSheet);
 
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::AddUserStyleSheets({ { userStyleSheet.identifier(), world->identifier(), userStyleSheet.userStyleSheet() } }), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::AddUserStyleSheets({ { userStyleSheet.identifier(), world->identifier(), userStyleSheet.userStyleSheet() } }), identifier());
 }
 
 void WebUserContentControllerProxy::removeUserStyleSheet(API::UserStyleSheet& userStyleSheet)
 {
     Ref<API::UserContentWorld> world = userStyleSheet.userContentWorld();
 
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::RemoveUserStyleSheet(world->identifier(), userStyleSheet.identifier()), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::RemoveUserStyleSheet(world->identifier(), userStyleSheet.identifier()), identifier());
 
     m_userStyleSheets->elements().removeAll(&userStyleSheet);
 
@@ -249,8 +252,8 @@ void WebUserContentControllerProxy::removeUserStyleSheet(API::UserStyleSheet& us
 
 void WebUserContentControllerProxy::removeAllUserStyleSheets(API::UserContentWorld& world)
 {
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::RemoveAllUserStyleSheets({ world.identifier() }), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::RemoveAllUserStyleSheets({ world.identifier() }), identifier());
 
     unsigned userStyleSheetsRemoved = m_userStyleSheets->removeAllOfTypeMatching<API::UserStyleSheet>([&](const auto& userStyleSheet) {
         return &userStyleSheet->userContentWorld() == &world;
@@ -270,8 +273,8 @@ void WebUserContentControllerProxy::removeAllUserStyleSheets()
     for (const auto& worldCountPair : worlds)
         worldIdentifiers.uncheckedAppend(worldCountPair.key->identifier());
 
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::RemoveAllUserStyleSheets(worldIdentifiers), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::RemoveAllUserStyleSheets(worldIdentifiers), identifier());
 
     m_userStyleSheets->elements().clear();
 
@@ -291,8 +294,8 @@ bool WebUserContentControllerProxy::addUserScriptMessageHandler(WebScriptMessage
 
     m_scriptMessageHandlers.add(handler.identifier(), &handler);
 
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::AddUserScriptMessageHandlers({ { handler.identifier(), world->identifier(), handler.name() } }), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::AddUserScriptMessageHandlers({ { handler.identifier(), world->identifier(), handler.name() } }), identifier());
     
     return true;
 }
@@ -301,8 +304,8 @@ void WebUserContentControllerProxy::removeUserMessageHandlerForName(const String
 {
     for (auto it = m_scriptMessageHandlers.begin(), end = m_scriptMessageHandlers.end(); it != end; ++it) {
         if (it->value->name() == name && &it->value->userContentWorld() == &world) {
-            for (WebProcessProxy* process : m_processes)
-                process->send(Messages::WebUserContentController::RemoveUserScriptMessageHandler(world.identifier(), it->value->identifier()), identifier());
+            for (auto& process : m_processes)
+                process.send(Messages::WebUserContentController::RemoveUserScriptMessageHandler(world.identifier(), it->value->identifier()), identifier());
 
             m_scriptMessageHandlers.remove(it);
 
@@ -314,8 +317,8 @@ void WebUserContentControllerProxy::removeUserMessageHandlerForName(const String
 
 void WebUserContentControllerProxy::removeAllUserMessageHandlers(API::UserContentWorld& world)
 {
-    for (WebProcessProxy* process : m_processes)
-        process->send(Messages::WebUserContentController::RemoveAllUserScriptMessageHandlers({ world.identifier() }), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::RemoveAllUserScriptMessageHandlers({ world.identifier() }), identifier());
 
     unsigned numberRemoved = 0;
     m_scriptMessageHandlers.removeIf([&](auto& entry) {
@@ -352,33 +355,33 @@ void WebUserContentControllerProxy::addContentRuleList(API::ContentRuleList& con
 
     auto pair = std::make_pair(contentRuleList.name(), contentRuleList.compiledRuleList().data());
 
-    for (auto* process : m_processes)
-        process->send(Messages::WebUserContentController::AddContentRuleLists({ pair }), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::AddContentRuleLists({ pair }), identifier());
 
-    for (auto* process : m_networkProcesses)
-        process->send(Messages::NetworkContentRuleListManager::AddContentRuleLists { identifier(), { pair } }, 0);
+    for (auto& process : m_networkProcesses)
+        process.send(Messages::NetworkContentRuleListManager::AddContentRuleLists { identifier(), { pair } }, 0);
 }
 
 void WebUserContentControllerProxy::removeContentRuleList(const String& name)
 {
     m_contentRuleLists.remove(name);
 
-    for (auto* process : m_processes)
-        process->send(Messages::WebUserContentController::RemoveContentRuleList(name), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::RemoveContentRuleList(name), identifier());
 
-    for (auto* process : m_networkProcesses)
-        process->send(Messages::NetworkContentRuleListManager::RemoveContentRuleList { identifier(), name }, 0);
+    for (auto& process : m_networkProcesses)
+        process.send(Messages::NetworkContentRuleListManager::RemoveContentRuleList { identifier(), name }, 0);
 }
 
 void WebUserContentControllerProxy::removeAllContentRuleLists()
 {
     m_contentRuleLists.clear();
 
-    for (auto* process : m_processes)
-        process->send(Messages::WebUserContentController::RemoveAllContentRuleLists(), identifier());
+    for (auto& process : m_processes)
+        process.send(Messages::WebUserContentController::RemoveAllContentRuleLists(), identifier());
 
-    for (auto* process : m_networkProcesses)
-        process->send(Messages::NetworkContentRuleListManager::RemoveAllContentRuleLists { identifier() }, 0);
+    for (auto& process : m_networkProcesses)
+        process.send(Messages::NetworkContentRuleListManager::RemoveAllContentRuleLists { identifier() }, 0);
 }
 #endif
 
index 0ed36a5..e6444b2 100644 (file)
 
 #include "APIObject.h"
 #include "MessageReceiver.h"
+#include "NetworkProcessProxy.h"
 #include "UserContentControllerIdentifier.h"
 #include <WebCore/PageIdentifier.h>
 #include <wtf/Forward.h>
 #include <wtf/HashCountedSet.h>
 #include <wtf/HashMap.h>
-#include <wtf/HashSet.h>
 #include <wtf/Ref.h>
 #include <wtf/RefCounted.h>
+#include <wtf/WeakHashSet.h>
 #include <wtf/text/StringHash.h>
 
 namespace API {
@@ -96,8 +97,8 @@ public:
     void removeAllUserMessageHandlers(API::UserContentWorld&);
 
 #if ENABLE(CONTENT_EXTENSIONS)
-    void addNetworkProcess(NetworkProcessProxy& proxy) { m_networkProcesses.add(&proxy); }
-    void removeNetworkProcess(NetworkProcessProxy& proxy) { m_networkProcesses.remove(&proxy); }
+    void addNetworkProcess(NetworkProcessProxy& proxy) { m_networkProcesses.add(proxy); }
+    void removeNetworkProcess(NetworkProcessProxy& proxy) { m_networkProcesses.remove(proxy); }
 
     void addContentRuleList(API::ContentRuleList&);
     void removeContentRuleList(const String&);
@@ -119,14 +120,14 @@ private:
     bool shouldSendRemoveUserContentWorldsMessage(API::UserContentWorld&, unsigned numberOfUsesToRemove);
 
     UserContentControllerIdentifier m_identifier;
-    HashSet<WebProcessProxy*> m_processes;
+    WeakHashSet<WebProcessProxy> m_processes;
     Ref<API::Array> m_userScripts;
     Ref<API::Array> m_userStyleSheets;
     HashMap<uint64_t, RefPtr<WebScriptMessageHandler>> m_scriptMessageHandlers;
     HashCountedSet<RefPtr<API::UserContentWorld>> m_userContentWorlds;
 
 #if ENABLE(CONTENT_EXTENSIONS)
-    HashSet<NetworkProcessProxy*> m_networkProcesses;
+    WeakHashSet<NetworkProcessProxy> m_networkProcesses;
     HashMap<String, RefPtr<API::ContentRuleList>> m_contentRuleLists;
 #endif
 };