From 8b73371be015a1f04beb06b230efe4930c2959e6 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Tue, 13 Aug 2019 08:31:49 +0000 Subject: [PATCH] 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 | 11 +++- js/public/TracingAPI.h | 12 +++++ js/src/gc/ClearEdgesTracer.h | 44 ++++++++++++++++ js/src/gc/DeletePolicy.h | 30 +---------- js/src/gc/GC.cpp | 31 ++++++++++-- js/src/gc/GCRuntime.h | 4 ++ js/src/gc/Marking-inl.h | 3 ++ js/src/gc/RootMarking.cpp | 70 ++++++++++++++------------ js/src/gc/Tracer.cpp | 7 ++- js/src/jsfriendapi.cpp | 8 +++ js/src/vm/Runtime.cpp | 6 +-- js/xpconnect/src/XPCJSRuntime.cpp | 19 +++---- xpcom/base/CycleCollectedJSRuntime.cpp | 29 +++++++++-- 13 files changed, 188 insertions(+), 86 deletions(-) create mode 100644 js/src/gc/ClearEdgesTracer.h diff --git a/dom/base/nsWrapperCache.h b/dom/base/nsWrapperCache.h index 2855b57a9e7e..4e85a97f4f4e 100644 --- a/dom/base/nsWrapperCache.h +++ b/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); } diff --git a/js/public/TracingAPI.h b/js/public/TracingAPI.h index 0abf0363cfc3..1fd9039c3d8a 100644 --- a/js/public/TracingAPI.h +++ b/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 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 */ diff --git a/js/src/gc/ClearEdgesTracer.h b/js/src/gc/ClearEdgesTracer.h new file mode 100644 index 000000000000..b776e95536aa --- /dev/null +++ b/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 + 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 diff --git a/js/src/gc/DeletePolicy.h b/js/src/gc/DeletePolicy.h index e12fa53c9418..f1db0150dbcd 100644 --- a/js/src/gc/DeletePolicy.h +++ b/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 - 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 diff --git a/js/src/gc/GC.cpp b/js/src/gc/GC.cpp index 6643c0c9ef56..0dcacdd0bc67 100644 --- a/js/src/gc/GC.cpp +++ b/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 inline bool js::gc::ClearEdgesTracer::clearEdge(S** thingp) { diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h index 73112caf3854..7ed9466de4ad 100644 --- a/js/src/gc/GCRuntime.h +++ b/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, diff --git a/js/src/gc/Marking-inl.h b/js/src/gc/Marking-inl.h index 5c24648d47a8..f9ea370a9271 100644 --- a/js/src/gc/Marking-inl.h +++ b/js/src/gc/Marking-inl.h @@ -37,6 +37,7 @@ struct TaggedPtr { "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 { 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 { static TaggedProto wrap(JSObject* obj) { return TaggedProto(obj); } + static TaggedProto empty() { return TaggedProto(); } }; template diff --git a/js/src/gc/RootMarking.cpp b/js/src/gc/RootMarking.cpp index 022df23a0b32..34f2ca30a885 100644 --- a/js/src/gc/RootMarking.cpp +++ b/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& 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& 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(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 } diff --git a/js/src/gc/Tracer.cpp b/js/src/gc/Tracer.cpp index 7a32e14ab9af..f0624d5c025f 100644 --- a/js/src/gc/Tracer.cpp +++ b/js/src/gc/Tracer.cpp @@ -51,10 +51,13 @@ JS_FOR_EACH_TRACEKIND(INSTANTIATE_ALL_VALID_TRACE_FUNCTIONS); template 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::empty(); + } return TaggedPtr::wrap(t); }); // Only update *thingp if the value changed, to avoid TSan false positives for diff --git a/js/src/jsfriendapi.cpp b/js/src/jsfriendapi.cpp index 7e3b377efd7a..e582f95c4c25 100644 --- a/js/src/jsfriendapi.cpp +++ b/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 diff --git a/js/src/vm/Runtime.cpp b/js/src/vm/Runtime.cpp index dd0819323aa3..10991f290c50 100644 --- a/js/src/vm/Runtime.cpp +++ b/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; diff --git a/js/xpconnect/src/XPCJSRuntime.cpp b/js/xpconnect/src/XPCJSRuntime.cpp index 46c88f49e867..8a267ddc15f5 100644 --- a/js/xpconnect/src/XPCJSRuntime.cpp +++ b/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 diff --git a/xpcom/base/CycleCollectedJSRuntime.cpp b/xpcom/base/CycleCollectedJSRuntime.cpp index cb269ae1702d..6d012e49b4cb 100644 --- a/xpcom/base/CycleCollectedJSRuntime.cpp +++ b/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.