Bug 1478533 - Add a static_assert for WeakMap. r=jonco, mccr8
authorYoshi Huang <allstars.chh@mozilla.com>
Thu, 15 Nov 2018 10:11:10 +0100
changeset 503194 3d4dd037c4ceb41c227ef41270fefad15a8bb6f8
parent 503193 298abb6be2dce482ecb8af9ec6ef78abf2c26e1d
child 503195 4024ceceab4466ddb1792989245c51d9e52dee03
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjonco, mccr8
bugs1478533
milestone65.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 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
js/src/gc/WeakMap-inl.h
xpcom/base/CycleCollectedJSRuntime.cpp
xpcom/base/CycleCollectedJSRuntime.h
xpcom/base/nsCycleCollector.cpp
xpcom/base/nsCycleCollectorTraceJSHelpers.cpp
--- a/js/public/TraceKind.h
+++ b/js/public/TraceKind.h
@@ -63,16 +63,31 @@ enum class TraceKind
     Scope = 0x3F,
     RegExpShared = 0x4F,
 #ifdef ENABLE_BIGINT
     BigInt = 0x5F
 #endif
 };
 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")
 ASSERT_TRACE_KIND(JS::TraceKind::BaseShape);
 ASSERT_TRACE_KIND(JS::TraceKind::JitCode);
 ASSERT_TRACE_KIND(JS::TraceKind::LazyScript);
 ASSERT_TRACE_KIND(JS::TraceKind::Scope);
 ASSERT_TRACE_KIND(JS::TraceKind::RegExpShared);
