Bug 1536154 - Eagerly run finalizer for any dead reflector JSObject when creating a new reflector for a DOM native r=bzbarsky
authorJon Coppeard <jcoppeard@mozilla.com>
Wed, 24 Apr 2019 15:58:39 +0100
changeset 530896 b6ca67e7684f4f9409b8e22e0b52afa8e1de2dab
parent 530895 24e75c822fd37393db9b1237ca42defe4ac89639
child 530897 5159ad4a890bf2e2fd94972d798ac9e22f929168
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs1536154
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1536154 - Eagerly run finalizer for any dead reflector JSObject when creating a new reflector for a DOM native r=bzbarsky Currently incremental GC can run the finalizer for a dead reflector for a native after a new reflector for that native has been created and attached. This leads to the confusing situation where there are two reflectors that contain pointers to the same native (which has a pointer to the new one). This is a problem for memory accounting because the JS engine sees the size of the native at finalization time but does not see updates to this size after a new reflector is created. Thus the engine's idea of the size of a native can become incorrect and the memory accounting can become unbalanced. Consider the following situation: 1. Native object created of size 20MB 2. Reflector 1 created 3. Reflector 1 becomes unreachable 4. Reflector 2 created 5. Native size changes to 40MB 6. Reflector 1 finalized The memory associated with reflector 1 will be: 20MB (step 2), -20MB (step 6) The memory associated with reflector 2 will be: 20MB (step 4), 40MB (step 5) The memory associated with reflector 1 ends up negative (which should not be possible) and the total is also wrong. The patch runs the finalizer for any dead reflector when creating a new one. This ensures that finalizer sees the correct state. The native object pointer is cleared when this happens so when the GC later runs the finalizer again it is a no-op. This situation occurs pretty rarely so I don't think there is much overhead to running the finalizer more than once. This also allows us to tighten up the assertions in the finalizer. Differential Revision: https://phabricator.services.mozilla.com/D28690
dom/base/nsWrapperCacheInlines.h
dom/bindings/BindingUtils.h
dom/bindings/Codegen.py
--- a/dom/base/nsWrapperCacheInlines.h
+++ b/dom/base/nsWrapperCacheInlines.h
@@ -9,19 +9,19 @@
 
 #include "nsWrapperCache.h"
 #include "js/TracingAPI.h"
 
 inline JSObject* nsWrapperCache::GetWrapperPreserveColor() const {
   JSObject* obj = GetWrapperMaybeDead();
   if (obj && js::gc::EdgeNeedsSweepUnbarriered(&obj)) {
     // The object has been found to be dead and is in the process of being
-    // finalized, so don't let the caller see it. As an optimisation, remove it
-    // from the cache so we don't have to do this check in future.
-    const_cast<nsWrapperCache*>(this)->ClearWrapper();
+    // finalized, so don't let the caller see it.
+    // Don't clear the cache though: this happens when a new wrapper is created
+    // for this native or when the wrapper is finalized.
     return nullptr;
   }
   MOZ_ASSERT(obj == mWrapper);
   return obj;
 }
 
 inline JSObject* nsWrapperCache::GetWrapper() const {
   JSObject* obj = GetWrapperPreserveColor();
--- a/dom/bindings/BindingUtils.h
+++ b/dom/bindings/BindingUtils.h
@@ -1379,33 +1379,33 @@ inline mozilla::dom::ReflectionScope Get
 
 inline mozilla::dom::ReflectionScope GetReflectionScope(
     const ParentObject& aParentObject) {
   return aParentObject.mReflectionScope;
 }
 
 template <class T>
 inline void ClearWrapper(T* p, nsWrapperCache* cache, JSObject* obj) {
-  JS::AutoAssertGCCallback inCallback;
-
   // Skip clearing the wrapper when replaying. This method is called during
   // finalization of |obj|, and when replaying a strong reference is kept on
   // the contents of the cache: since |obj| is being finalized, the cache
   // cannot point to |obj|, and clearing here won't do anything.
   // Additionally, the reference held on the cache may have already been
   // released, if we are finalizing later than we did while recording, and the
   // cache may have already been deleted.
   if (!recordreplay::IsReplaying()) {
+    MOZ_ASSERT(cache->GetWrapperMaybeDead() == obj);
     cache->ClearWrapper(obj);
   }
 }
 
 template <class T>
 inline void ClearWrapper(T* p, void*, JSObject* obj) {
-  JS::AutoAssertGCCallback inCallback;
+  // QueryInterface to nsWrapperCache can't GC, we hope.
+  JS::AutoSuppressGCAnalysis nogc;
 
   // Skip clearing the wrapper when replaying, for the same reason as in the
   // overload above: |p| may have been deleted and we cannot QI it.
   if (!recordreplay::IsReplaying()) {
     nsWrapperCache* cache;
     CallQueryInterface(p, &cache);
     ClearWrapper(p, cache, obj);
   }
--- a/dom/bindings/Codegen.py
+++ b/dom/bindings/Codegen.py
@@ -1725,17 +1725,17 @@ class CGAddPropertyHook(CGAbstractClassH
             if (self && self->GetWrapperPreserveColor()) {
               PreserveWrapper(self);
             }
             return true;
             """)
 
 
 def finalizeHook(descriptor, hookName, freeOp, obj):
-    finalize = ""
+    finalize = "js::SetReservedSlot(%s, DOM_OBJECT_SLOT, JS::UndefinedValue());\n" % obj
     if descriptor.interface.getExtendedAttribute('OverrideBuiltins'):
         finalize += fill(
             """
             // Either our proxy created an expando object or not.  If it did,
             // then we would have preserved ourselves, and hence if we're going
             // away so is our C++ object and we should reset its expando value.
             // It's possible that in this situation the C++ object's reflector
             // pointer has been nulled out, but if not it's pointing to us.  If
@@ -3842,29 +3842,45 @@ class CGWrapWithCacheMethod(CGAbstractMe
     def definition_body(self):
         failureCode = dedent(
             """
             aCache->ReleaseWrapper(aObject);
             aCache->ClearWrapper();
             return false;
             """)
 
+        if self.descriptor.proxy:
+            finalize = "DOMProxyHandler::getInstance()->finalize"
+        else:
+            finalize = FINALIZE_HOOK_NAME
+
         return fill(
             """
             static_assert(!IsBaseOf<NonRefcountedDOMObject, ${nativeType}>::value,
                           "Shouldn't have wrappercached things that are not refcounted.");
             $*{assertInheritance}
             MOZ_ASSERT_IF(aGivenProto, js::IsObjectInContextCompartment(aGivenProto, aCx));
             MOZ_ASSERT(!aCache->GetWrapper(),
                        "You should probably not be using Wrap() directly; use "
                        "GetOrCreateDOMReflector instead");
 
             MOZ_ASSERT(ToSupportsIsOnPrimaryInheritanceChain(aObject, aCache),
                        "nsISupports must be on our primary inheritance chain");
 
+            // If the wrapper cache contains a dead reflector then finalize that
+            // now, ensuring that the finalizer for the old reflector always
+            // runs before the new reflector is created and attached. This
+            // avoids the awkward situation where there are multiple reflector
+            // objects that contain pointers to the same native.
+
+            if (JSObject* oldReflector = aCache->GetWrapperMaybeDead()) {
+              ${finalize}(nullptr /* unused */, oldReflector);
+              MOZ_ASSERT(!aCache->GetWrapperMaybeDead());
+            }
+
             JS::Rooted<JSObject*> global(aCx, FindAssociatedGlobal(aCx, aObject->GetParentObject()));
             if (!global) {
               return false;
             }
             MOZ_ASSERT(JS_IsGlobalObject(global));
             JS::AssertObjectIsNotGray(global);
 
             // That might have ended up wrapping us already, due to the wonders
@@ -3902,17 +3918,18 @@ class CGWrapWithCacheMethod(CGAbstractMe
             return true;
             """,
             nativeType=self.descriptor.nativeType,
             assertInheritance=AssertInheritanceChain(self.descriptor),
             declareProto=DeclareProto(self.descriptor),
             createObject=CreateBindingJSObject(self.descriptor, self.properties),
             unforgeable=CopyUnforgeablePropertiesToInstance(self.descriptor,
                                                             failureCode),
-            slots=InitMemberSlots(self.descriptor, failureCode))
+            slots=InitMemberSlots(self.descriptor, failureCode),
+            finalize=finalize)
 
 
 class CGWrapMethod(CGAbstractMethod):
     def __init__(self, descriptor):
         # XXX can we wrap if we don't have an interface prototype object?
         assert descriptor.interface.hasInterfacePrototypeObject()
         args = [Argument('JSContext*', 'aCx'),
                 Argument('T*', 'aObject'),
@@ -12864,24 +12881,24 @@ class CGDOMJSProxyHandler_set(ClassMetho
             }
 
             JS::Rooted<JS::Value> wrappedValue(cx, v);
             if (!MaybeWrapValue(cx, &wrappedValue)) {
               return false;
             }
 
             JS_MarkCrossZoneId(cx, id);
-            
+
             return dom::DOMProxyHandler::set(cx, proxy, id, wrappedValue, wrappedReceiver, result);
             """)
 
 
 class CGDOMJSProxyHandler_EnsureHolder(ClassMethod):
     """
-    Implementation of set().  We only use this for cross-origin objects. 
+    Implementation of set().  We only use this for cross-origin objects.
     """
     def __init__(self, descriptor):
         args = [Argument('JSContext*', 'cx'),
                 Argument('JS::Handle<JSObject*>', 'proxy'),
                 Argument('JS::MutableHandle<JSObject*>', 'holder')]
         ClassMethod.__init__(self, "EnsureHolder", "bool", args,
                              virtual=True, override=True, const=True)
         self.descriptor = descriptor