From f2e1d9efb31c80b0fc71601da836e68ecfdad47a Mon Sep 17 00:00:00 2001 From: Yoshi Huang Date: Thu, 15 Nov 2018 10:11:10 +0100 Subject: [PATCH] Bug 1478533 - Add a static_assert for WeakMap. r=jonco, mccr8 If an object is used as a WeakMap key, its TraceKind should be added into CC graph, so adding a static_assert to make sure this. --- js/public/TraceKind.h | 17 ++++++++++++++++- js/src/gc/WeakMap-inl.h | 8 ++++++++ xpcom/base/CycleCollectedJSRuntime.cpp | 18 +++++++++--------- xpcom/base/CycleCollectedJSRuntime.h | 16 +--------------- xpcom/base/nsCycleCollector.cpp | 4 ++-- xpcom/base/nsCycleCollectorTraceJSHelpers.cpp | 2 +- 6 files changed, 37 insertions(+), 28 deletions(-) diff --git a/js/public/TraceKind.h b/js/public/TraceKind.h index be3f6b36b9ca..39da4226fb7b 100644 --- a/js/public/TraceKind.h +++ b/js/public/TraceKind.h @@ -68,6 +68,21 @@ enum class TraceKind }; const static uintptr_t OutOfLineTraceKindMask = 0x07; +// Returns true if the JS::TraceKind is one the cycle collector cares about. +// Everything used as WeakMap key should be listed here, to represent the key +// in cycle collector's graph, otherwise the key is considered to be pointed +// from somewhere unknown, and results in leaking the subgraph which contains +// the key. +// See the comments in NoteWeakMapsTracer::trace for more details. +inline constexpr bool IsCCTraceKind(JS::TraceKind aKind) +{ + return aKind == JS::TraceKind::Object || + aKind == JS::TraceKind::Script || + aKind == JS::TraceKind::LazyScript || + aKind == JS::TraceKind::Scope || + aKind == JS::TraceKind::RegExpShared; +} + #define ASSERT_TRACE_KIND(tk) \ static_assert((uintptr_t(tk) & OutOfLineTraceKindMask) == OutOfLineTraceKindMask, \ "mask bits are set") @@ -89,7 +104,7 @@ struct MapTypeToTraceKind { // When this header is used outside SpiderMonkey, the class definitions are not // available, so the following table containing all public GC types is used. #define JS_FOR_EACH_TRACEKIND(D) \ - /* PrettyName TypeName AddToCCKind */ \ + /* PrettyName TypeName IsCCTraceKind */ \ D(BaseShape, js::BaseShape, true) \ D(JitCode, js::jit::JitCode, true) \ D(LazyScript, js::LazyScript, true) \ diff --git a/js/src/gc/WeakMap-inl.h b/js/src/gc/WeakMap-inl.h index d4a517037239..14f8c2dce8b0 100644 --- a/js/src/gc/WeakMap-inl.h +++ b/js/src/gc/WeakMap-inl.h @@ -9,6 +9,7 @@ #include "gc/WeakMap.h" +#include "js/TraceKind.h" #include "vm/JSContext.h" namespace js { @@ -28,6 +29,13 @@ template WeakMap::WeakMap(JSContext* cx, JSObject* memOf) : Base(cx->zone()), WeakMapBase(memOf, cx->zone()) { + using ElemType = typename K::ElementType; + using NonPtrType = typename mozilla::RemovePointer::Type; + // The object's TraceKind needs to be added to CC graph if this object is + // used as a WeakMap key. See the comments for IsCCTraceKind for details. + static_assert(JS::IsCCTraceKind(NonPtrType::TraceKind), + "Object's TraceKind should be added to CC graph."); + zone()->gcWeakMapList().insertFront(this); marked = JS::IsIncrementalGCInProgress(TlsContext.get()); } diff --git a/xpcom/base/CycleCollectedJSRuntime.cpp b/xpcom/base/CycleCollectedJSRuntime.cpp index 11e4b9a0da95..6f4d779d2ae4 100644 --- a/xpcom/base/CycleCollectedJSRuntime.cpp +++ b/xpcom/base/CycleCollectedJSRuntime.cpp @@ -163,7 +163,7 @@ NoteWeakMapChildrenTracer::onChild(const JS::GCCellPtr& aThing) return; } - if (AddToCCKind(aThing.kind())) { + if (JS::IsCCTraceKind(aThing.kind())) { mCb.NoteWeakMapping(mMap, mKey, mKeyDelegate, aThing); mTracedAny = true; } else { @@ -198,13 +198,13 @@ NoteWeakMapsTracer::trace(JSObject* aMap, JS::GCCellPtr aKey, // reason about the liveness of their keys, which in turn requires that // the key can be represented in the cycle collector graph. All existing // uses of weak maps use either objects or scripts as keys, which are okay. - MOZ_ASSERT(AddToCCKind(aKey.kind())); + MOZ_ASSERT(JS::IsCCTraceKind(aKey.kind())); // As an emergency fallback for non-debug builds, if the key is not // representable in the cycle collector graph, we treat it as marked. This // can cause leaks, but is preferable to ignoring the binding, which could // cause the cycle collector to free live objects. - if (!AddToCCKind(aKey.kind())) { + if (!JS::IsCCTraceKind(aKey.kind())) { aKey = nullptr; } @@ -213,7 +213,7 @@ NoteWeakMapsTracer::trace(JSObject* aMap, JS::GCCellPtr aKey, kdelegate = js::GetWeakmapKeyDelegate(&aKey.as()); } - if (AddToCCKind(aValue.kind())) { + if (JS::IsCCTraceKind(aValue.kind())) { mCb.NoteWeakMapping(aMap, aKey, kdelegate, aValue); } else { mChildTracer.mTracedAny = false; @@ -251,7 +251,7 @@ ShouldWeakMappingEntryBeBlack(JSObject* aMap, JS::GCCellPtr aKey, JS::GCCellPtr return; } - if (!AddToCCKind(aKey.kind())) { + if (!JS::IsCCTraceKind(aKey.kind())) { aKey = nullptr; } @@ -357,7 +357,7 @@ CheckParticipatesInCycleCollection(JS::GCCellPtr aThing, const char* aName, return; } - if (AddToCCKind(aThing.kind()) && JS::GCThingIsMarkedGray(aThing)) { + if (JS::IsCCTraceKind(aThing.kind()) && JS::GCThingIsMarkedGray(aThing)) { *cycleCollectionEnabled = true; } } @@ -421,12 +421,12 @@ TraversalTracer::onChild(const JS::GCCellPtr& aThing) /* * This function needs to be careful to avoid stack overflow. Normally, when - * AddToCCKind is true, the recursion terminates immediately as we just add + * IsCCTraceKind is true, the recursion terminates immediately as we just add * |thing| to the CC graph. So overflow is only possible when there are long - * or cyclic chains of non-AddToCCKind GC things. Places where this can occur + * or cyclic chains of non-IsCCTraceKind GC things. Places where this can occur * use special APIs to handle such chains iteratively. */ - if (AddToCCKind(aThing.kind())) { + if (JS::IsCCTraceKind(aThing.kind())) { if (MOZ_UNLIKELY(mCb.WantDebugInfo())) { char buffer[200]; getTracingEdgeName(buffer, sizeof(buffer)); diff --git a/xpcom/base/CycleCollectedJSRuntime.h b/xpcom/base/CycleCollectedJSRuntime.h index d48c03d8be78..186523794e43 100644 --- a/xpcom/base/CycleCollectedJSRuntime.h +++ b/xpcom/base/CycleCollectedJSRuntime.h @@ -17,6 +17,7 @@ #include "mozilla/SegmentedVector.h" #include "jsapi.h" #include "jsfriendapi.h" +#include "js/TraceKind.h" #include "nsCycleCollectionParticipant.h" #include "nsDataHashtable.h" @@ -412,21 +413,6 @@ private: void TraceScriptHolder(nsISupports* aHolder, JSTracer* aTracer); -// Returns true if the JS::TraceKind is one the cycle collector cares about. -// Everything used as WeakMap key should be listed here, to represent the key -// in cycle collector's graph, otherwise the key is considered to be pointed -// from somewhere unknown, and results in leaking the subgraph which contains -// the key. -// See the comments in NoteWeakMapsTracer::trace for more details. -inline bool AddToCCKind(JS::TraceKind aKind) -{ - return aKind == JS::TraceKind::Object || - aKind == JS::TraceKind::Script || - aKind == JS::TraceKind::LazyScript || - aKind == JS::TraceKind::Scope || - aKind == JS::TraceKind::RegExpShared; -} - } // namespace mozilla #endif // mozilla_CycleCollectedJSRuntime_h diff --git a/xpcom/base/nsCycleCollector.cpp b/xpcom/base/nsCycleCollector.cpp index d50175c5cc25..3b82839857e9 100644 --- a/xpcom/base/nsCycleCollector.cpp +++ b/xpcom/base/nsCycleCollector.cpp @@ -2062,14 +2062,14 @@ nsCycleCollector_createLogger() static bool GCThingIsGrayCCThing(JS::GCCellPtr thing) { - return AddToCCKind(thing.kind()) && + return JS::IsCCTraceKind(thing.kind()) && JS::GCThingIsMarkedGray(thing); } static bool ValueIsGrayCCThing(const JS::Value& value) { - return AddToCCKind(value.traceKind()) && + return JS::IsCCTraceKind(value.traceKind()) && JS::GCThingIsMarkedGray(value.toGCCellPtr()); } diff --git a/xpcom/base/nsCycleCollectorTraceJSHelpers.cpp b/xpcom/base/nsCycleCollectorTraceJSHelpers.cpp index f65a92e6171c..4ec2f3469927 100644 --- a/xpcom/base/nsCycleCollectorTraceJSHelpers.cpp +++ b/xpcom/base/nsCycleCollectorTraceJSHelpers.cpp @@ -28,7 +28,7 @@ nsCycleCollectionParticipant::NoteJSChild(JS::GCCellPtr aGCThing, nsCycleCollectionTraversalCallback* cb = static_cast(aClosure); NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(*cb, aName); - if (mozilla::AddToCCKind(aGCThing.kind())) { + if (JS::IsCCTraceKind(aGCThing.kind())) { cb->NoteJSChild(aGCThing); } }