Bug 1013972 - Take account of reentrancy when removing entries from sJSObjWrappers. r=bsmedberg, a=lsblakk
authorJon Coppeard <jcoppeard@mozilla.com>
Tue, 27 May 2014 10:35:13 +0100
changeset 193438 69853748c1d7f3887e3f387c6b681e0375fedf5c
parent 193437 e2ed68483277c0225ee276bea77ec3d83c928069
child 193439 f2a2af61e05db88d9281debc71782a5fad5ca7f6
push id474
push userasasaki@mozilla.com
push dateMon, 02 Jun 2014 21:01:02 +0000
treeherdermozilla-release@967f4cf1b31c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg, lsblakk
bugs1013972
milestone30.0
Bug 1013972 - Take account of reentrancy when removing entries from sJSObjWrappers. r=bsmedberg, a=lsblakk
dom/plugins/base/nsJSNPRuntime.cpp
dom/plugins/base/nsJSNPRuntime.h
--- a/dom/plugins/base/nsJSNPRuntime.cpp
+++ b/dom/plugins/base/nsJSNPRuntime.cpp
@@ -60,16 +60,20 @@ struct JSObjWrapperHasher : public js::D
 // don't want to leak the world just because a plugin leaks an
 // NPObject).
 typedef js::HashMap<nsJSObjWrapperKey,
                     nsJSObjWrapper*,
                     JSObjWrapperHasher,
                     js::SystemAllocPolicy> JSObjWrapperTable;
 static JSObjWrapperTable sJSObjWrappers;
 
+// Whether it's safe to iterate sJSObjWrappers.  Set to true when sJSObjWrappers
+// has been initialized and is not currently being enumerated.
+static bool sJSObjWrappersAccessible = false;
+
 // Hash of NPObject wrappers that wrap NPObjects as JSObjects.
 static PLDHashTable sNPObjWrappers;
 
 // Global wrapper count. This includes JSObject wrappers *and*
 // NPObject wrappers. When this count goes to zero, there are no more
 // wrappers and we can kill off hash tables etc.
 static int32_t sWrapperCount;
 
