Bug 1407593 - Report embedding leaks of JS GC things r=sfink,sfink?

Patch to report JS GC things that are live at shutdown as leaks by calling MOZ_LOG_CTOR for them. The JS engine now clears any remaining roots to prevent dangling pointers to freed memory after shutdown. I made XPCJSRuntime::Shutdown keep the weak pointer update callbacks as sometimes these tables have entries remaining at shutdown (if there are leaks) and we need the callbacks to update them.

This is looking more green on try now.

Differential Revision: https://phabricator.services.mozilla.com/D40449

--HG--
extra : moz-landing-system : lando

dom/base/nsWrapperCache.h

@@ -106,7 +106,10 @@ class nsWrapperCache {
     if (mozilla::recordreplay::IsReplaying()) {
       mozilla::recordreplay::SetWeakPointerJSRoot(this, nullptr);
     }
-    MOZ_ASSERT(!PreservingWrapper(),
+    // Preserved wrappers should never end up getting cleared, but this can
+    // happen during shutdown when a leaked wrapper object is finalized, causing
+    // its wrapper to be cleared.
+    MOZ_ASSERT(!PreservingWrapper() || js::RuntimeIsBeingDestroyed(),
                "Destroying cache with a preserved wrapper!");
   }
 
@@ -183,7 +186,11 @@ class nsWrapperCache {
    * Clear the cache.
    */
   void ClearWrapper() {
-    MOZ_ASSERT(!PreservingWrapper(), "Clearing a preserved wrapper!");
+    // Preserved wrappers should never end up getting cleared, but this can
+    // happen during shutdown when a leaked wrapper object is finalized, causing
+    // its wrapper to be cleared.
+    MOZ_ASSERT(!PreservingWrapper() || js::RuntimeIsBeingDestroyed(),
+               "Clearing a preserved wrapper!");
     SetWrapperJSObject(nullptr);
   }
 

js/public/TracingAPI.h

@@ -24,6 +24,9 @@ class TenuredHeap;
 /** Returns a static string equivalent of |kind|. */
 JS_FRIEND_API const char* GCTraceKindToAscii(JS::TraceKind kind);
 
+/** Returns the base size in bytes of the GC thing of kind |kind|. */
+JS_FRIEND_API size_t GCTraceKindSize(JS::TraceKind kind);
+
 }  // namespace JS
 
 enum WeakMapTraceKind {
@@ -477,6 +480,15 @@ template <typename T>
 bool IsAboutToBeFinalizedUnbarriered(T* thingp);
 
 }  // namespace gc
+
+#ifdef DEBUG
+/*
+ * Return whether the runtime is currently being destroyed, for use in
+ * assertions.
+ */
+extern JS_FRIEND_API bool RuntimeIsBeingDestroyed();
+#endif
+
 }  // namespace js
 
 #endif /* js_TracingAPI_h */

js/src/gc/ClearEdgesTracer.h

@@ -0,0 +1,44 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
+ * vim: set ts=8 sts=2 et sw=2 tw=80:
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef gc_ClearEdgesTracer_h
+#define gc_ClearEdgesTracer_h
+
+#include "js/TracingAPI.h"
+
+namespace js {
+namespace gc {
+
+struct ClearEdgesTracer final : public JS::CallbackTracer {
+  explicit ClearEdgesTracer(JSRuntime* rt);
+  ClearEdgesTracer();
+
+#ifdef DEBUG
+  TracerKind getTracerKind() const override { return TracerKind::ClearEdges; }
+#endif
+
+  template <typename T>
+  inline bool clearEdge(T** thingp);
+
+  bool onObjectEdge(JSObject** objp) override;
+  bool onStringEdge(JSString** strp) override;
+  bool onSymbolEdge(JS::Symbol** symp) override;
+  bool onBigIntEdge(JS::BigInt** bip) override;
+  bool onScriptEdge(JSScript** scriptp) override;
+  bool onShapeEdge(js::Shape** shapep) override;
+  bool onObjectGroupEdge(js::ObjectGroup** groupp) override;
+  bool onBaseShapeEdge(js::BaseShape** basep) override;
+  bool onJitCodeEdge(js::jit::JitCode** codep) override;
+  bool onLazyScriptEdge(js::LazyScript** lazyp) override;
+  bool onScopeEdge(js::Scope** scopep) override;
+  bool onRegExpSharedEdge(js::RegExpShared** sharedp) override;
+  bool onChild(const JS::GCCellPtr& thing) override;
+};
+
+}  // namespace gc
+}  // namespace js
+
+#endif  // gc_ClearEdgesTracer_h

