From 70a6b1e10f66a233e155e122d4ea1e097a22d646 Mon Sep 17 00:00:00 2001 From: Steve Fink Date: Wed, 14 Oct 2015 16:49:12 -0700 Subject: [PATCH] Backed out 5 changesets (bug 1212624) for breaking stuff. Backed out changeset cf5ffa45a4a2 Backed out changeset 0d7a968d2d64 (bug 1212624) Backed out changeset 379edefa8e47 (bug 1212624) Backed out changeset f73fca35daad (bug 1212624) Backed out changeset 4f499d30a0e0 (bug 1212624) --- js/src/gc/Marking.cpp | 2 +- js/src/gc/RootMarking.cpp | 2 +- js/src/gc/Zone.cpp | 1 + js/src/gc/Zone.h | 4 +- js/src/jsweakmap.cpp | 46 ++++++++--- js/src/jsweakmap.h | 24 ++++-- js/src/vm/Debugger.cpp | 13 +-- js/src/vm/Debugger.h | 11 ++- js/src/vm/UnboxedObject.cpp | 5 +- js/src/vm/WeakMapPtr.cpp | 7 +- mfbt/LinkedList.h | 32 -------- mfbt/tests/TestLinkedList.cpp | 147 ---------------------------------- mfbt/tests/moz.build | 1 - 13 files changed, 86 insertions(+), 209 deletions(-) delete mode 100644 mfbt/tests/TestLinkedList.cpp diff --git a/js/src/gc/Marking.cpp b/js/src/gc/Marking.cpp index cd00dc16457..59048cf5481 100644 --- a/js/src/gc/Marking.cpp +++ b/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) { + for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) { if (m->marked) m->markEphemeronEntries(this); } diff --git a/js/src/gc/RootMarking.cpp b/js/src/gc/RootMarking.cpp index a90250cc6d3..67b0986323c 100644 --- a/js/src/gc/RootMarking.cpp +++ b/js/src/gc/RootMarking.cpp @@ -220,7 +220,7 @@ struct PersistentRootedMarker static void markChain(JSTracer* trc, List& list, const char* name) { - for (Element* r : list) + for (Element* r = list.getFirst(); r; r = r->getNext()) TraceFn(trc, r->address(), name); } }; diff --git a/js/src/gc/Zone.cpp b/js/src/gc/Zone.cpp index 87d23451656..5821ed7ce7f 100644 --- a/js/src/gc/Zone.cpp +++ b/js/src/gc/Zone.cpp @@ -27,6 +27,7 @@ JS::Zone::Zone(JSRuntime* rt) debuggers(nullptr), arenas(rt), types(this), + gcWeakMapList(nullptr), compartments(), gcGrayRoots(), gcMallocBytes(0), diff --git a/js/src/gc/Zone.h b/js/src/gc/Zone.h index 2430cd057a0..87f99dd53e6 100644 --- a/js/src/gc/Zone.h +++ b/js/src/gc/Zone.h @@ -283,8 +283,8 @@ struct Zone : public JS::shadow::Zone, js::TypeZone types; - /* Live weakmaps in this zone. */ - mozilla::LinkedList gcWeakMapList; + /* Linked list of live weakmaps in this zone. */ + js::WeakMapBase* gcWeakMapList; // The set of compartments in this zone. typedef js::Vector CompartmentVector; diff --git a/js/src/jsweakmap.cpp b/js/src/jsweakmap.cpp index fd42fc64e91..9aed71a0913 100644 --- a/js/src/jsweakmap.cpp +++ b/js/src/jsweakmap.cpp @@ -25,6 +25,7 @@ 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); @@ -33,6 +34,9 @@ WeakMapBase::WeakMapBase(JSObject* memOf, Zone* zone) WeakMapBase::~WeakMapBase() { MOZ_ASSERT(CurrentThreadIsGCSweeping() || CurrentThreadIsHandlingInitFailure()); + MOZ_ASSERT_IF(CurrentThreadIsGCSweeping(), !isInList()); + if (isInList()) + removeWeakMapFromList(this); } void @@ -67,7 +71,7 @@ WeakMapBase::trace(JSTracer* tracer) void WeakMapBase::unmarkZone(JS::Zone* zone) { - for (WeakMapBase* m : zone->gcWeakMapList) + for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) m->marked = false; } @@ -75,7 +79,7 @@ void WeakMapBase::markAll(JS::Zone* zone, JSTracer* tracer) { MOZ_ASSERT(tracer->weakMapAction() != DoNotTraceWeakMaps); - for (WeakMapBase* m : zone->gcWeakMapList) { + for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) { m->trace(tracer); if (m->memberOf) TraceEdge(tracer, &m->memberOf, "memberOf"); @@ -86,7 +90,7 @@ bool WeakMapBase::markZoneIteratively(JS::Zone* zone, JSTracer* tracer) { bool markedAny = false; - for (WeakMapBase* m : zone->gcWeakMapList) { + for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) { if (m->marked && m->markIteratively(tracer)) markedAny = true; } @@ -96,7 +100,7 @@ WeakMapBase::markZoneIteratively(JS::Zone* zone, JSTracer* tracer) bool WeakMapBase::findInterZoneEdges(JS::Zone* zone) { - for (WeakMapBase* m : zone->gcWeakMapList) { + for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) { if (!m->findZoneEdges()) return false; } @@ -106,20 +110,24 @@ WeakMapBase::findInterZoneEdges(JS::Zone* zone) void WeakMapBase::sweepZone(JS::Zone* zone) { - for (WeakMapBase* m = zone->gcWeakMapList.getFirst(); m; ) { - WeakMapBase* next = m->getNext(); + WeakMapBase** tailPtr = &zone->gcWeakMapList; + for (WeakMapBase* m = zone->gcWeakMapList; m; ) { + WeakMapBase* next = m->next; 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->removeFrom(zone->gcWeakMapList); + m->next = WeakMapNotInList; } m = next; } + *tailPtr = nullptr; #ifdef DEBUG - for (WeakMapBase* m : zone->gcWeakMapList) + for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) MOZ_ASSERT(m->isInList() && m->marked); #endif } @@ -129,7 +137,7 @@ WeakMapBase::traceAllMappings(WeakMapTracer* tracer) { JSRuntime* rt = tracer->runtime; for (ZonesIter zone(rt, SkipAtoms); !zone.done(); zone.next()) { - for (WeakMapBase* m : zone->gcWeakMapList) { + for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) { // The WeakMapTracer callback is not allowed to GC. JS::AutoSuppressGCAnalysis nogc; m->traceMappings(tracer); @@ -140,7 +148,7 @@ WeakMapBase::traceAllMappings(WeakMapTracer* tracer) bool WeakMapBase::saveZoneMarkedWeakMaps(JS::Zone* zone, WeakMapSet& markedWeakMaps) { - for (WeakMapBase* m : zone->gcWeakMapList) { + for (WeakMapBase* m = zone->gcWeakMapList; m; m = m->next) { if (m->marked && !markedWeakMaps.put(m)) return false; } @@ -158,6 +166,19 @@ 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() { @@ -196,6 +217,11 @@ ObjectWeakMap::init() return map.init(); } +ObjectWeakMap::~ObjectWeakMap() +{ + WeakMapBase::removeWeakMapFromList(&map); +} + JSObject* ObjectWeakMap::lookup(const JSObject* obj) { diff --git a/js/src/jsweakmap.h b/js/src/jsweakmap.h index fa33e3cca1b..56ce873f348 100644 --- a/js/src/jsweakmap.h +++ b/js/src/jsweakmap.h @@ -7,7 +7,6 @@ #ifndef jsweakmap_h #define jsweakmap_h -#include "mozilla/LinkedList.h" #include "mozilla/Move.h" #include "jscompartment.h" @@ -36,12 +35,14 @@ 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(1); + typedef HashSet, SystemAllocPolicy> WeakMapSet; // Common base class for all WeakMap specializations. The collector uses this to call // their markIteratively and sweep methods. -class WeakMapBase : public mozilla::LinkedListElement -{ +class WeakMapBase { friend void js::GCMarker::enterWeakMarkingMode(); public: @@ -74,12 +75,17 @@ class WeakMapBase : public mozilla::LinkedListElement // 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; @@ -103,6 +109,11 @@ class WeakMapBase : public mozilla::LinkedListElement // 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; }; @@ -136,7 +147,8 @@ class WeakMap : public HashMap, publ bool init(uint32_t len = 16) { if (!Base::init(len)) return false; - zone->gcWeakMapList.insertFront(this); + next = zone->gcWeakMapList; + zone->gcWeakMapList = this; marked = JS::IsIncrementalGCInProgress(zone->runtimeFromMainThread()); return true; } @@ -165,9 +177,6 @@ class WeakMap : public HashMap, 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 @@ -409,6 +418,7 @@ class ObjectWeakMap public: explicit ObjectWeakMap(JSContext* cx); bool init(); + ~ObjectWeakMap(); JSObject* lookup(const JSObject* obj); bool add(JSContext* cx, JSObject* obj, JSObject* target); diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp index 405d66a1f65..d74fffac25f 100644 --- a/js/src/vm/Debugger.cpp +++ b/js/src/vm/Debugger.cpp @@ -2439,7 +2439,7 @@ Debugger::markIncomingCrossCompartmentEdges(JSTracer* trc) gc::State state = rt->gc.state(); MOZ_ASSERT(state == gc::MARK_ROOTS || state == gc::COMPACT); - for (Debugger* dbg : rt->debuggerList) { + for (Debugger* dbg = rt->debuggerList.getFirst(); dbg; dbg = dbg->getNext()) { Zone* zone = dbg->object->zone(); if ((state == gc::MARK_ROOTS && !zone->isCollecting()) || (state == gc::COMPACT && !zone->isGCCompacting())) @@ -2535,7 +2535,7 @@ Debugger::markAllIteratively(GCMarker* trc) Debugger::markAll(JSTracer* trc) { JSRuntime* rt = trc->runtime(); - for (Debugger* dbg : rt->debuggerList) { + for (Debugger* dbg = rt->debuggerList.getFirst(); dbg; dbg = dbg->getNext()) { WeakGlobalObjectSet& debuggees = dbg->debuggees; for (WeakGlobalObjectSet::Enum e(debuggees); !e.empty(); e.popFront()) { GlobalObject* global = e.front(); @@ -2607,7 +2607,7 @@ Debugger::sweepAll(FreeOp* fop) { JSRuntime* rt = fop->runtime(); - for (Debugger* dbg : rt->debuggerList) { + for (Debugger* dbg = rt->debuggerList.getFirst(); dbg; dbg = dbg->getNext()) { if (IsAboutToBeFinalized(&dbg->object)) { /* * dbg is being GC'd. Detach it from its debuggees. The debuggee @@ -2638,7 +2638,10 @@ Debugger::findZoneEdges(Zone* zone, js::gc::ComponentFinder& finder) * This ensure that debuggers and their debuggees are finalized in the same * group. */ - for (Debugger* dbg : zone->runtimeFromMainThread()->debuggerList) { + for (Debugger* dbg = zone->runtimeFromMainThread()->debuggerList.getFirst(); + dbg; + dbg = dbg->getNext()) + { Zone* w = dbg->object->zone(); if (w == zone || !w->isGCMarking()) continue; @@ -8476,7 +8479,7 @@ FireOnGarbageCollectionHook(JSContext* cx, JS::dbg::GarbageCollectionEvent::Ptr& // participated in this GC. AutoCheckCannotGC noGC; - for (Debugger* dbg : cx->runtime()->debuggerList) { + for (Debugger* dbg = cx->runtime()->debuggerList.getFirst(); dbg; dbg = dbg->getNext()) { if (dbg->enabled && dbg->observedGC(data->majorGCNumber()) && dbg->getHook(Debugger::OnGarbageCollection)) diff --git a/js/src/vm/Debugger.h b/js/src/vm/Debugger.h index 6e589c970bf..14ca32c4e27 100644 --- a/js/src/vm/Debugger.h +++ b/js/src/vm/Debugger.h @@ -93,6 +93,16 @@ class DebuggerWeakMap : private WeakMap, Relocatabl compartment(cx->compartment()) { } + ~DebuggerWeakMap() { + // If our owning Debugger fails construction after already initializing + // this DebuggerWeakMap, we need to make sure that we aren't in the + // compartment's weak map list anymore. Normally, when we are destroyed + // because the GC finds us unreachable, the GC takes care of removing us + // from this list. + if (WeakMapBase::isInList()) + WeakMapBase::removeWeakMapFromList(this); + } + public: /* Expose those parts of HashMap public interface that are used by Debugger methods. */ @@ -200,7 +210,6 @@ class Debugger : private mozilla::LinkedListElement friend class DebuggerMemory; friend class SavedStacks; friend class mozilla::LinkedListElement; - friend class mozilla::LinkedList; friend bool (::JS_DefineDebuggerObject)(JSContext* cx, JS::HandleObject obj); friend bool (::JS::dbg::IsDebugger)(JSObject&); friend bool (::JS::dbg::GetDebuggeeGlobals)(JSContext*, JSObject&, AutoObjectVector&); diff --git a/js/src/vm/UnboxedObject.cpp b/js/src/vm/UnboxedObject.cpp index 38e21130521..6e22a9c5694 100644 --- a/js/src/vm/UnboxedObject.cpp +++ b/js/src/vm/UnboxedObject.cpp @@ -1780,7 +1780,10 @@ ComputePlainObjectLayout(ExclusiveContext* cx, Shape* templateShape, // properties, which will allow us to generate better code if the objects // have a subtype/supertype relation and are accessed at common sites. UnboxedLayout* bestExisting = nullptr; - for (UnboxedLayout* existing : cx->compartment()->unboxedLayouts) { + for (UnboxedLayout* existing = cx->compartment()->unboxedLayouts.getFirst(); + existing; + existing = existing->getNext()) + { if (PropertiesAreSuperset(properties, existing)) { if (!bestExisting || existing->properties().length() > bestExisting->properties().length()) diff --git a/js/src/vm/WeakMapPtr.cpp b/js/src/vm/WeakMapPtr.cpp index c998d996100..6bd62412943 100644 --- a/js/src/vm/WeakMapPtr.cpp +++ b/js/src/vm/WeakMapPtr.cpp @@ -53,7 +53,12 @@ void JS::WeakMapPtr::destroy() { MOZ_ASSERT(initialized()); - js_delete(Utils::cast(ptr)); + auto map = Utils::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); ptr = nullptr; } diff --git a/mfbt/LinkedList.h b/mfbt/LinkedList.h index 6eb4c63d570..b78dbcebcd7 100644 --- a/mfbt/LinkedList.h +++ b/mfbt/LinkedList.h @@ -299,26 +299,6 @@ private: LinkedListElement sentinel; public: - class Iterator { - T* mCurrent; - - public: - explicit Iterator(T* aCurrent) : mCurrent(aCurrent) {} - - T* operator *() const { - return mCurrent; - } - - const Iterator& operator++() { - mCurrent = mCurrent->getNext(); - return *this; - } - - bool operator!=(Iterator& aOther) const { - return mCurrent != aOther.mCurrent; - } - }; - LinkedList() : sentinel(LinkedListElement::NODE_KIND_SENTINEL) { } LinkedList(LinkedList&& aOther) @@ -403,18 +383,6 @@ public: } } - /* - * Allow range-based iteration: - * - * for (MyElementType* elt : myList) { ... } - */ - Iterator begin() { - return Iterator(getFirst()); - } - Iterator end() { - return Iterator(nullptr); - } - /* * Measures the memory consumption of the list excluding |this|. Note that * it only measures the list elements themselves. If the list elements diff --git a/mfbt/tests/TestLinkedList.cpp b/mfbt/tests/TestLinkedList.cpp deleted file mode 100644 index a0e2515bb2e..00000000000 --- a/mfbt/tests/TestLinkedList.cpp +++ /dev/null @@ -1,147 +0,0 @@ -/* -*- 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/. */ - -#include "mozilla/Assertions.h" -#include "mozilla/LinkedList.h" - -using mozilla::LinkedList; -using mozilla::LinkedListElement; - -struct SomeClass : public LinkedListElement { - unsigned int mValue; - explicit SomeClass(int aValue) : mValue(aValue) {} - void incr() { ++mValue; } -}; - -struct PrivateClass : private LinkedListElement { - friend class LinkedList; - friend class LinkedListElement; -}; - -template -static void -CheckListValues(LinkedList& list, unsigned int (&values)[N]) -{ - size_t count = 0; - for (SomeClass* x : list) { - MOZ_RELEASE_ASSERT(x->mValue == values[count]); - ++count; - } - MOZ_RELEASE_ASSERT(count == N); -} - -static void -TestList() -{ - LinkedList list; - - SomeClass one(1), two(2), three(3); - - MOZ_RELEASE_ASSERT(list.isEmpty()); - MOZ_RELEASE_ASSERT(!list.getFirst()); - MOZ_RELEASE_ASSERT(!list.getLast()); - MOZ_RELEASE_ASSERT(!list.popFirst()); - MOZ_RELEASE_ASSERT(!list.popLast()); - - for (SomeClass* x : list) { - MOZ_RELEASE_ASSERT(x); - MOZ_RELEASE_ASSERT(false); - } - - list.insertFront(&one); - { unsigned int check[] { 1 }; CheckListValues(list, check); } - - MOZ_RELEASE_ASSERT(one.isInList()); - MOZ_RELEASE_ASSERT(!two.isInList()); - MOZ_RELEASE_ASSERT(!three.isInList()); - - MOZ_RELEASE_ASSERT(!list.isEmpty()); - MOZ_RELEASE_ASSERT(list.getFirst()->mValue == 1); - MOZ_RELEASE_ASSERT(list.getLast()->mValue == 1); - - list.insertFront(&two); - { unsigned int check[] { 2, 1 }; CheckListValues(list, check); } - - MOZ_RELEASE_ASSERT(list.getFirst()->mValue == 2); - MOZ_RELEASE_ASSERT(list.getLast()->mValue == 1); - - list.insertBack(&three); - { unsigned int check[] { 2, 1, 3 }; CheckListValues(list, check); } - - MOZ_RELEASE_ASSERT(list.getFirst()->mValue == 2); - MOZ_RELEASE_ASSERT(list.getLast()->mValue == 3); - - one.removeFrom(list); - { unsigned int check[] { 2, 3 }; CheckListValues(list, check); } - - three.setPrevious(&one); - { unsigned int check[] { 2, 1, 3 }; CheckListValues(list, check); } - - three.removeFrom(list); - { unsigned int check[] { 2, 1 }; CheckListValues(list, check); } - - two.setPrevious(&three); - { unsigned int check[] { 3, 2, 1 }; CheckListValues(list, check); } - - three.removeFrom(list); - { unsigned int check[] { 2, 1 }; CheckListValues(list, check); } - - two.setNext(&three); - { unsigned int check[] { 2, 3, 1 }; CheckListValues(list, check); } - - one.remove(); - { unsigned int check[] { 2, 3 }; CheckListValues(list, check); } - - two.remove(); - { unsigned int check[] { 3 }; CheckListValues(list, check); } - - three.setPrevious(&two); - { unsigned int check[] { 2, 3 }; CheckListValues(list, check); } - - three.remove(); - { unsigned int check[] { 2 }; CheckListValues(list, check); } - - two.remove(); - - list.insertBack(&three); - { unsigned int check[] { 3 }; CheckListValues(list, check); } - - list.insertFront(&two); - { unsigned int check[] { 2, 3 }; CheckListValues(list, check); } - - for (SomeClass* x : list) { - x->incr(); - } - - MOZ_RELEASE_ASSERT(list.getFirst() == &two); - MOZ_RELEASE_ASSERT(list.getLast() == &three); - MOZ_RELEASE_ASSERT(list.getFirst()->mValue == 3); - MOZ_RELEASE_ASSERT(list.getLast()->mValue == 4); -} - -static void -TestPrivate() -{ - LinkedList list; - PrivateClass one, two; - list.insertBack(&one); - list.insertBack(&two); - - size_t count = 0; - for (PrivateClass* p : list) { - MOZ_RELEASE_ASSERT(p, "cannot have null elements in list"); - count++; - } - MOZ_RELEASE_ASSERT(count == 2); -} - -int -main() -{ - TestList(); - TestPrivate(); - return 0; -} diff --git a/mfbt/tests/moz.build b/mfbt/tests/moz.build index 850254b57e0..227e02a03b9 100644 --- a/mfbt/tests/moz.build +++ b/mfbt/tests/moz.build @@ -22,7 +22,6 @@ CppUnitTests([ 'TestIntegerPrintfMacros', 'TestIntegerRange', 'TestJSONWriter', - 'TestLinkedList', 'TestMacroArgs', 'TestMacroForEach', 'TestMathAlgorithms',