Bug 1294747 - Make sure we expose the expando of a [OverrideBuiltins] proxy to active JS when it gets cleared from the proxy. r=peterv, a=ritu
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 15 Sep 2016 15:04:56 -0400
changeset 350325 c339f78c47138782709cb0a415bab14328b1286c
parent 350324 40c40afc1ab74a9b207322e5aa9eb62dd19cadde
child 350326 4abcabc313ae8bd2d37560da4289b6fc1804ac17
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv, ritu
bugs1294747, 1296775, 1290359, 1293386, 1292855, 1289452, 1303340
milestone50.0
Bug 1294747 - Make sure we expose the expando of a [OverrideBuiltins] proxy to active JS when it gets cleared from the proxy. r=peterv, a=ritu This should also fix bug 1296775 and bug 1290359. There's a very good chance it will also fix bug 1293386, bug 1292855, bug 1289452, and bug 1303340: those would get hit if we happened to start _another_ gc after the expando died but while it was still in the Rooted. All of them seem to be dying under the domClass->mGetProto call, which could finish up a GC that kills the expando and then do _another_ one, causing the Rooted to try to mark a dead object.
dom/base/nsWrapperCache.cpp
dom/bindings/DOMJSProxyHandler.cpp
dom/bindings/DOMJSProxyHandler.h
--- a/dom/base/nsWrapperCache.cpp
+++ b/dom/base/nsWrapperCache.cpp
@@ -50,17 +50,17 @@ void
 nsWrapperCache::ReleaseWrapper(void* aScriptObjectHolder)
 {
   if (PreservingWrapper()) {
     // PreserveWrapper puts new DOM bindings in the JS holders hash, but they
     // can also be in the DOM expando hash, so we need to try to remove them
     // from both here.
     JSObject* obj = GetWrapperPreserveColor();
     if (IsDOMBinding() && obj && js::IsProxy(obj)) {
-      DOMProxyHandler::GetAndClearExpandoObject(obj);
+      DOMProxyHandler::ClearExternalRefsForWrapperRelease(obj);
     }
     SetPreservingWrapper(false);
     cyclecollector::DropJSObjectsImpl(aScriptObjectHolder);
   }
 }
 
 #ifdef DEBUG
 
--- a/dom/bindings/DOMJSProxyHandler.cpp
+++ b/dom/bindings/DOMJSProxyHandler.cpp
@@ -66,16 +66,43 @@ struct SetDOMProxyInformation
     js::SetDOMProxyInformation((const void*) &DOMProxyHandler::family,
                                JSPROXYSLOT_EXPANDO, DOMProxyShadows);
   }
 };
 
 SetDOMProxyInformation gSetDOMProxyInformation;
 
 // static
+void
+DOMProxyHandler::ClearExternalRefsForWrapperRelease(JSObject* obj)
+{
+  MOZ_ASSERT(IsDOMProxy(obj), "expected a DOM proxy object");
+  JS::Value v = js::GetProxyExtra(obj, JSPROXYSLOT_EXPANDO);
+  if (v.isUndefined()) {
+    // No expando.
+    return;
+  }
+
+  // See EnsureExpandoObject for the work we're trying to undo here.
+
+  if (v.isObject()) {
+    // Drop us from the DOM expando hashtable.  Don't worry about clearing our
+    // slot reference to the expando; we're about to die anyway.
+    xpc::ObjectScope(obj)->RemoveDOMExpandoObject(obj);
+    return;
+  }
+
+  // Prevent having a dangling pointer to our expando from the
+  // ExpandoAndGeneration.
+  js::ExpandoAndGeneration* expandoAndGeneration =
+    static_cast<js::ExpandoAndGeneration*>(v.toPrivate());
+  expandoAndGeneration->expando = UndefinedValue();
+}
+
+// static
 JSObject*
 DOMProxyHandler::GetAndClearExpandoObject(JSObject* obj)
 {
   MOZ_ASSERT(IsDOMProxy(obj), "expected a DOM proxy object");
   JS::Value v = js::GetProxyExtra(obj, JSPROXYSLOT_EXPANDO);
   if (v.isUndefined()) {
     return nullptr;
   }
@@ -85,16 +112,29 @@ DOMProxyHandler::GetAndClearExpandoObjec
     xpc::ObjectScope(obj)->RemoveDOMExpandoObject(obj);
   } else {
     js::ExpandoAndGeneration* expandoAndGeneration =
       static_cast<js::ExpandoAndGeneration*>(v.toPrivate());
     v = expandoAndGeneration->expando;
     if (v.isUndefined()) {
       return nullptr;
     }
+    // We have to expose v to active JS here.  The reason for that is that we
+    // might be in the middle of a GC right now.  If our proxy hasn't been
+    // traced yet, when it _does_ get traced it won't trace the expando, since
+    // we're breaking that link.  But the Rooted we're presumably being placed
+    // into is also not going to trace us, because Rooted marking is done at
+    // the very beginning of the GC.  In that situation, we need to manually
+    // mark the expando as live here.  JS::ExposeValueToActiveJS will do just
+    // that for us.
+    //
+    // We don't need to do this in the non-expandoAndGeneration case, because
+    // in that case our value is stored in a slot and slots will already mark
+    // the old thing live when the value in the slot changes.
+    JS::ExposeValueToActiveJS(v);
     expandoAndGeneration->expando = UndefinedValue();
   }
 
 
   return &v.toObject();
 }
 
 // static
--- a/dom/bindings/DOMJSProxyHandler.h
+++ b/dom/bindings/DOMJSProxyHandler.h
@@ -127,20 +127,45 @@ public:
   /*
    * If assigning to proxy[id] hits a named setter with OverrideBuiltins or
    * an indexed setter, call it and set *done to true on success. Otherwise, set
    * *done to false.
    */
   virtual bool setCustom(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id,
                          JS::Handle<JS::Value> v, bool *done) const;
 
+  /*
+   * Get the expando object for the given DOM proxy.
+   */
   static JSObject* GetExpandoObject(JSObject* obj);
 
-  /* GetAndClearExpandoObject does not DROP or clear the preserving wrapper flag. */
+  /*
+   * Clear the "external references" to this object.  If you are not
+   * nsWrapperCAche::ReleaseWrapper, you do NOT want to be calling this method.
+   *
+   * XXXbz if we nixed the DOM expando hash and just had a finalizer that
+   * cleared out the value in the ExpandoAndGeneration in the shadowing case,
+   * could we just get rid of this function altogether?
+   */
+  static void ClearExternalRefsForWrapperRelease(JSObject* obj);
+
+  /*
+   * Clear the expando object for the given DOM proxy and return it.  This
+   * function will ensure that the returned object is exposed to active JS if
+   * the given object is exposed.
+   *
+   * GetAndClearExpandoObject does not DROP or clear the preserving wrapper
+   * flag.
+   */
   static JSObject* GetAndClearExpandoObject(JSObject* obj);
+
+  /*
+   * Ensure that the given proxy (obj) has an expando object, and return it.
+   * Returns null on failure.
+   */
   static JSObject* EnsureExpandoObject(JSContext* cx,
                                        JS::Handle<JSObject*> obj);
 
   static const char family;
 };
 
 // Class used by shadowing handlers (the ones that have [OverrideBuiltins].
 // This handles tracing the expando in ExpandoAndGeneration.