js/src/gc/DeletePolicy.h

@@ -7,37 +7,9 @@
 #ifndef gc_DeletePolicy_h
 #define gc_DeletePolicy_h
 
-#include "js/TracingAPI.h"
+#include "gc/ClearEdgesTracer.h"
 
 namespace js {
-namespace gc {
-
-struct ClearEdgesTracer final : public JS::CallbackTracer {
-  ClearEdgesTracer();
-
-#ifdef DEBUG
-  TracerKind getTracerKind() const override { return TracerKind::ClearEdges; }
-#endif
-
-  template <typename T>
-  inline bool clearEdge(T** thingp);
-
-  bool onObjectEdge(JSObject** objp) override;
-  bool onStringEdge(JSString** strp) override;
-  bool onSymbolEdge(JS::Symbol** symp) override;
-  bool onBigIntEdge(JS::BigInt** bip) override;
-  bool onScriptEdge(JSScript** scriptp) override;
-  bool onShapeEdge(js::Shape** shapep) override;
-  bool onObjectGroupEdge(js::ObjectGroup** groupp) override;
-  bool onBaseShapeEdge(js::BaseShape** basep) override;
-  bool onJitCodeEdge(js::jit::JitCode** codep) override;
-  bool onLazyScriptEdge(js::LazyScript** lazyp) override;
-  bool onScopeEdge(js::Scope** scopep) override;
-  bool onRegExpSharedEdge(js::RegExpShared** sharedp) override;
-  bool onChild(const JS::GCCellPtr& thing) override;
-};
-
-}  // namespace gc
 
 /*
  * Provides a delete policy that can be used for objects which have their

js/src/gc/GC.cpp

@@ -1912,6 +1912,12 @@ void GCRuntime::setGrayRootsTracer(JSTraceDataOp traceOp, void* data) {
   grayRootTracer.data = data;
 }
 
+void GCRuntime::clearBlackAndGrayRootTracers() {
+  MOZ_ASSERT(rt->isBeingDestroyed());
+  blackRootTracers.ref().clear();
+  setGrayRootsTracer(nullptr, nullptr);
+}
+
 void GCRuntime::setGCCallback(JSGCCallback callback, void* data) {
   gcCallback.op = callback;
   gcCallback.data = data;
@@ -4544,7 +4550,11 @@ bool GCRuntime::beginMarkPhase(JS::GCReason reason, AutoGCSession& session) {
    * Mark phase.
    */
   gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::MARK);
