2011-03-17 Anders Carlsson <andersca@apple.com>

Reviewed by Darin Adler. Invalidate all NPObjects for a plug-in when that plug-in is destroyed https://bugs.webkit.org/show_bug.cgi?id=56511 <rdar://problem/8993491> Before this change, we would invalidate NPObjectProxy objects and delete NPObjectMessageReceiver objects when the last plug-in of a certain type was destroyed. Doing so caused us to hold on to memory which we don't need, and could also lead to crashes if the NPObjectMessageReceiver would get a message and tried to invoke it on a already deallocated NPObject. * PluginProcess/PluginControllerProxy.cpp: (WebKit::PluginControllerProxy::initialize): If we fail to initialize, call removePluginControllerProxy instead of having WebProcessConnection do so. (WebKit::PluginControllerProxy::destroy): Pass the plug-in to removePluginControllerProxy. * PluginProcess/WebProcessConnection.cpp: (WebKit::WebProcessConnection::removePluginControllerProxy): Call NPRemoteObjectMap::pluginDestroyed when the plug-in has been destroyed. (WebKit::WebProcessConnection::createPlugin): Don't call removePluginControllerProxy if the plug-in fails to initialize. PluginControllerProxy::initialize now takes care of doing this. * Shared/Plugins/NPObjectMessageReceiver.cpp: (WebKit::NPObjectMessageReceiver::NPObjectMessageReceiver): (WebKit::NPObjectMessageReceiver::~NPObjectMessageReceiver): * Shared/Plugins/NPObjectMessageReceiver.h: Remove m_shouldReleaseObjectWhenInvalidating, we now know that no NPObjects will have been deallocated by the time the NPObjectMessageReceiver is destroyed. (WebKit::NPObjectMessageReceiver::plugin): Add getter. * Shared/Plugins/NPObjectProxy.h: (WebKit::NPObjectProxy::plugin): Add getter. * Shared/Plugins/NPRemoteObjectMap.cpp: (WebKit::NPRemoteObjectMap::NPRemoteObjectMap): Remove m_isInvalidating. (WebKit::NPRemoteObjectMap::npObjectProxyDestroyed): Simplify code. (WebKit::NPRemoteObjectMap::pluginDestroyed): Rename invalidate to pluginDestroyed. Only invalidate/delete objects that belong to the given plug-in. * Shared/Plugins/NPRemoteObjectMap.h: Remove m_isInvalidating. * WebProcess/Plugins/PluginProcessConnection.cpp: (WebKit::PluginProcessConnection::removePluginProxy): Call NPRemoteObjectMap::pluginDestroyed when the plug-in has been destroyed. Canonical link: https://commits.webkit.org/71244@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@81370 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Mar 17, 2011 · d456eb6 · d456eb6
1 parent 719154f
commit d456eb6
Show file tree

Hide file tree

