Bug 544074 - Crashes at mozilla::plugins::PluginModuleChild::UnregisterActorForNPObject - the hash of scriptable objects is mutating while we enumerate the actors being destroyed after a plugin instance is destroyed. Save the list of actors off into a separate non-mutating table, r=bent

dom/plugins/PluginInstanceChild.cpp

@@ -1457,17 +1457,60 @@ PluginInstanceChild::UnscheduleTimer(uint32_t id)
     mTimers.RemoveElement(id, ChildTimer::IDComparator());
 }
 
+static PLDHashOperator
+InvalidateObject(DeletingObjectEntry* e, void* userArg)
+{
+    NPObject* o = e->GetKey();
+    if (!e->mDeleted && o->_class && o->_class->invalidate)
+        o->_class->invalidate(o);
+
+    return PL_DHASH_NEXT;
+}
+
+static PLDHashOperator
+DeleteObject(DeletingObjectEntry* e, void* userArg)
+{
+    NPObject* o = e->GetKey();
+    if (!e->mDeleted) {
+        e->mDeleted = true;
+
+#ifdef NS_BUILD_REFCNT_LOGGING
+        {
+            int32_t refcnt = o->referenceCount;
+            while (refcnt) {
+                --refcnt;
+                NS_LOG_RELEASE(o, refcnt, "NPObject");
+            }
+        }
+#endif
+
+        PluginModuleChild::DeallocNPObject(o);
+    }
+
+    return PL_DHASH_NEXT;
+}
+
 bool
 PluginInstanceChild::AnswerNPP_Destroy(NPError* aResult)
 {
+    PLUGIN_LOG_DEBUG_METHOD;
+    AssertPluginThread();
+
     for (PRUint32 i = 0; i < mPendingAsyncCalls.Length(); ++i)
         mPendingAsyncCalls[i]->Cancel();
     mPendingAsyncCalls.TruncateLength(0);
 
     mTimers.Clear();
 
-    PluginModuleChild* module = PluginModuleChild::current();
-    bool retval = module->PluginInstanceDestroyed(this, aResult);
+    PluginModuleChild::current()->NPP_Destroy(this);
+    mData.ndata = 0;
+
+    mDeletingHash = new nsTHashtable<DeletingObjectEntry>;
+    mDeletingHash->Init();
+    PluginModuleChild::current()->FindNPObjectsForInstance(this);
+
+    mDeletingHash->EnumerateEntries(InvalidateObject, NULL);
+    mDeletingHash->EnumerateEntries(DeleteObject, NULL);
 
     // Null out our cached actors as they should have been killed in the
     // PluginInstanceDestroyed call above.
@@ -1480,5 +1523,5 @@ PluginInstanceChild::AnswerNPP_Destroy(NPError* aResult)
     ResetPumpHooks();
 #endif
 
-    return retval;
+    return true;
 }

dom/plugins/PluginInstanceChild.h

@@ -52,6 +52,7 @@
 #include "ChildAsyncCall.h"
 #include "ChildTimer.h"
 #include "nsRect.h"
+#include "nsTHashtable.h"
 
 namespace mozilla {
 namespace plugins {
@@ -181,6 +182,8 @@ public:
     void UnscheduleTimer(uint32_t id);
 
 private:
+    friend class PluginModuleChild;
+
     // Quirks mode support for various plugin mime types
     enum PluginQuirks {
         QUIRK_SILVERLIGHT_WINLESS_INPUT_TRANSLATION = 1, // Win32
@@ -253,6 +256,13 @@ private:
     nsTArray<ChildAsyncCall*> mPendingAsyncCalls;
     nsTArray<nsAutoPtr<ChildTimer> > mTimers;
 
+    /**
+     * During destruction we enumerate all remaining scriptable objects and
+     * invalidate/delete them. Enumeration can re-enter, so maintain a
+     * hash separate from PluginModuleChild.mObjectMap.
+     */
+    nsAutoPtr< nsTHashtable<DeletingObjectEntry> > mDeletingHash;
+
 #if defined(OS_WIN)
 private:
     // Shared dib rendering management for windowless plugins.

dom/plugins/PluginMessageUtils.h

@@ -50,6 +50,7 @@
 #include "nsTArray.h"
 #include "nsThreadUtils.h"
 #include "prlog.h"
+#include "nsHashKeys.h"
 
 namespace mozilla {
 
@@ -227,6 +228,16 @@ NullableStringGet(const nsCString& str)
   return str.get();
 }
 
+struct DeletingObjectEntry : public nsPtrHashKey<NPObject>
+{
+  DeletingObjectEntry(const NPObject* key)
+    : nsPtrHashKey<NPObject>(key)
+    , mDeleted(false)
+  { }
+
+  bool mDeleted;
+};
+
 } /* namespace plugins */
 
 } /* namespace mozilla */

dom/plugins/PluginModuleChild.cpp

@@ -1514,23 +1514,6 @@ PluginModuleChild::DeallocPPluginInstance(PPluginInstanceChild* aActor)
     return true;
 }
 