-  traceRuntimeForMajorGC(gcmarker, session);
+  if (rt->isBeingDestroyed()) {
+    checkNoRuntimeRoots(session);
+  } else {
+    traceRuntimeForMajorGC(gcmarker, session);
+  }
 
   if (isIncremental) {
     markCompartments();
@@ -8581,7 +8591,7 @@ JS_FRIEND_API const char* JS::GCTraceKindToAscii(JS::TraceKind kind) {
   switch (kind) {
 #define MAP_NAME(name, _0, _1, _2) \
   case JS::TraceKind::name:        \
-    return #name;
+    return "JS " #name;
     JS_FOR_EACH_TRACEKIND(MAP_NAME);
 #undef MAP_NAME
     default:
@@ -8589,6 +8599,18 @@ JS_FRIEND_API const char* JS::GCTraceKindToAscii(JS::TraceKind kind) {
   }
 }
 
+JS_FRIEND_API size_t JS::GCTraceKindSize(JS::TraceKind kind) {
+  switch (kind) {
+#define MAP_SIZE(name, type, _0, _1) \
+  case JS::TraceKind::name:          \
+    return sizeof(type);
+    JS_FOR_EACH_TRACEKIND(MAP_SIZE);
+#undef MAP_SIZE
+    default:
+      return 0;
+  }
+}
+
 JS::GCCellPtr::GCCellPtr(const Value& v) : ptr(0) {
   switch (v.type()) {
     case ValueType::String:
@@ -9192,8 +9214,11 @@ extern JS_PUBLIC_API bool js::gc::detail::ObjectIsMarkedBlack(
 
 #endif
 
+js::gc::ClearEdgesTracer::ClearEdgesTracer(JSRuntime* rt)
+    : CallbackTracer(rt, TraceWeakMapKeysValues) {}
+
 js::gc::ClearEdgesTracer::ClearEdgesTracer()
-    : CallbackTracer(TlsContext.get(), TraceWeakMapKeysValues) {}
+    : ClearEdgesTracer(TlsContext.get()->runtime()) {}
 
 template <typename S>
 inline bool js::gc::ClearEdgesTracer::clearEdge(S** thingp) {

js/src/gc/GCRuntime.h

@@ -371,6 +371,7 @@ class GCRuntime {
   void setGrayRootsTracer(JSTraceDataOp traceOp, void* data);
   MOZ_MUST_USE bool addBlackRootsTracer(JSTraceDataOp traceOp, void* data);
   void removeBlackRootsTracer(JSTraceDataOp traceOp, void* data);
+  void clearBlackAndGrayRootTracers();
 
   void updateMemoryCountersOnGCStart();
 
@@ -612,6 +613,9 @@ class GCRuntime {
   void traceRuntimeAtoms(JSTracer* trc, const AutoAccessAtomsZone& atomsAccess);
   void traceKeptAtoms(JSTracer* trc);
   void traceRuntimeCommon(JSTracer* trc, TraceOrMarkRuntime traceOrMark);
+  void traceEmbeddingBlackRoots(JSTracer* trc);
+  void traceEmbeddingGrayRoots(JSTracer* trc);
+  void checkNoRuntimeRoots(AutoGCSession& session);
   void maybeDoCycleCollection();
   void markCompartments();
   IncrementalProgress markUntilBudgetExhausted(SliceBudget& sliceBudget,

js/src/gc/Marking-inl.h

@@ -37,6 +37,7 @@ struct TaggedPtr<JS::Value> {
                   "Type must be a GC thing derived from js::gc::Cell");
     return JS::PrivateGCThingValue(priv);
   }
+  static JS::Value empty() { return JS::UndefinedValue(); }
 };
 
 template <>
@@ -45,11 +46,13 @@ struct TaggedPtr<jsid> {
     return NON_INTEGER_ATOM_TO_JSID(&str->asAtom());
   }
   static jsid wrap(JS::Symbol* sym) { return SYMBOL_TO_JSID(sym); }
+  static jsid empty() { return JSID_VOID; }
 };
 
 template <>
 struct TaggedPtr<TaggedProto> {
   static TaggedProto wrap(JSObject* obj) { return TaggedProto(obj); }
+  static TaggedProto empty() { return TaggedProto(); }
 };
 
 template <typename T>

js/src/gc/RootMarking.cpp

@@ -13,6 +13,7 @@
 #include "builtin/MapObject.h"
 #include "debugger/DebugAPI.h"
 #include "frontend/BytecodeCompiler.h"
+#include "gc/ClearEdgesTracer.h"
 #include "gc/GCInternals.h"
 #include "gc/Marking.h"
 #include "jit/MacroAssembler.h"
@@ -270,12 +271,6 @@ void js::gc::GCRuntime::traceRuntimeForMajorGC(JSTracer* trc,
                                                AutoGCSession& session) {
   MOZ_ASSERT(!TlsContext.get()->suppressGC);
 
-  // FinishRoots will have asserted that every root that we do not expect
-  // is gone, so we can simply skip traceRuntime here.
-  if (rt->isBeingDestroyed()) {
-    return;
-  }
-
   gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::MARK_ROOTS);
   if (atomsZone->isCollecting()) {
     traceRuntimeAtoms(trc, session.checkAtomsAccess());
@@ -390,9 +385,6 @@ void js::gc::GCRuntime::traceRuntimeCommon(JSTracer* trc,
   if (!JS::RuntimeHeapIsMinorCollecting()) {
     gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::MARK_EMBEDDING);
 
-    // The analysis doesn't like the function pointers below.
-    JS::AutoSuppressGCAnalysis nogc;
-
     /*
      * The embedding can register additional roots here.
      *
@@ -400,30 +392,44 @@ void js::gc::GCRuntime::traceRuntimeCommon(JSTracer* trc,
      * the nursery should be in the store buffer, and we want to avoid the
      * time taken to trace all these roots.
      */
-    for (size_t i = 0; i < blackRootTracers.ref().length(); i++) {
-      const Callback<JSTraceDataOp>& e = blackRootTracers.ref()[i];
-      (*e.op)(trc, e.data);
-    }
+    traceEmbeddingBlackRoots(trc);
 
     /* During GC, we don't trace gray roots at this stage. */
-    if (JSTraceDataOp op = grayRootTracer.op) {
-      if (traceOrMark == TraceRuntime) {
-        (*op)(trc, grayRootTracer.data);
-      }
+    if (traceOrMark == TraceRuntime) {
+      traceEmbeddingGrayRoots(trc);
     }
   }
 }
 
+void GCRuntime::traceEmbeddingBlackRoots(JSTracer* trc) {
+  // The analysis doesn't like the function pointer below.
+  JS::AutoSuppressGCAnalysis nogc;
+
+  for (size_t i = 0; i < blackRootTracers.ref().length(); i++) {
+    const Callback<JSTraceDataOp>& e = blackRootTracers.ref()[i];
+    (*e.op)(trc, e.data);
+  }
+}
+
+void GCRuntime::traceEmbeddingGrayRoots(JSTracer* trc) {
+  // The analysis doesn't like the function pointer below.
+  JS::AutoSuppressGCAnalysis nogc;
+
+  if (JSTraceDataOp op = grayRootTracer.op) {
+    (*op)(trc, grayRootTracer.data);
+  }
+}
+
 #ifdef DEBUG
 class AssertNoRootsTracer final : public JS::CallbackTracer {
   bool onChild(const JS::GCCellPtr& thing) override {
-    MOZ_CRASH("There should not be any roots after finishRoots");
+    MOZ_CRASH("There should not be any roots during runtime shutdown");
     return true;
   }
 
  public:
-  AssertNoRootsTracer(JSRuntime* rt, WeakMapTraceKind weakTraceKind)
-      : JS::CallbackTracer(rt, weakTraceKind) {}
+  explicit AssertNoRootsTracer(JSRuntime* rt)
+      : JS::CallbackTracer(rt, TraceWeakMapKeysValues) {}
 };
 #endif  // DEBUG
 
@@ -442,20 +448,18 @@ void js::gc::GCRuntime::finishRoots() {
     r->finishRoots();
   }
 
+  // Clear any remaining roots from the embedding (as otherwise they will be
+  // left dangling after we shut down) and remove the callbacks.
+  ClearEdgesTracer trc(rt);
+  traceEmbeddingBlackRoots(&trc);
+  traceEmbeddingGrayRoots(&trc);
+  clearBlackAndGrayRootTracers();
+}
+
+void js::gc::GCRuntime::checkNoRuntimeRoots(AutoGCSession& session) {
 #ifdef DEBUG
-  // The nsWrapperCache may not be empty before our shutdown GC, so we have
-  // to skip that table when verifying that we are fully unrooted.
-  auto prior = grayRootTracer;
-  grayRootTracer = Callback<JSTraceDataOp>(nullptr, nullptr);
-
-  AssertNoRootsTracer trc(rt, TraceWeakMapKeysValues);
-  AutoTraceSession session(rt);
-  gcstats::AutoPhase ap(rt->gc.stats(), gcstats::PhaseKind::TRACE_HEAP);
-  traceRuntime(&trc, session);
-
-  // Restore the wrapper tracing so that we leak instead of leaving dangling
-  // pointers.
-  grayRootTracer = prior;
+  AssertNoRootsTracer trc(rt);
+  traceRuntimeForMajorGC(&trc, session);
 #endif  // DEBUG
 }
 

js/src/gc/Tracer.cpp

@@ -51,10 +51,13 @@ JS_FOR_EACH_TRACEKIND(INSTANTIATE_ALL_VALID_TRACE_FUNCTIONS);
 
 template <typename T>
 bool DoCallback(JS::CallbackTracer* trc, T* thingp, const char* name) {
-  // return true as default. For some types the lambda below won't be called.
+  // Return true by default. For some types the lambda below won't be called.
   bool ret = true;
   auto thing = MapGCThingTyped(*thingp, [trc, name, &ret](auto t) {
-    ret = DoCallback(trc, &t, name);
+    if (!DoCallback(trc, &t, name)) {
+      ret = false;
+      return TaggedPtr<T>::empty();
+    }
     return TaggedPtr<T>::wrap(t);
   });
   // Only update *thingp if the value changed, to avoid TSan false positives for

js/src/jsfriendapi.cpp

@@ -1398,3 +1398,11 @@ JS_FRIEND_API JS::Value js::MaybeGetScriptPrivate(JSObject* object) {
 JS_FRIEND_API uint64_t js::GetGCHeapUsageForObjectZone(JSObject* obj) {
   return obj->zone()->zoneSize.gcBytes();
 }
+
+#ifdef DEBUG
+JS_FRIEND_API bool js::RuntimeIsBeingDestroyed() {
+  JSRuntime* runtime = TlsContext.get()->runtime();
+  MOZ_ASSERT(js::CurrentThreadCanAccessRuntime(runtime));
+  return runtime->isBeingDestroyed();
+}
+#endif

js/src/vm/Runtime.cpp

@@ -267,15 +267,15 @@ void JSRuntime::destroyRuntime() {
     CancelOffThreadParses(this);
     CancelOffThreadCompressions(this);
 
-    /* Remove persistent GC roots. */
-    gc.finishRoots();
-
     /*
      * Flag us as being destroyed. This allows the GC to free things like
      * interned atoms and Ion trampolines.
      */
     beingDestroyed_ = true;
 
+    /* Remove persistent GC roots. */
+    gc.finishRoots();
+
     /* Allow the GC to release scripts that were being profiled. */
     profilingScripts = false;
 

js/xpconnect/src/XPCJSRuntime.cpp

@@ -1132,13 +1132,12 @@ void DispatchOffThreadTask(RunnableTask* task) {
 }
 
 void XPCJSRuntime::Shutdown(JSContext* cx) {
-  // This destructor runs before ~CycleCollectedJSContext, which does the
-  // actual JS_DestroyContext() call. But destroying the context triggers
-  // one final GC, which can call back into the context with various
-  // callbacks if we aren't careful. Null out the relevant callbacks.
+  // This destructor runs before ~CycleCollectedJSContext, which does the actual
+  // JS_DestroyContext() call. But destroying the context triggers one final GC,
+  // which can call back into the context with various callbacks if we aren't
+  // careful. Remove the relevant callbacks, but leave the weak pointer
+  // callbacks to clear out any remaining table entries.
   JS_RemoveFinalizeCallback(cx, FinalizeCallback);
-  JS_RemoveWeakPointerZonesCallback(cx, WeakPointerZonesCallback);
-  JS_RemoveWeakPointerCompartmentCallback(cx, WeakPointerCompartmentCallback);
   xpc_DelocalizeRuntime(JS_GetRuntime(cx));
 
   JS::SetGCSliceCallback(cx, mPrevGCSliceCallback);
@@ -1147,11 +1146,8 @@ void XPCJSRuntime::Shutdown(JSContext* cx) {
   gHelperThreads->Shutdown();
   gHelperThreads = nullptr;
 
-  // clean up and destroy maps...
-  mWrappedJSMap->ShutdownMarker();
-  delete mWrappedJSMap;
-  mWrappedJSMap = nullptr;
-
+  // Clean up and destroy maps. Any remaining entries in mWrappedJSMap will be
+  // cleaned up by the weak pointer callbacks.
   delete mIID2NativeInterfaceMap;
   mIID2NativeInterfaceMap = nullptr;
 
@@ -1172,6 +1168,7 @@ void XPCJSRuntime::Shutdown(JSContext* cx) {
 
 XPCJSRuntime::~XPCJSRuntime() {
   MOZ_COUNT_DTOR_INHERITED(XPCJSRuntime, CycleCollectedJSRuntime);
+  delete mWrappedJSMap;
 }
 
 // If |*anonymizeID| is non-zero and this is a user realm, the name will

xpcom/base/CycleCollectedJSRuntime.cpp

@@ -535,12 +535,35 @@ CycleCollectedJSRuntime::CycleCollectedJSRuntime(JSContext* aCx)
 #endif  // MOZ_JS_DEV_ERROR_INTERCEPTOR
 }
 
+#ifdef NS_BUILD_REFCNT_LOGGING
+class JSLeakTracer : public JS::CallbackTracer {
+ public:
+  explicit JSLeakTracer(JSRuntime* aRuntime)
+      : JS::CallbackTracer(aRuntime, TraceWeakMapKeysValues) {}
+
+ private:
+  bool onChild(const JS::GCCellPtr& thing) override {
+    const char* kindName = JS::GCTraceKindToAscii(thing.kind());
+    size_t size = JS::GCTraceKindSize(thing.kind());
+    MOZ_LOG_CTOR(thing.asCell(), kindName, size);
+    return true;
+  }
+};
+#endif
+
 void CycleCollectedJSRuntime::Shutdown(JSContext* cx) {
 #ifdef MOZ_JS_DEV_ERROR_INTERCEPTOR
   mErrorInterceptor.Shutdown(mJSRuntime);
 #endif  // MOZ_JS_DEV_ERROR_INTERCEPTOR
-  JS_RemoveExtraGCRootsTracer(cx, TraceBlackJS, this);
-  JS_RemoveExtraGCRootsTracer(cx, TraceGrayJS, this);
+
+  // There should not be any roots left to trace at this point. Ensure any that
+  // remain are flagged as leaks.
+#ifdef NS_BUILD_REFCNT_LOGGING
+  JSLeakTracer tracer(Runtime());
+  TraceNativeBlackRoots(&tracer);
+  TraceNativeGrayRoots(&tracer);
+#endif
+
 #ifdef DEBUG
   mShutdownCalled = true;
 #endif
@@ -618,7 +641,7 @@ void CycleCollectedJSRuntime::DescribeGCThing(
       SprintfLiteral(name, "JS Object (%s)", clasp->name);
     }
   } else {
-    SprintfLiteral(name, "JS %s", JS::GCTraceKindToAscii(aThing.kind()));
+    SprintfLiteral(name, "%s", JS::GCTraceKindToAscii(aThing.kind()));
   }
 
   // Disable printing global for objects while we figure out ObjShrink fallout.