Bug 1352430 - Add barrier to wrapper cache to clear dying objects that have not yet been finalized r=bz r=sfink

* * *
Code review followup

--HG--
extra : rebase_source : 10c1fd603c2dd1ac2ff5770ae9aec2e9131681ce

dom/base/nsGlobalWindow.cpp

@@ -1161,7 +1161,7 @@ nsOuterWindowProxy::finalize(JSFreeOp *fop, JSObject *proxy) const
 {
   nsGlobalWindow* outerWindow = GetOuterWindow(proxy);
   if (outerWindow) {
-    outerWindow->ClearWrapper();
+    outerWindow->ClearWrapper(proxy);
 
     // Ideally we would use OnFinalize here, but it's possible that
     // EnsureScriptEnvironment will later be called on the window, and we don't
@@ -1728,7 +1728,7 @@ nsGlobalWindow::~nsGlobalWindow()
   MOZ_LOG(gDOMLeakPRLog, LogLevel::Debug, ("DOMWINDOW %p destroyed", this));
 
   if (IsOuterWindow()) {
-    JSObject *proxy = GetWrapperPreserveColor();
+    JSObject *proxy = GetWrapperMaybeDead();
     if (proxy) {
       js::SetProxyExtra(proxy, 0, js::PrivateValue(nullptr));
     }
@@ -3947,7 +3947,7 @@ void
 nsGlobalWindow::PoisonOuterWindowProxy(JSObject *aObject)
 {
   MOZ_ASSERT(IsOuterWindow());
-  if (aObject == GetWrapperPreserveColor()) {
+  if (aObject == GetWrapperMaybeDead()) {
     PoisonWrapper();
   }
 }

dom/base/nsWrapperCache.h

@@ -95,12 +95,25 @@ public:
    * object returned is not guaranteed to be kept alive past the next CC.
    *
    * This should only be called if you are certain that the return value won't
-   * be passed into a JS API function and that it won't be stored without being
+   * be passed into a JSAPI function and that it won't be stored without being
    * rooted (or otherwise signaling the stored value to the CC).
    */
-  JSObject* GetWrapperPreserveColor() const
+  JSObject* GetWrapperPreserveColor() const;
+
+  /**
+   * Get the cached wrapper.
+   *
+   * This getter does not check whether the wrapper is dead and in the process
+   * of being finalized.
+   *
+   * This should only be called if you really need to see the raw contents of
+   * this cache, for example as part of finalization. Don't store the result
+   * anywhere or pass it into JSAPI functions that may cause the value to
+   * escape.
+   */
+  JSObject* GetWrapperMaybeDead() const
   {
-    return GetWrapperJSObject();
+    return mWrapper;
   }
 
 #ifdef DEBUG
@@ -121,16 +134,25 @@ public:
   }
 
   /**
-   * Clear the wrapper. This should be called from the finalizer for the
-   * wrapper.
+   * Clear the cache.
    */
   void ClearWrapper()
   {
     MOZ_ASSERT(!PreservingWrapper(), "Clearing a preserved wrapper!");
-
     SetWrapperJSObject(nullptr);
   }
 
+  /**
+   * Clear the cache if it still contains a specific wrapper object. This should
+   * be called from the finalizer for the wrapper.
+   */
+  void ClearWrapper(JSObject* obj)
+  {
+    if (obj == mWrapper) {
+      ClearWrapper();
+    }
+  }
+
   /**
    * Update the wrapper if the object it contains is moved.
    *
@@ -294,11 +316,6 @@ private:
     SetWrapperFlags(WRAPPER_IS_NOT_DOM_BINDING);
   }
 
-  JSObject *GetWrapperJSObject() const
-  {
-    return mWrapper;
-  }
-
   void SetWrapperJSObject(JSObject* aWrapper);
 
   FlagsType GetWrapperFlags() const

dom/base/nsWrapperCacheInlines.h

@@ -11,6 +11,21 @@
 #include "js/GCAPI.h"
 #include "js/TracingAPI.h"
 
+inline JSObject*
+nsWrapperCache::GetWrapperPreserveColor() const
+{
+  JSObject* obj = mWrapper;
+  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();
+    return nullptr;
+  }
+  MOZ_ASSERT(obj == mWrapper);
+  return obj;
+}
+
 inline JSObject*
 nsWrapperCache::GetWrapper() const
 {

dom/bindings/BindingUtils.h

@@ -1318,18 +1318,20 @@ GetUseXBLScope(const ParentObject& aParentObject)
 
 template<class T>
 inline void
-ClearWrapper(T* p, nsWrapperCache* cache)
+ClearWrapper(T* p, nsWrapperCache* cache, JSObject* obj)
 {
-  cache->ClearWrapper();
+  JS::AutoAssertGCCallback inCallback;
+  cache->ClearWrapper(obj);
 }
 
 template<class T>
 inline void
-ClearWrapper(T* p, void*)
+ClearWrapper(T* p, void*, JSObject* obj)
 {
+  JS::AutoAssertGCCallback inCallback;
   nsWrapperCache* cache;
   CallQueryInterface(p, &cache);
-  ClearWrapper(p, cache);
+  ClearWrapper(p, cache, obj);
 }
 
 template<class T>

dom/bindings/Codegen.py

@@ -1652,14 +1652,14 @@ class CGAddPropertyHook(CGAbstractClassHook):
             """)
 
 
-def finalizeHook(descriptor, hookName, freeOp):
+def finalizeHook(descriptor, hookName, freeOp, obj):
     finalize = ""
     if descriptor.wrapperCache:
-        finalize += "ClearWrapper(self, self);\n"
+        finalize += "ClearWrapper(self, self, %s);\n" % obj
     if descriptor.interface.getExtendedAttribute('OverrideBuiltins'):
         finalize += "self->mExpandoAndGeneration.expando = JS::UndefinedValue();\n"
     if descriptor.isGlobal():
-        finalize += "mozilla::dom::FinalizeGlobal(CastToJSFreeOp(%s), obj);\n" % freeOp
+        finalize += "mozilla::dom::FinalizeGlobal(CastToJSFreeOp(%s), %s);\n" % (freeOp, obj)
     finalize += ("AddForDeferredFinalization<%s>(self);\n" %
                  descriptor.nativeType)
     return CGIfWrapper(CGGeneric(finalize), "self")
@@ -1675,7 +1675,8 @@ class CGClassFinalizeHook(CGAbstractClassHook):
                                      'void', args)
 
     def generate_code(self):
-        return finalizeHook(self.descriptor, self.name, self.args[0].name).define()
+        return finalizeHook(self.descriptor, self.name,
+                            self.args[0].name, self.args[1].name).define()
 
 
 class CGClassObjectMovedHook(CGAbstractClassHook):
@@ -12227,7 +12228,8 @@ class CGDOMJSProxyHandler_finalize(ClassMethod):
     def getBody(self):
         return (("%s* self = UnwrapPossiblyNotInitializedDOMObject<%s>(proxy);\n" %
                  (self.descriptor.nativeType, self.descriptor.nativeType)) +
-                finalizeHook(self.descriptor, FINALIZE_HOOK_NAME, self.args[0].name).define())
+                finalizeHook(self.descriptor, FINALIZE_HOOK_NAME,
+                             self.args[0].name, self.args[1].name).define())
 
 
 class CGDOMJSProxyHandler_getElements(ClassMethod):

dom/bindings/SimpleGlobalObject.cpp

@@ -46,6 +46,7 @@ SimpleGlobal_finalize(js::FreeOp *fop, JSObject *obj)
 {
   SimpleGlobalObject* globalObject =
     static_cast<SimpleGlobalObject*>(JS_GetPrivate(obj));
+  globalObject->ClearWrapper(obj);
   NS_RELEASE(globalObject);
 }
 

dom/bindings/SimpleGlobalObject.h

@@ -87,7 +87,7 @@ private:
 
   virtual ~SimpleGlobalObject()
   {
-    ClearWrapper();
+    MOZ_ASSERT(!GetWrapperMaybeDead());
   }
 
   const GlobalType mType;

js/public/GCAPI.h

@@ -656,6 +656,27 @@ ExposeGCThingToActiveJS(JS::GCCellPtr thing)
     MOZ_ASSERT(!js::gc::detail::TenuredCellIsMarkedGray(thing.asCell()));
 }
 
+template <typename T>
+extern JS_PUBLIC_API(bool)
+EdgeNeedsSweepUnbarrieredSlow(T* thingp);
+
+static MOZ_ALWAYS_INLINE bool
+EdgeNeedsSweepUnbarriered(JSObject** objp)
+{
+    // This function does not handle updating nursery pointers. Raw JSObject
+    // pointers should be updated separately or replaced with
+    // JS::Heap<JSObject*> which handles this automatically.
+    MOZ_ASSERT(!JS::CurrentThreadIsHeapMinorCollecting());
+    if (IsInsideNursery(reinterpret_cast<Cell*>(*objp)))
+        return false;
+
+    auto zone = JS::shadow::Zone::asShadowZone(detail::GetGCThingZone(uintptr_t(*objp)));
+    if (!zone->isGCSweepingOrCompacting())
+        return false;
+
+    return EdgeNeedsSweepUnbarrieredSlow(objp);
+}
+
 } /* namespace gc */
 } /* namespace js */
 
@@ -671,12 +692,14 @@ static MOZ_ALWAYS_INLINE void
 ExposeObjectToActiveJS(JSObject* obj)
 {
     MOZ_ASSERT(obj);
+    MOZ_ASSERT(!js::gc::EdgeNeedsSweepUnbarrieredSlow(&obj));
     js::gc::ExposeGCThingToActiveJS(GCCellPtr(obj));
 }
 
 static MOZ_ALWAYS_INLINE void
 ExposeScriptToActiveJS(JSScript* script)
 {
+    MOZ_ASSERT(!js::gc::EdgeNeedsSweepUnbarrieredSlow(&script));
     js::gc::ExposeGCThingToActiveJS(GCCellPtr(script));
 }
 

js/public/HeapAPI.h

@@ -101,19 +101,29 @@ namespace shadow {
 
 struct Zone
 {
+    enum GCState : uint8_t {
+        NoGC,
+        Mark,
+        MarkGray,
+        Sweep,
+        Finished,
+        Compact
+    };
+
   protected:
     JSRuntime* const runtime_;
     JSTracer* const barrierTracer_;     // A pointer to the JSRuntime's |gcMarker|.
-
-  public:
     bool needsIncrementalBarrier_;
+    GCState gcState_;
 
     Zone(JSRuntime* runtime, JSTracer* barrierTracerArg)
       : runtime_(runtime),
         barrierTracer_(barrierTracerArg),
-        needsIncrementalBarrier_(false)
+        needsIncrementalBarrier_(false),
+        gcState_(NoGC)
     {}
 
+  public:
     bool needsIncrementalBarrier() const {
         return needsIncrementalBarrier_;
     }
@@ -135,6 +145,15 @@ struct Zone
         return runtime_;
     }
 
+    GCState gcState() const { return gcState_; }
+    bool wasGCStarted() const { return gcState_ != NoGC; }
+    bool isGCMarkingBlack() { return gcState_ == Mark; }
+    bool isGCMarkingGray() { return gcState_ == MarkGray; }
+    bool isGCSweeping() { return gcState_ == Sweep; }
+    bool isGCFinished() { return gcState_ == Finished; }
+    bool isGCCompacting() { return gcState_ == Compact; }
+    bool isGCSweepingOrCompacting() { return gcState_ == Sweep || gcState_ == Compact; }
+
     static MOZ_ALWAYS_INLINE JS::shadow::Zone* asShadowZone(JS::Zone* zone) {
         return reinterpret_cast<JS::shadow::Zone*>(zone);
     }

js/public/Value.h

@@ -960,6 +960,10 @@ IsOptimizedPlaceholderMagicValue(const Value& v)
 static MOZ_ALWAYS_INLINE void
 ExposeValueToActiveJS(const Value& v)
 {
+#ifdef DEBUG
+    Value tmp = v;
+    MOZ_ASSERT(!js::gc::EdgeNeedsSweepUnbarrieredSlow(&tmp));
+#endif
     if (v.isGCThing())
         js::gc::ExposeGCThingToActiveJS(GCCellPtr(v));
 }

js/src/gc/Marking.cpp

@@ -3153,8 +3153,8 @@ IsAboutToBeFinalizedInternal(T** thingp)
         return false;
 
     if (IsInsideNursery(thing)) {
-        MOZ_ASSERT(JS::CurrentThreadIsHeapMinorCollecting());
-        return !Nursery::getForwardedPointer(reinterpret_cast<JSObject**>(thingp));
+        return JS::CurrentThreadIsHeapMinorCollecting() &&
+               !Nursery::getForwardedPointer(reinterpret_cast<JSObject**>(thingp));
     }
 
     Zone* zone = thing->asTenured().zoneFromAnyThread();
@@ -3230,6 +3230,13 @@ EdgeNeedsSweep(JS::Heap<T>* thingp)
     return IsAboutToBeFinalizedInternal(ConvertToBase(thingp->unsafeGet()));
 }
 
+template <typename T>
+JS_PUBLIC_API(bool)
+EdgeNeedsSweepUnbarrieredSlow(T* thingp)
+{
+    return IsAboutToBeFinalizedInternal(ConvertToBase(thingp));
+}
+
 // Instantiate a copy of the Tracing templates for each derived type.
 #define INSTANTIATE_ALL_VALID_TRACE_FUNCTIONS(type) \
     template bool IsMarkedUnbarriered<type>(JSRuntime*, type*);                \
@@ -3238,7 +3245,8 @@ EdgeNeedsSweep(JS::Heap<T>* thingp)
     template bool IsAboutToBeFinalized<type>(WriteBarrieredBase<type>*); \
     template bool IsAboutToBeFinalized<type>(ReadBarrieredBase<type>*);
 #define INSTANTIATE_ALL_VALID_HEAP_TRACE_FUNCTIONS(type) \
-    template JS_PUBLIC_API(bool) EdgeNeedsSweep<type>(JS::Heap<type>*);
+    template JS_PUBLIC_API(bool) EdgeNeedsSweep<type>(JS::Heap<type>*); \
+    template JS_PUBLIC_API(bool) EdgeNeedsSweepUnbarrieredSlow<type>(type*);
 FOR_EACH_GC_POINTER_TYPE(INSTANTIATE_ALL_VALID_TRACE_FUNCTIONS)
 FOR_EACH_PUBLIC_GC_POINTER_TYPE(INSTANTIATE_ALL_VALID_HEAP_TRACE_FUNCTIONS)
 FOR_EACH_PUBLIC_TAGGED_GC_POINTER_TYPE(INSTANTIATE_ALL_VALID_HEAP_TRACE_FUNCTIONS)

js/src/gc/Zone.cpp

@@ -55,7 +55,6 @@ JS::Zone::Zone(JSRuntime* rt, ZoneGroup* group)
     gcLastSweepGroupIndex(group, 0),
 #endif
     jitZone_(group, nullptr),
-    gcState_(NoGC),
     gcScheduled_(false),
     gcPreserveCode_(group, false),
     jitUsingBarriers_(group, false),

js/src/gc/Zone.h

@@ -215,14 +215,6 @@ struct Zone : public JS::shadow::Zone,
 
     void notifyObservingDebuggers();
 
-    enum GCState {
-        NoGC,
-        Mark,
-        MarkGray,
-        Sweep,
-        Finished,
-        Compact
-    };
     void setGCState(GCState state) {
         MOZ_ASSERT(CurrentThreadIsHeapBusy());
         MOZ_ASSERT_IF(state != NoGC, canCollect());
@@ -257,15 +249,6 @@ struct Zone : public JS::shadow::Zone,
             return needsIncrementalBarrier();
     }
 
-    GCState gcState() const { return gcState_; }
-    bool wasGCStarted() const { return gcState_ != NoGC; }
-    bool isGCMarkingBlack() { return gcState_ == Mark; }
-    bool isGCMarkingGray() { return gcState_ == MarkGray; }
-    bool isGCSweeping() { return gcState_ == Sweep; }
-    bool isGCFinished() { return gcState_ == Finished; }
-    bool isGCCompacting() { return gcState_ == Compact; }
-    bool isGCSweepingOrCompacting() { return gcState_ == Sweep || gcState_ == Compact; }
-
     // Get a number that is incremented whenever this zone is collected, and
     // possibly at other times too.
     uint64_t gcNumber();
@@ -621,7 +604,6 @@ struct Zone : public JS::shadow::Zone,
   private:
     js::ZoneGroupData<js::jit::JitZone*> jitZone_;
 
-    js::UnprotectedData<GCState> gcState_;
     js::ActiveThreadData<bool> gcScheduled_;
     js::ZoneGroupData<bool> gcPreserveCode_;
     js::ZoneGroupData<bool> jitUsingBarriers_;

js/xpconnect/src/Sandbox.cpp

@@ -415,7 +415,7 @@ sandbox_finalize(js::FreeOp* fop, JSObject* obj)
         return;
     }
 
-    static_cast<SandboxPrivate*>(sop)->ForgetGlobalObject();
+    static_cast<SandboxPrivate*>(sop)->ForgetGlobalObject(obj);
     DestroyProtoAndIfaceCache(obj);
     DeferredFinalize(sop);
 }