-bool
-PluginModuleChild::PluginInstanceDestroyed(PluginInstanceChild* aActor,
-                                           NPError* rv)
-{
-    PLUGIN_LOG_DEBUG_METHOD;
-    AssertPluginThread();
-
-    NPP npp = aActor->GetNPP();
-
-    *rv = mFunctions.destroy(npp, 0);
-    npp->ndata = 0;
-
-    DeallocNPObjectsForInstance(aActor);
-
-    return true;
-}
-
 NPObject* NP_CALLBACK
 PluginModuleChild::NPN_CreateObject(NPP aNPP, NPClass* aClass)
 {
@@ -1538,6 +1521,10 @@ PluginModuleChild::NPN_CreateObject(NPP aNPP, NPClass* aClass)
     AssertPluginThread();
 
     PluginInstanceChild* i = InstCast(aNPP);
+    if (i->mDeletingHash) {
+        NS_ERROR("Plugin used NPP after NPP_Destroy");
+        return NULL;
+    }
 
     NPObject* newObject;
     if (aClass && aClass->allocate) {
@@ -1577,17 +1564,30 @@ PluginModuleChild::NPN_ReleaseObject(NPObject* aNPObj)
 {
     AssertPluginThread();
 
+    NPObjectData* d = current()->mObjectMap.GetEntry(aNPObj);
+    if (!d) {
+        NS_ERROR("Releasing object not in mObjectMap?");
+        return;
+    }
+
+    DeletingObjectEntry* doe = NULL;
+    if (d->instance->mDeletingHash) {
+        doe = d->instance->mDeletingHash->GetEntry(aNPObj);
+        if (!doe) {
+            NS_ERROR("An object for a destroyed instance isn't in the instance deletion hash");
+            return;
+        }
+        if (doe->mDeleted)
+            return;
+    }
+
     int32_t refCnt = PR_AtomicDecrement((PRInt32*)&aNPObj->referenceCount);
     NS_LOG_RELEASE(aNPObj, refCnt, "NPObject");
 
     if (refCnt == 0) {
         DeallocNPObject(aNPObj);
-#ifdef DEBUG
-        NPObjectData* d = current()->mObjectMap.GetEntry(aNPObj);
-        NS_ASSERTION(d, "NPObject not mapped?");
-        NS_ASSERTION(!d->actor, "NPObject has actor at destruction?");
-#endif
-        current()->mObjectMap.RemoveEntry(aNPObj);
+        if (doe)
+            doe->mDeleted = true;
     }
     return;
 }
@@ -1600,39 +1600,28 @@ PluginModuleChild::DeallocNPObject(NPObject* aNPObj)
     } else {
         child::_memfree(aNPObj);
     }
-}
 
-PLDHashOperator
-PluginModuleChild::DeallocForInstance(NPObjectData* d, void* userArg)
-{
-    if (d->instance == static_cast<PluginInstanceChild*>(userArg)) {
-        NPObject* o = d->GetKey();
-        if (o->_class && o->_class->invalidate)
-            o->_class->invalidate(o);
+    NPObjectData* d = current()->mObjectMap.GetEntry(aNPObj);
+    if (d->actor)
+        d->actor->NPObjectDestroyed();
 
-#ifdef NS_BUILD_REFCNT_LOGGING
-        {
-            int32_t refCnt = o->referenceCount;
-            while (refCnt) {
-                --refCnt;
-                NS_LOG_RELEASE(o, refCnt, "NPObject");
-            }
-        }
-#endif
-
-        DeallocNPObject(o);
-
-        if (d->actor)
-            d->actor->NPObjectDestroyed();
-
-        return PL_DHASH_REMOVE;
-    }
-
-    return PL_DHASH_NEXT;
+    current()->mObjectMap.RemoveEntry(aNPObj);
 }
 
 void
-PluginModuleChild::DeallocNPObjectsForInstance(PluginInstanceChild* instance)
+PluginModuleChild::FindNPObjectsForInstance(PluginInstanceChild* instance)
 {
-    mObjectMap.EnumerateEntries(DeallocForInstance, instance);
+    NS_ASSERTION(instance->mDeletingHash, "filling null mDeletingHash?");
+    mObjectMap.EnumerateEntries(CollectForInstance, instance);
+}
+
+PLDHashOperator
+PluginModuleChild::CollectForInstance(NPObjectData* d, void* userArg)
+{
+    PluginInstanceChild* instance = static_cast<PluginInstanceChild*>(userArg);
+    if (d->instance == instance) {
+        NPObject* o = d->GetKey();
+        instance->mDeletingHash->PutEntry(o);
+    }
+    return PL_DHASH_NEXT;
 }

dom/plugins/PluginModuleChild.h

@@ -147,10 +147,6 @@ public:
     bool NPObjectIsRegistered(NPObject* aObject);
 #endif
 
-    bool
-    PluginInstanceDestroyed(PluginInstanceChild* aActor,
-                            NPError* rv);
-
     /**
      * The child implementation of NPN_CreateObject.
      */
@@ -202,22 +198,25 @@ private:
      */
     nsTHashtable<NPObjectData> mObjectMap;
 
+public: // called by PluginInstanceChild
     /**
      * Dealloc an NPObject after last-release or when the associated instance
-     * is destroyed. It is the callers responsibility to remove the object
-     * from mObjectMap.
+     * is destroyed. This function will remove the object from mObjectMap.
      */
     static void DeallocNPObject(NPObject* o);
 
+    NPError NPP_Destroy(PluginInstanceChild* instance) {
+        return mFunctions.destroy(instance->GetNPP(), 0);
+    }
+
     /**
-     * After an instance has been destroyed, dealloc the objects associated
-     * with that instance.
+     * Fill PluginInstanceChild.mDeletingHash with all the remaining NPObjects
+     * associated with that instance.
      */
-    void DeallocNPObjectsForInstance(PluginInstanceChild* instance);
-    /**
-     * Enumeration helper function for DeallocNPObjectsForInstance.
-     */
-    static PLDHashOperator DeallocForInstance(NPObjectData* d, void* userArg);
+    void FindNPObjectsForInstance(PluginInstanceChild* instance);
+
+private:
+    static PLDHashOperator CollectForInstance(NPObjectData* d, void* userArg);
 };
 
 } /* namespace plugins */