@@ -84,17 +99,17 @@ ASSERT_TRACE_KIND(JS::TraceKind::RegExpS
 template <typename T>
 struct MapTypeToTraceKind {
     static const JS::TraceKind kind = T::TraceKind;
 };
 
 // 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) \
     D(Scope,         js::Scope,         true) \
     D(Object,        JSObject,          true) \
     D(ObjectGroup,   js::ObjectGroup,   true) \
     D(Script,        JSScript,          true) \
     D(Shape,         js::Shape,         true) \
--- a/js/src/gc/WeakMap-inl.h
+++ b/js/src/gc/WeakMap-inl.h
@@ -4,16 +4,17 @@
  * 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_WeakMap_inl_h
 #define gc_WeakMap_inl_h
 
 #include "gc/WeakMap.h"
 
+#include "js/TraceKind.h"
 #include "vm/JSContext.h"
 
 namespace js {
 
 template <typename T>
 static T extractUnbarriered(const WriteBarrieredBase<T>& v)
 {
     return v.get();
@@ -23,16 +24,23 @@ static T* extractUnbarriered(T* v)
 {
     return v;
 }
 
 template <class K, class V>
 WeakMap<K, V>::WeakMap(JSContext* cx, JSObject* memOf)
   : Base(cx->zone()), WeakMapBase(memOf, cx->zone())
 {
+    using ElemType = typename K::ElementType;
+    using NonPtrType = typename mozilla::RemovePointer<ElemType>::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());
 }
 
 // Trace a WeakMap entry based on 'markedCell' getting marked, where 'origKey'
 // is the key in the weakmap. These will probably be the same, but can be
 // different eg when markedCell is a delegate for origKey.
 //
--- a/xpcom/base/CycleCollectedJSRuntime.cpp
+++ b/xpcom/base/CycleCollectedJSRuntime.cpp
@@ -158,17 +158,17 @@ NoteWeakMapChildrenTracer::onChild(const
   if (aThing.is<JSString>()) {
     return;
   }
 
   if (!JS::GCThingIsMarkedGray(aThing) && !mCb.WantAllTraces()) {
     return;
   }
 
-  if (AddToCCKind(aThing.kind())) {
+  if (JS::IsCCTraceKind(aThing.kind())) {
     mCb.NoteWeakMapping(mMap, mKey, mKeyDelegate, aThing);
     mTracedAny = true;
   } else {
     JS::TraceChildren(this, aThing);
   }
 }
 
 struct NoteWeakMapsTracer : public js::WeakMapTracer
@@ -193,32 +193,32 @@ NoteWeakMapsTracer::trace(JSObject* aMap
       return;
     }
   }
 
   // The cycle collector can only properly reason about weak maps if it can
   // 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;
   }
 
   JSObject* kdelegate = nullptr;
   if (aKey.is<JSObject>()) {
     kdelegate = js::GetWeakmapKeyDelegate(&aKey.as<JSObject>());
   }
 
-  if (AddToCCKind(aValue.kind())) {
+  if (JS::IsCCTraceKind(aValue.kind())) {
     mCb.NoteWeakMapping(aMap, aKey, kdelegate, aValue);
   } else {
     mChildTracer.mTracedAny = false;
     mChildTracer.mMap = aMap;
     mChildTracer.mKey = aKey;
     mChildTracer.mKeyDelegate = kdelegate;
 
     if (!aValue.is<JSString>()) {
@@ -246,17 +246,17 @@ ShouldWeakMappingEntryBeBlack(JSObject* 
   // If nothing that could be held alive by this entry is marked gray, return.
   bool keyMightNeedMarking = aKey && JS::GCThingIsMarkedGray(aKey);
   bool valueMightNeedMarking = aValue && JS::GCThingIsMarkedGray(aValue) &&
     aValue.kind() != JS::TraceKind::String;
   if (!keyMightNeedMarking && !valueMightNeedMarking) {
     return;
   }
 
-  if (!AddToCCKind(aKey.kind())) {
+  if (!JS::IsCCTraceKind(aKey.kind())) {
     aKey = nullptr;
   }
 
   if (keyMightNeedMarking && aKey.is<JSObject>()) {
     JSObject* kdelegate = js::GetWeakmapKeyDelegate(&aKey.as<JSObject>());
     if (kdelegate && !JS::ObjectIsMarkedGray(kdelegate) &&
         (!aMap || !JS::ObjectIsMarkedGray(aMap)))
     {
@@ -352,17 +352,17 @@ CheckParticipatesInCycleCollection(JS::G
                                    void* aClosure)
 {
   bool* cycleCollectionEnabled = static_cast<bool*>(aClosure);
 
   if (*cycleCollectionEnabled) {
     return;
   }
 
-  if (AddToCCKind(aThing.kind()) && JS::GCThingIsMarkedGray(aThing)) {
+  if (JS::IsCCTraceKind(aThing.kind()) && JS::GCThingIsMarkedGray(aThing)) {
     *cycleCollectionEnabled = true;
   }
 }
 
 NS_IMETHODIMP
 JSGCThingParticipant::TraverseNative(void* aPtr,
                                      nsCycleCollectionTraversalCallback& aCb)
 {
@@ -416,22 +416,22 @@ TraversalTracer::onChild(const JS::GCCel
 
   // Don't traverse non-gray objects, unless we want all traces.
   if (!JS::GCThingIsMarkedGray(aThing) && !mCb.WantAllTraces()) {
     return;
   }
 
   /*
    * 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));
       mCb.NoteNextEdgeName(buffer);
     }
     mCb.NoteJSChild(aThing);
   } else if (aThing.is<js::Shape>()) {
     // The maximum depth of traversal when tracing a Shape is unbounded, due to
--- a/xpcom/base/CycleCollectedJSRuntime.h
+++ b/xpcom/base/CycleCollectedJSRuntime.h
@@ -12,16 +12,17 @@
 #include "mozilla/CycleCollectedJSContext.h"
 #include "mozilla/DeferredFinalize.h"
 #include "mozilla/LinkedList.h"
 #include "mozilla/mozalloc.h"
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/SegmentedVector.h"
 #include "jsapi.h"
 #include "jsfriendapi.h"
+#include "js/TraceKind.h"
 
 #include "nsCycleCollectionParticipant.h"
 #include "nsDataHashtable.h"
 #include "nsHashKeys.h"
 #include "nsTHashtable.h"
 
 class nsCycleCollectionNoteRootCallback;
 class nsIException;
@@ -407,26 +408,11 @@ private:
   ErrorInterceptor mErrorInterceptor;
 
 #endif // defined(NIGHTLY_BUILD)
 
 };
 
 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
--- a/xpcom/base/nsCycleCollector.cpp
+++ b/xpcom/base/nsCycleCollector.cpp
@@ -2057,24 +2057,24 @@ nsCycleCollector_createLogger()
 {
   nsCOMPtr<nsICycleCollectorListener> logger = new nsCycleCollectorLogger();
   return logger.forget();
 }
 
 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());
 }
 
 ////////////////////////////////////////////////////////////////////////
 // Bacon & Rajan's |MarkRoots| routine.
 ////////////////////////////////////////////////////////////////////////
 
 class CCGraphBuilder final : public nsCycleCollectionTraversalCallback,
--- a/xpcom/base/nsCycleCollectorTraceJSHelpers.cpp
+++ b/xpcom/base/nsCycleCollectorTraceJSHelpers.cpp
@@ -23,17 +23,17 @@ CycleCollectionNoteEdgeNameImpl(nsCycleC
 void
 nsCycleCollectionParticipant::NoteJSChild(JS::GCCellPtr aGCThing,
                                           const char* aName,
                                           void* aClosure)
 {
   nsCycleCollectionTraversalCallback* cb =
     static_cast<nsCycleCollectionTraversalCallback*>(aClosure);
   NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(*cb, aName);
-  if (mozilla::AddToCCKind(aGCThing.kind())) {
+  if (JS::IsCCTraceKind(aGCThing.kind())) {
     cb->NoteJSChild(aGCThing);
   }
 }
 
 void
 TraceCallbackFunc::Trace(JS::Heap<JS::Value>* aPtr, const char* aName,
                          void* aClosure) const
 {