js/xpconnect/src/SandboxPrivate.h

@@ -43,9 +43,9 @@ public:
         return GetWrapper();
     }
 
-    void ForgetGlobalObject()
+    void ForgetGlobalObject(JSObject* obj)
     {
-        ClearWrapper();
+        ClearWrapper(obj);
     }
 
     virtual JSObject* WrapObject(JSContext* cx, JS::Handle<JSObject*> aGivenProto) override

js/xpconnect/src/XPCWrappedNative.cpp

@@ -862,7 +862,7 @@ XPCWrappedNative::FlatJSObjectFinalized()
     nsWrapperCache* cache = nullptr;
     CallQueryInterface(mIdentity, &cache);
     if (cache)
-        cache->ClearWrapper();
+        cache->ClearWrapper(mFlatJSObject.unbarrieredGetPtr());
 
     mFlatJSObject = nullptr;
     mFlatJSObject.unsetFlags(FLAT_JS_OBJECT_VALID);

netwerk/base/nsUDPSocket.cpp

@@ -31,6 +31,7 @@
 #include "nsIDNSRecord.h"
 #include "nsIDNSService.h"
 #include "nsICancelable.h"
+#include "nsWrapperCacheInlines.h"
 
 #ifdef MOZ_WIDGET_GONK
 #include "NetStatistics.h"

