From 5afc8a9e030aade89c63b17409dde1447cefa2d4 Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@mit.edu>
Date: Thu, 15 Sep 2016 15:04:56 -0400
Subject: [PATCH] Bug 1294747.  Make sure we expose the expando of a
 [OverrideBuiltins] proxy to active JS when it gets cleared from the proxy. 
 r=peterv

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        |  2 +-
 dom/bindings/DOMJSProxyHandler.cpp | 40 ++++++++++++++++++++++++++++++
 dom/bindings/DOMJSProxyHandler.h   | 27 +++++++++++++++++++-
 3 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/dom/base/nsWrapperCache.cpp b/dom/base/nsWrapperCache.cpp
index 0130c8e87aba..aa46e6c1c314 100644
--- a/dom/base/nsWrapperCache.cpp
+++ b/dom/base/nsWrapperCache.cpp
@@ -55,7 +55,7 @@ nsWrapperCache::ReleaseWrapper(void* aScriptObjectHolder)
     // from both here.
     JSObject* obj = GetWrapperPreserveColor();
     if (IsDOMBinding() && obj && js::IsProxy(obj)) {
-      DOMProxyHandler::GetAndClearExpandoObject(obj);
+      DOMProxyHandler::ClearExternalRefsForWrapperRelease(obj);
     }
     SetPreservingWrapper(false);
     cyclecollector::DropJSObjectsImpl(aScriptObjectHolder);
diff --git a/dom/bindings/DOMJSProxyHandler.cpp b/dom/bindings/DOMJSProxyHandler.cpp
index 5fc63a71e026..65e540bc1daa 100644
--- a/dom/bindings/DOMJSProxyHandler.cpp
+++ b/dom/bindings/DOMJSProxyHandler.cpp
@@ -70,6 +70,33 @@ struct SetDOMProxyInformation
 
 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)
@@ -90,6 +117,19 @@ DOMProxyHandler::GetAndClearExpandoObject(JSObject* obj)
     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();
   }
 
diff --git a/dom/bindings/DOMJSProxyHandler.h b/dom/bindings/DOMJSProxyHandler.h
index b99d3f2b9cc4..544ad52e83a7 100644
--- a/dom/bindings/DOMJSProxyHandler.h
+++ b/dom/bindings/DOMJSProxyHandler.h
@@ -132,10 +132,35 @@ public:
   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);