Bug 1212624 - Make WeakMapBase be a LinkedListElement, r=Waldo

--HG--
extra : rebase_source : b4f30de9e25f14d8cdcd09f724d183f2524e59f7

js/src/gc/Marking.cpp

@@ -1775,7 +1775,7 @@ GCMarker::enterWeakMarkingMode()
         tag_ = TracerKindTag::WeakMarking;
 
         for (GCZoneGroupIter zone(runtime()); !zone.done(); zone.next()) {
-            for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) {
+            for (WeakMapBase* m : zone->gcWeakMapList) {
                 if (m->marked)
                     m->markEphemeronEntries(this);
             }

js/src/gc/Zone.cpp

@@ -27,7 +27,6 @@ JS::Zone::Zone(JSRuntime* rt)
     debuggers(nullptr),
     arenas(rt),
     types(this),
-    gcWeakMapList(nullptr),
     compartments(),
     gcGrayRoots(),
     gcMallocBytes(0),

js/src/gc/Zone.h

@@ -283,8 +283,8 @@ struct Zone : public JS::shadow::Zone,
 
     js::TypeZone types;
 
-    /* Linked list of live weakmaps in this zone. */
-    js::WeakMapBase* gcWeakMapList;
+    /* Live weakmaps in this zone. */
+    mozilla::LinkedList<js::WeakMapBase> gcWeakMapList;
 
     // The set of compartments in this zone.
     typedef js::Vector<JSCompartment*, 1, js::SystemAllocPolicy> CompartmentVector;

js/src/jsweakmap.cpp

@@ -25,7 +25,6 @@ using namespace js::gc;
 WeakMapBase::WeakMapBase(JSObject* memOf, Zone* zone)
   : memberOf(memOf),
     zone(zone),
-    next(WeakMapNotInList),
     marked(false)
 {
     MOZ_ASSERT_IF(memberOf, memberOf->compartment()->zone() == zone);
@@ -34,9 +33,6 @@ WeakMapBase::WeakMapBase(JSObject* memOf, Zone* zone)
 WeakMapBase::~WeakMapBase()
 {
     MOZ_ASSERT(CurrentThreadIsGCSweeping() || CurrentThreadIsHandlingInitFailure());
-    MOZ_ASSERT_IF(CurrentThreadIsGCSweeping(), !isInList());
-    if (isInList())
-        removeWeakMapFromList(this);
 }
 
 void
@@ -71,7 +67,7 @@ WeakMapBase::trace(JSTracer* tracer)
 void
 WeakMapBase::unmarkZone(JS::Zone* zone)
 {
-    for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next)
+    for (WeakMapBase* m : zone->gcWeakMapList)
         m->marked = false;
 }
 
@@ -79,7 +75,7 @@ void
 WeakMapBase::markAll(JS::Zone* zone, JSTracer* tracer)
 {
     MOZ_ASSERT(tracer->weakMapAction() != DoNotTraceWeakMaps);
-    for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) {
+    for (WeakMapBase* m : zone->gcWeakMapList) {
         m->trace(tracer);
         if (m->memberOf)
             TraceEdge(tracer, &m->memberOf, "memberOf");
@@ -90,7 +86,7 @@ bool
 WeakMapBase::markZoneIteratively(JS::Zone* zone, JSTracer* tracer)
 {
     bool markedAny = false;
-    for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) {
+    for (WeakMapBase* m : zone->gcWeakMapList) {
         if (m->marked && m->markIteratively(tracer))
             markedAny = true;
     }
@@ -100,7 +96,7 @@ WeakMapBase::markZoneIteratively(JS::Zone* zone, JSTracer* tracer)
 bool
 WeakMapBase::findInterZoneEdges(JS::Zone* zone)
 {
-    for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) {
+    for (WeakMapBase* m : zone->gcWeakMapList) {
         if (!m->findZoneEdges())
             return false;
     }
@@ -110,24 +106,20 @@ WeakMapBase::findInterZoneEdges(JS::Zone* zone)
 void
 WeakMapBase::sweepZone(JS::Zone* zone)
 {
-    WeakMapBase** tailPtr = &zone->gcWeakMapList;
-    for (WeakMapBase* m = zone->gcWeakMapList; m; ) {
-        WeakMapBase* next = m->next;
+    for (WeakMapBase* m = zone->gcWeakMapList.getFirst(); m; ) {
+        WeakMapBase* next = m->getNext();
         if (m->marked) {
             m->sweep();
-            *tailPtr = m;
-            tailPtr = &m->next;
         } else {
             /* Destroy the hash map now to catch any use after this point. */
             m->finish();
-            m->next = WeakMapNotInList;
+            m->removeFrom(zone->gcWeakMapList);
         }
         m = next;
     }
-    *tailPtr = nullptr;
 
 #ifdef DEBUG
-    for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next)
+    for (WeakMapBase* m : zone->gcWeakMapList)
         MOZ_ASSERT(m->isInList() && m->marked);
 #endif
 }
@@ -137,7 +129,7 @@ WeakMapBase::traceAllMappings(WeakMapTracer* tracer)
 {
     JSRuntime* rt = tracer->runtime;
     for (ZonesIter zone(rt, SkipAtoms); !zone.done(); zone.next()) {
-        for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) {
+        for (WeakMapBase* m : zone->gcWeakMapList) {
             // The WeakMapTracer callback is not allowed to GC.
             JS::AutoSuppressGCAnalysis nogc;
             m->traceMappings(tracer);
@@ -148,7 +140,7 @@ WeakMapBase::traceAllMappings(WeakMapTracer* tracer)
 bool
 WeakMapBase::saveZoneMarkedWeakMaps(JS::Zone* zone, WeakMapSet& markedWeakMaps)
 {
-    for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) {
+    for (WeakMapBase* m : zone->gcWeakMapList) {
         if (m->marked && !markedWeakMaps.put(m))
             return false;
     }
@@ -166,19 +158,6 @@ WeakMapBase::restoreMarkedWeakMaps(WeakMapSet& markedWeakMaps)
     }
 }
 
-void
-WeakMapBase::removeWeakMapFromList(WeakMapBase* weakmap)
-{
-    JS::Zone* zone = weakmap->zone;
-    for (WeakMapBase** p = &zone->gcWeakMapList; *p; p = &(*p)->next) {
-        if (*p == weakmap) {
-            *p = (*p)->next;
-            weakmap->next = WeakMapNotInList;
-            break;
-        }
-    }
-}
-
 bool
 ObjectValueMap::findZoneEdges()
 {
@@ -217,11 +196,6 @@ ObjectWeakMap::init()
     return map.init();
 }
 
-ObjectWeakMap::~ObjectWeakMap()
-{
-    WeakMapBase::removeWeakMapFromList(&map);
-}
-
 JSObject*
 ObjectWeakMap::lookup(const JSObject* obj)
 {

js/src/jsweakmap.h

@@ -8,6 +8,7 @@
 #define jsweakmap_h
 
 #include "mozilla/Move.h"
+#include "mozilla/LinkedList.h"
 
 #include "jscompartment.h"
 #include "jsfriendapi.h"
@@ -35,14 +36,12 @@ class WeakMapBase;
 // implementation takes care of the iterative marking needed for weak tables and removing
 // table entries when collection is complete.
 
-// The value for the next pointer for maps not in the map list.
-static WeakMapBase * const WeakMapNotInList = reinterpret_cast<WeakMapBase*>(1);
-
 typedef HashSet<WeakMapBase*, DefaultHasher<WeakMapBase*>, SystemAllocPolicy> WeakMapSet;
 
 // Common base class for all WeakMap specializations. The collector uses this to call
 // their markIteratively and sweep methods.
-class WeakMapBase {
+class WeakMapBase : public mozilla::LinkedListElement<WeakMapBase>
+{
     friend void js::GCMarker::enterWeakMarkingMode();
 
   public:
@@ -75,17 +74,12 @@ class WeakMapBase {
     // Trace all delayed weak map bindings. Used by the cycle collector.
     static void traceAllMappings(WeakMapTracer* tracer);
 
-    bool isInList() { return next != WeakMapNotInList; }
-
     // Save information about which weak maps are marked for a zone.
     static bool saveZoneMarkedWeakMaps(JS::Zone* zone, WeakMapSet& markedWeakMaps);
 
     // Restore information about which weak maps are marked for many zones.
     static void restoreMarkedWeakMaps(WeakMapSet& markedWeakMaps);
 
-    // Remove a weakmap from its zone's weakmaps list.
-    static void removeWeakMapFromList(WeakMapBase* weakmap);
-
     // Any weakmap key types that want to participate in the non-iterative
     // ephemeron marking must override this method.
     virtual void maybeMarkEntry(JSTracer* trc, gc::Cell* markedCell, JS::GCCellPtr l) = 0;
@@ -109,11 +103,6 @@ class WeakMapBase {
     // Zone containing this weak map.
     JS::Zone* zone;
 
-    // Link in a list of all WeakMaps in a Zone, headed by
-    // JS::Zone::gcWeakMapList. The last element of the list has nullptr as its
-    // next. Maps not in the list have WeakMapNotInList as their next.
-    WeakMapBase* next;
-
     // Whether this object has been traced during garbage collection.
     bool marked;
 };
@@ -147,8 +136,7 @@ class WeakMap : public HashMap<Key, Value, HashPolicy, RuntimeAllocPolicy>, publ
     bool init(uint32_t len = 16) {
         if (!Base::init(len))
             return false;
-        next = zone->gcWeakMapList;
-        zone->gcWeakMapList = this;
+        zone->gcWeakMapList.insertFront(this);
         marked = JS::IsIncrementalGCInProgress(zone->runtimeFromMainThread());
         return true;
     }
@@ -177,6 +165,9 @@ class WeakMap : public HashMap<Key, Value, HashPolicy, RuntimeAllocPolicy>, publ
         return p;
     }
 
+    // Resolve ambiguity with LinkedListElement<>::remove.
+    using Base::remove;
+
     // The WeakMap and some part of the key are marked. If the entry is marked
     // according to the exact semantics of this WeakMap, then mark the value.
     // (For a standard WeakMap, the entry is marked if either the key its
@@ -418,7 +409,6 @@ class ObjectWeakMap
   public:
     explicit ObjectWeakMap(JSContext* cx);
     bool init();
-    ~ObjectWeakMap();
 
     JSObject* lookup(const JSObject* obj);
     bool add(JSContext* cx, JSObject* obj, JSObject* target);

js/src/vm/WeakMapPtr.cpp

@@ -53,12 +53,7 @@ void
 JS::WeakMapPtr<K, V>::destroy()
 {
     MOZ_ASSERT(initialized());
-    auto map = Utils<K, V>::cast(ptr);
-    // If this destruction happens mid-GC, we might be in the compartment's list
-    // of known live weakmaps. If we are, remove ourselves before deleting.
-    if (map->isInList())
-        WeakMapBase::removeWeakMapFromList(map);
-    js_delete(map);
+    js_delete(Utils<K, V>::cast(ptr));
     ptr = nullptr;
 }