xpcom/base/CycleCollectedJSRuntime.cpp

@@ -1181,7 +1181,7 @@ CycleCollectedJSRuntime::JSObjectsTenured()
 
   for (auto iter = mNurseryObjects.Iter(); !iter.Done(); iter.Next()) {
     nsWrapperCache* cache = iter.Get();
-    JSObject* wrapper = cache->GetWrapperPreserveColor();
+    JSObject* wrapper = cache->GetWrapperMaybeDead();
     MOZ_DIAGNOSTIC_ASSERT(wrapper);
     if (!JS::ObjectIsTenured(wrapper)) {
       MOZ_ASSERT(!cache->PreservingWrapper());
@@ -1205,8 +1205,8 @@ CycleCollectedJSRuntime::NurseryWrapperAdded(nsWrapperCache* aCache)
 {
   MOZ_ASSERT(mJSContext);
   MOZ_ASSERT(aCache);
-  MOZ_ASSERT(aCache->GetWrapperPreserveColor());
-  MOZ_ASSERT(!JS::ObjectIsTenured(aCache->GetWrapperPreserveColor()));
+  MOZ_ASSERT(aCache->GetWrapperMaybeDead());
+  MOZ_ASSERT(!JS::ObjectIsTenured(aCache->GetWrapperMaybeDead()));
   mNurseryObjects.InfallibleAppend(aCache);
 }