Showing 10 changed files with 116 additions and 47 deletions.
diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
@@ -1,3 +1,62 @@
+2011-03-17  Anders Carlsson  <andersca@apple.com>
+
+        Reviewed by Darin Adler.
+
+        Invalidate all NPObjects for a plug-in when that plug-in is destroyed
+        https://bugs.webkit.org/show_bug.cgi?id=56511
+        <rdar://problem/8993491>
+
+        Before this change, we would invalidate NPObjectProxy objects and delete NPObjectMessageReceiver
+        objects when the last plug-in of a certain type was destroyed. Doing so caused us to hold on to memory
+        which we don't need, and could also lead to crashes if the NPObjectMessageReceiver would get a message and
+        tried to invoke it on a already deallocated NPObject.
+
+        * PluginProcess/PluginControllerProxy.cpp:
+        (WebKit::PluginControllerProxy::initialize):
+        If we fail to initialize, call removePluginControllerProxy instead of having WebProcessConnection do so.
+
+        (WebKit::PluginControllerProxy::destroy):
+        Pass the plug-in to removePluginControllerProxy.
+
+        * PluginProcess/WebProcessConnection.cpp:
+        (WebKit::WebProcessConnection::removePluginControllerProxy):
+        Call NPRemoteObjectMap::pluginDestroyed when the plug-in has been destroyed.
+
+        (WebKit::WebProcessConnection::createPlugin):
+        Don't call removePluginControllerProxy if the plug-in fails to initialize. PluginControllerProxy::initialize now
+        takes care of doing this.
+
+        * Shared/Plugins/NPObjectMessageReceiver.cpp:
+        (WebKit::NPObjectMessageReceiver::NPObjectMessageReceiver):
+        (WebKit::NPObjectMessageReceiver::~NPObjectMessageReceiver):
+        * Shared/Plugins/NPObjectMessageReceiver.h:
+        Remove m_shouldReleaseObjectWhenInvalidating, we now know that no NPObjects will have been deallocated
+        by the time the NPObjectMessageReceiver is destroyed.
+
+        (WebKit::NPObjectMessageReceiver::plugin):
+        Add getter.
+
+        * Shared/Plugins/NPObjectProxy.h:
+        (WebKit::NPObjectProxy::plugin):
+        Add getter.
+
+        * Shared/Plugins/NPRemoteObjectMap.cpp:
+        (WebKit::NPRemoteObjectMap::NPRemoteObjectMap):
+        Remove m_isInvalidating.
+
+        (WebKit::NPRemoteObjectMap::npObjectProxyDestroyed):
+        Simplify code.
+
+        (WebKit::NPRemoteObjectMap::pluginDestroyed):
+        Rename invalidate to pluginDestroyed. Only invalidate/delete objects that belong to the given plug-in.
+
+        * Shared/Plugins/NPRemoteObjectMap.h:
+        Remove m_isInvalidating.
+
+        * WebProcess/Plugins/PluginProcessConnection.cpp:
+        (WebKit::PluginProcessConnection::removePluginProxy):
+        Call NPRemoteObjectMap::pluginDestroyed when the plug-in has been destroyed.
+
 2011-03-17  Oleg Romashin  <oleg.romashin@nokia.com>
 
         Reviewed by Anders Carlsson.

diff --git a/Source/WebKit2/PluginProcess/PluginControllerProxy.cpp b/Source/WebKit2/PluginProcess/PluginControllerProxy.cpp
@@ -82,7 +82,14 @@ bool PluginControllerProxy::initialize(const Plugin::Parameters& parameters)
         return false;
 
     if (!m_plugin->initialize(this, parameters)) {
+        // Get the plug-in so we can pass it to removePluginControllerProxy. The pointer is only
+        // used as an identifier so it's OK to just get a weak reference.
+        Plugin* plugin = m_plugin.get();
+
         m_plugin = 0;
+
+        // This will delete the plug-in controller proxy object.
+        m_connection->removePluginControllerProxy(this, plugin);
         return false;
     }
 
@@ -102,13 +109,17 @@ void PluginControllerProxy::destroy()
         return;
     }
 
+    // Get the plug-in so we can pass it to removePluginControllerProxy. The pointer is only
+    // used as an identifier so it's OK to just get a weak reference.
+    Plugin* plugin = m_plugin.get();
+
     m_plugin->destroy();
     m_plugin = 0;
 
     platformDestroy();
 
     // This will delete the plug-in controller proxy object.
-    m_connection->removePluginControllerProxy(this);
+    m_connection->removePluginControllerProxy(this, plugin);
 }
 
 void PluginControllerProxy::paint()

diff --git a/Source/WebKit2/PluginProcess/WebProcessConnection.cpp b/Source/WebKit2/PluginProcess/WebProcessConnection.cpp
@@ -70,20 +70,23 @@ void WebProcessConnection::destroyPluginControllerProxy(PluginControllerProxy* p
     pluginController->destroy();
 }
 