@@ -252,22 +256,23 @@ OnWrapperCreated()
 }
 
 static void
 OnWrapperDestroyed()
 {
   NS_ASSERTION(sWrapperCount, "Whaaa, unbalanced created/destroyed calls!");
 
   if (--sWrapperCount == 0) {
-    if (sJSObjWrappers.initialized()) {
+    if (sJSObjWrappersAccessible) {
       MOZ_ASSERT(sJSObjWrappers.count() == 0);
 
       // No more wrappers, and our hash was initialized. Finish the
       // hash to prevent leaking it.
       sJSObjWrappers.finish();
+      sJSObjWrappersAccessible = false;
     }
 
     if (sNPObjWrappers.ops) {
       MOZ_ASSERT(sNPObjWrappers.entryCount == 0);
 
       // No more wrappers, and our hash was initialized. Finish the
       // hash to prevent leaking it.
       PL_DHashTableFinish(&sNPObjWrappers);
@@ -507,25 +512,16 @@ nsJSObjWrapper::~nsJSObjWrapper()
   MOZ_COUNT_DTOR(nsJSObjWrapper);
 
   // Invalidate first, since it relies on sJSRuntime and sJSObjWrappers.
   NP_Invalidate(this);
 
   OnWrapperDestroyed();
 }
 
-void
-nsJSObjWrapper::ClearJSObject() {
-  // Unroot the object's JSObject
-  JS_RemoveObjectRootRT(sJSRuntime, &mJSObj);
-
-  // Forget our reference to the JSObject.
-  mJSObj = nullptr;
-}
-
 // static
 NPObject *
 nsJSObjWrapper::NP_Allocate(NPP npp, NPClass *aClass)
 {
   NS_ASSERTION(aClass == &sJSObjWrapperNPClass,
                "Huh, wrong class passed to NP_Allocate()!!!");
 
   return new nsJSObjWrapper(npp);
@@ -542,22 +538,26 @@ nsJSObjWrapper::NP_Deallocate(NPObject *
 // static
 void
 nsJSObjWrapper::NP_Invalidate(NPObject *npobj)
 {
   nsJSObjWrapper *jsnpobj = (nsJSObjWrapper *)npobj;
 
   if (jsnpobj && jsnpobj->mJSObj) {
 
-    // Remove the wrapper from the hash
-    MOZ_ASSERT(sJSObjWrappers.initialized());
-    nsJSObjWrapperKey key(jsnpobj->mJSObj, jsnpobj->mNpp);
-    sJSObjWrappers.remove(key);
-
-    jsnpobj->ClearJSObject();
+    if (sJSObjWrappersAccessible) {
+      // Remove the wrapper from the hash
+      nsJSObjWrapperKey key(jsnpobj->mJSObj, jsnpobj->mNpp);
+      JSObjWrapperTable::Ptr ptr = sJSObjWrappers.lookup(key);
+      MOZ_ASSERT(ptr.found());
+      sJSObjWrappers.remove(ptr);
+    }
+
+    // Forget our reference to the JSObject.
+    jsnpobj->mJSObj = nullptr;
   }
 }
 
 static bool
 GetProperty(JSContext *cx, JSObject *objArg, NPIdentifier npid, JS::MutableHandle<JS::Value> rval)
 {
   NS_ASSERTION(NPIdentifierIsInt(npid) || NPIdentifierIsString(npid),
                "id must be either string or int!\n");
@@ -928,16 +928,17 @@ nsJSObjWrapper::NP_Construct(NPObject *n
  * table that has been moved.
  *
  * Note that the wrapper may be dead at this point, and even the table may have
  * been finalized if all wrappers have died.
  */
 static void
 JSObjWrapperKeyMarkCallback(JSTracer *trc, JSObject *obj, void *data) {
   NPP npp = static_cast<NPP>(data);
+  MOZ_ASSERT(sJSObjWrappersAccessible);
   if (!sJSObjWrappers.initialized())
     return;
 
   JSObject *prior = obj;
   nsJSObjWrapperKey oldKey(prior, npp);
   JSObjWrapperTable::Ptr p = sJSObjWrappers.lookup(oldKey);
   if (!p)
     return;
@@ -996,17 +997,19 @@ nsJSObjWrapper::GetNewOrUsed(NPP npp, JS
 
   if (!sJSObjWrappers.initialized()) {
     // No hash yet (or any more), initialize it.
     if (!sJSObjWrappers.init(16)) {
       NS_ERROR("Error initializing PLDHashTable!");
 
       return nullptr;
     }
+    sJSObjWrappersAccessible = true;
   }
+  MOZ_ASSERT(sJSObjWrappersAccessible);
 
   JSObjWrapperTable::Ptr p = sJSObjWrappers.lookupForAdd(nsJSObjWrapperKey(obj, npp));
   if (p) {
     MOZ_ASSERT(p->value());
     // Found a live nsJSObjWrapper, return it.
 
     return _retainobject(p->value());
   }
@@ -1853,26 +1856,36 @@ NPObjWrapperPluginDestroyedCallback(PLDH
 
   return PL_DHASH_NEXT;
 }
 
 // static
 void
 nsJSNPRuntime::OnPluginDestroy(NPP npp)
 {
-  if (sJSObjWrappers.initialized()) {
+  if (sJSObjWrappersAccessible) {
+
+    // Prevent modification of sJSObjWrappers table if we go reentrant.
+    sJSObjWrappersAccessible = false;
+
     for (JSObjWrapperTable::Enum e(sJSObjWrappers); !e.empty(); e.popFront()) {
       nsJSObjWrapper *npobj = e.front().value();
       MOZ_ASSERT(npobj->_class == &nsJSObjWrapper::sJSObjWrapperNPClass);
       if (npobj->mNpp == npp) {
-        npobj->ClearJSObject();
+        if (npobj->_class && npobj->_class->invalidate) {
+          npobj->_class->invalidate(npobj);
+        }
+
         _releaseobject(npobj);
+
         e.removeFront();
       }
     }
+
+    sJSObjWrappersAccessible = true;
   }
 
   // Use the safe JSContext here as we're not always able to find the
   // JSContext associated with the NPP any more.
   AutoSafeJSContext cx;
   if (sNPObjWrappers.ops) {
     NppAndCx nppcx = { npp, cx };
     PL_DHashTableEnumerate(&sNPObjWrappers,
--- a/dom/plugins/base/nsJSNPRuntime.h
+++ b/dom/plugins/base/nsJSNPRuntime.h
@@ -42,18 +42,16 @@ class nsJSObjWrapper : public NPObject
 {
 public:
   JSObject *mJSObj;  /* Added as a GC root. */
   const NPP mNpp;
 
   static NPObject *GetNewOrUsed(NPP npp, JSContext *cx,
                                 JS::Handle<JSObject*> obj);
 
-  void ClearJSObject();
-
 protected:
   nsJSObjWrapper(NPP npp);
   ~nsJSObjWrapper();
 
   static NPObject * NP_Allocate(NPP npp, NPClass *aClass);
   static void NP_Deallocate(NPObject *obj);
   static void NP_Invalidate(NPObject *obj);
   static bool NP_HasMethod(NPObject *, NPIdentifier identifier);