-void WebProcessConnection::removePluginControllerProxy(PluginControllerProxy* pluginController)
+void WebProcessConnection::removePluginControllerProxy(PluginControllerProxy* pluginController, Plugin* plugin)
 {
+    ASSERT(plugin);
+
     {
         ASSERT(m_pluginControllers.contains(pluginController->pluginInstanceID()));
 
         OwnPtr<PluginControllerProxy> pluginControllerOwnPtr = adoptPtr(m_pluginControllers.take(pluginController->pluginInstanceID()));
         ASSERT(pluginControllerOwnPtr == pluginController);
     }
-
+
+    // Invalidate all objects related to this plug-in.
+    m_npRemoteObjectMap->pluginDestroyed(plugin);
+
     if (!m_pluginControllers.isEmpty())
         return;
 
-    // Invalidate our remote object map.
-    m_npRemoteObjectMap->invalidate();
     m_npRemoteObjectMap = nullptr;
 
     // The last plug-in went away, close this connection.
@@ -167,13 +170,10 @@ void WebProcessConnection::createPlugin(uint64_t pluginInstanceID, const Plugin:
     // Now try to initialize the plug-in.
     result = pluginControllerProxyPtr->initialize(parameters);
 
-    if (result) {
-        remoteLayerClientID = pluginControllerProxyPtr->remoteLayerClientID();
+    if (!result)
         return;
-    }
 
-    // We failed to initialize, remove the plug-in controller. This could cause us to be deleted.
-    removePluginControllerProxy(pluginControllerProxyPtr);
+    remoteLayerClientID = pluginControllerProxyPtr->remoteLayerClientID();
 }
 
 } // namespace WebKit

diff --git a/Source/WebKit2/PluginProcess/WebProcessConnection.h b/Source/WebKit2/PluginProcess/WebProcessConnection.h
@@ -47,7 +47,7 @@ class WebProcessConnection : public RefCounted<WebProcessConnection>, CoreIPC::C
     CoreIPC::Connection* connection() const { return m_connection.get(); }
     NPRemoteObjectMap* npRemoteObjectMap() const { return m_npRemoteObjectMap.get(); }
 
-    void removePluginControllerProxy(PluginControllerProxy*);
+    void removePluginControllerProxy(PluginControllerProxy*, Plugin*);
 
 private:
     WebProcessConnection(CoreIPC::Connection::Identifier);

diff --git a/Source/WebKit2/Shared/Plugins/NPObjectMessageReceiver.cpp b/Source/WebKit2/Shared/Plugins/NPObjectMessageReceiver.cpp
@@ -33,9 +33,6 @@
 #include "NPRuntimeUtilities.h"
 #include "NPVariantData.h"
 
-// FIXME: This code shouldn't know about NPJSObject.
-#include "NPJSObject.h"
-
 namespace WebKit {
 
 PassOwnPtr<NPObjectMessageReceiver> NPObjectMessageReceiver::create(NPRemoteObjectMap* npRemoteObjectMap, Plugin* plugin, uint64_t npObjectID, NPObject* npObject)
@@ -48,7 +45,6 @@ NPObjectMessageReceiver::NPObjectMessageReceiver(NPRemoteObjectMap* npRemoteObje
     , m_plugin(plugin)
     , m_npObjectID(npObjectID)
     , m_npObject(npObject)
-    , m_shouldReleaseObjectWhenInvalidating(!NPJSObject::isNPJSObject(npObject))
 {
     retainNPObject(m_npObject);
 }
@@ -57,13 +53,6 @@ NPObjectMessageReceiver::~NPObjectMessageReceiver()
 {
     m_npRemoteObjectMap->unregisterNPObject(m_npObjectID);
 
-    // If we're invalidating the remote object map, we don't always want to release the underlying NPObject.
-    // One example of this is NPJSObjects in the Web process, which have already been deallocated by the plug-in view.
-    // FIXME: This is not the ideal way to handle this. Maybe NPObjectMessageReceiver should be notified somehow when the underlying
-    // NPObject is deallocated.
-    if (m_npRemoteObjectMap->isInvalidating() && !m_shouldReleaseObjectWhenInvalidating)
-        return;
-
     releaseNPObject(m_npObject);
 }
 

diff --git a/Source/WebKit2/Shared/Plugins/NPObjectMessageReceiver.h b/Source/WebKit2/Shared/Plugins/NPObjectMessageReceiver.h
@@ -49,6 +49,7 @@ class NPObjectMessageReceiver {
 
     CoreIPC::SyncReplyMode didReceiveSyncNPObjectMessageReceiverMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::ArgumentDecoder*, CoreIPC::ArgumentEncoder*);
 
+    Plugin* plugin() const { return m_plugin; }
     NPObject* npObject() const { return m_npObject; }
 
 private:
@@ -70,7 +71,6 @@ class NPObjectMessageReceiver {
     Plugin* m_plugin;
     uint64_t m_npObjectID;
     NPObject* m_npObject;
-    bool m_shouldReleaseObjectWhenInvalidating;
 };
 
 } // namespace WebKit

diff --git a/Source/WebKit2/Shared/Plugins/NPObjectProxy.h b/Source/WebKit2/Shared/Plugins/NPObjectProxy.h
@@ -50,6 +50,7 @@ class NPObjectProxy : public NPObject {
         return static_cast<NPObjectProxy*>(npObject);
     }
 
+    Plugin* plugin() const { return m_plugin; }
     uint64_t npObjectID() const { return m_npObjectID; }
 
     void invalidate();

diff --git a/Source/WebKit2/Shared/Plugins/NPRemoteObjectMap.cpp b/Source/WebKit2/Shared/Plugins/NPRemoteObjectMap.cpp
@@ -50,7 +50,6 @@ PassRefPtr<NPRemoteObjectMap> NPRemoteObjectMap::create(CoreIPC::Connection* con
 
 NPRemoteObjectMap::NPRemoteObjectMap(CoreIPC::Connection* connection)
     : m_connection(connection)
-    , m_isInvalidating(false)
 {
 }
 
@@ -71,10 +70,10 @@ NPObject* NPRemoteObjectMap::createNPObjectProxy(uint64_t remoteObjectID, Plugin
 
 void NPRemoteObjectMap::npObjectProxyDestroyed(NPObject* npObject)
 {
-    ASSERT(NPObjectProxy::isNPObjectProxy(npObject));
-    ASSERT(m_npObjectProxies.contains(npObject));
+    NPObjectProxy* npObjectProxy = NPObjectProxy::toNPObjectProxy(npObject);
+    ASSERT(m_npObjectProxies.contains(npObjectProxy));
 
-    m_npObjectProxies.remove(npObject);
+    m_npObjectProxies.remove(npObjectProxy);
 }
 
 uint64_t NPRemoteObjectMap::registerNPObject(NPObject* npObject, Plugin* plugin)
@@ -187,25 +186,37 @@ NPVariant NPRemoteObjectMap::npVariantDataToNPVariant(const NPVariantData& npVar
     return npVariant;
 }
 
-void NPRemoteObjectMap::invalidate()
+void NPRemoteObjectMap::pluginDestroyed(Plugin* plugin)
 {
-    ASSERT(!m_isInvalidating);
-
-    m_isInvalidating = true;
-
     Vector<NPObjectMessageReceiver*> messageReceivers;
-    copyValuesToVector(m_registeredNPObjects, messageReceivers);
+
+    // Gather the receivers associated with this plug-in.
+    for (HashMap<uint64_t, NPObjectMessageReceiver*>::const_iterator it = m_registeredNPObjects.begin(), end = m_registeredNPObjects.end(); it != end; ++it) {
+        NPObjectMessageReceiver* npObjectMessageReceiver = it->second;
+        if (npObjectMessageReceiver->plugin() == plugin)
+            messageReceivers.append(npObjectMessageReceiver);
+    }
 
     // Now delete all the receivers.
     deleteAllValues(messageReceivers);
 
-    ASSERT(m_registeredNPObjects.isEmpty());
+    Vector<NPObjectProxy*> objectProxies;
+    for (HashSet<NPObjectProxy*>::const_iterator it = m_npObjectProxies.begin(), end = m_npObjectProxies.end(); it != end; ++it) {
+        NPObjectProxy* npObjectProxy = *it;
 
-    for (HashSet<NPObject*>::const_iterator it = m_npObjectProxies.begin(), end = m_npObjectProxies.end(); it != end; ++it)
-        NPObjectProxy::toNPObjectProxy(*it)->invalidate();
-    m_npObjectProxies.clear();
+        if (npObjectProxy->plugin() == plugin)
+            objectProxies.append(npObjectProxy);
+    }
+
+    // Invalidate and remove all proxies associated with this plug-in.
+    for (size_t i = 0; i < objectProxies.size(); ++i) {
+        NPObjectProxy* npObjectProxy = objectProxies[i];
 
-    m_isInvalidating = false;
+        npObjectProxy->invalidate();
+
+        ASSERT(m_npObjectProxies.contains(npObjectProxy));
+        m_npObjectProxies.remove(npObjectProxy);
+    }
 }
 
 CoreIPC::SyncReplyMode NPRemoteObjectMap::didReceiveSyncMessage(CoreIPC::Connection* connection, CoreIPC::MessageID messageID, CoreIPC::ArgumentDecoder* arguments, CoreIPC::ArgumentEncoder* reply)

diff --git a/Source/WebKit2/Shared/Plugins/NPRemoteObjectMap.h b/Source/WebKit2/Shared/Plugins/NPRemoteObjectMap.h
@@ -61,24 +61,21 @@ class NPRemoteObjectMap : public RefCounted<NPRemoteObjectMap> {
     NPVariant npVariantDataToNPVariant(const NPVariantData&, Plugin*);
 
     CoreIPC::Connection* connection() const { return m_connection; }
-    bool isInvalidating() const { return m_isInvalidating; }
 
-    void invalidate();
+    void pluginDestroyed(Plugin*);
 
     CoreIPC::SyncReplyMode didReceiveSyncMessage(CoreIPC::Connection* connection, CoreIPC::MessageID messageID, CoreIPC::ArgumentDecoder* arguments, CoreIPC::ArgumentEncoder* reply);
 
 private:
     explicit NPRemoteObjectMap(CoreIPC::Connection*);
     CoreIPC::Connection* m_connection;
 
-    bool m_isInvalidating;
-
     // A map of NPObjectMessageReceiver classes, wrapping objects that we export to the
     // other end of the connection.
     HashMap<uint64_t, NPObjectMessageReceiver*> m_registeredNPObjects;
 
     // A set of NPObjectProxy objects associated with this map.
-    HashSet<NPObject*> m_npObjectProxies;
+    HashSet<NPObjectProxy*> m_npObjectProxies;
 };
 
 } // namespace WebKit

diff --git a/Source/WebKit2/WebProcess/Plugins/PluginProcessConnection.cpp b/Source/WebKit2/WebProcess/Plugins/PluginProcessConnection.cpp
@@ -62,13 +62,14 @@ void PluginProcessConnection::removePluginProxy(PluginProxy* plugin)
     ASSERT(m_plugins.contains(plugin->pluginInstanceID()));
     m_plugins.remove(plugin->pluginInstanceID());
 
+    // Invalidate all objects related to this plug-in.
+    m_npRemoteObjectMap->pluginDestroyed(plugin);
+
     if (!m_plugins.isEmpty())
         return;
 
-    // Invalidate our remote object map.
-    m_npRemoteObjectMap->invalidate();
-    m_npRemoteObjectMap = 0;
-
+    m_npRemoteObjectMap = nullptr;
+
     // We have no more plug-ins, invalidate the connection to the plug-in process.
     ASSERT(m_connection);
     m_connection->invalidate();