Bug 1370069 - Fix several issues with incremental atom sweeping r=sfink

js/public/HashTable.h

@@ -1789,6 +1789,7 @@ class HashTable : private AllocPolicy
     {
         mozilla::ReentrancyGuard g(*this);
         MOZ_ASSERT(table);
+        MOZ_ASSERT_IF(!p.isValid(), p.table_ == this);
         MOZ_ASSERT(!p.found());
         MOZ_ASSERT(!(p.keyHash & sCollisionBit));
 

js/src/gc/GCRuntime.h

@@ -1005,6 +1005,7 @@ class GCRuntime
                                                       SliceBudget& budget, AllocKind kind);
     static IncrementalProgress sweepAtomsTable(GCRuntime* gc, FreeOp* fop, Zone* zone,
                                                SliceBudget& budget, AllocKind kind);
+    void startSweepingAtomsTable();
     IncrementalProgress sweepAtomsTable(SliceBudget& budget);
     static IncrementalProgress finalizeAllocKind(GCRuntime* gc, FreeOp* fop, Zone* zone,
                                                  SliceBudget& budget, AllocKind kind);

js/src/jit-test/tests/gc/bug-1370069.js

@@ -0,0 +1,6 @@
+// |jit-test| error:SyntaxError
+try {
+    eval("}");
+} catch (exc) {}
+gczeal(17, 1);
+6.900653737167637, (yield);

js/src/jsatom.cpp

@@ -170,6 +170,19 @@ JSRuntime::finishAtoms()
     emptyString = nullptr;
 }
 
+static inline void
+TracePinnedAtoms(JSTracer* trc, const AtomSet& atoms)
+{
+    for (auto r = atoms.all(); !r.empty(); r.popFront()) {
+        const AtomStateEntry& entry = r.front();
+        if (entry.isPinned()) {
+            JSAtom* atom = entry.asPtrUnbarriered();
+            TraceRoot(trc, &atom, "interned_atom");
+            MOZ_ASSERT(entry.asPtrUnbarriered() == atom);
+        }
+    }
+}
+
 void
 js::TraceAtoms(JSTracer* trc, AutoLockForExclusiveAccess& lock)
 {
@@ -178,15 +191,9 @@ js::TraceAtoms(JSTracer* trc, AutoLockForExclusiveAccess& lock)
     if (rt->atomsAreFinished())
         return;
 
-    for (AtomSet::Enum e(rt->atoms(lock)); !e.empty(); e.popFront()) {
-        const AtomStateEntry& entry = e.front();
-        if (!entry.isPinned())
-            continue;
-
-        JSAtom* atom = entry.asPtrUnbarriered();
-        TraceRoot(trc, &atom, "interned_atom");
-        MOZ_ASSERT(entry.asPtrUnbarriered() == atom);
-    }
+    TracePinnedAtoms(trc, rt->atoms(lock));
+    if (rt->atomsAddedWhileSweeping())
+        TracePinnedAtoms(trc, *rt->atomsAddedWhileSweeping());
 }
 
 void
@@ -250,6 +257,17 @@ JSRuntime::transformToPermanentAtoms(JSContext* cx)
     return true;
 }
 
+static inline AtomSet::Ptr
+LookupAtomState(JSRuntime* rt, const AtomHasher::Lookup& lookup)
+{
+    MOZ_ASSERT(rt->currentThreadHasExclusiveAccess());
+
+    AtomSet::Ptr p = rt->unsafeAtoms().lookup(lookup); // Safe because we hold the lock.
+    if (!p && rt->atomsAddedWhileSweeping())
+        p = rt->atomsAddedWhileSweeping()->lookup(lookup);
+    return p;
+}
+
 bool
 AtomIsPinned(JSContext* cx, JSAtom* atom)
 {
@@ -267,7 +285,7 @@ AtomIsPinned(JSContext* cx, JSAtom* atom)
 
     AutoLockForExclusiveAccess lock(cx);
 
-    p = cx->runtime()->atoms(lock).lookup(lookup);
+    p = LookupAtomState(cx->runtime(), lookup);
     if (!p)
         return false;
 
@@ -285,7 +303,7 @@ AtomIsPinnedInRuntime(JSRuntime* rt, JSAtom* atom)
 
     AtomHasher::Lookup lookup(atom);
 
-    AtomSet::Ptr p = rt->unsafeAtoms().lookup(lookup);
+    AtomSet::Ptr p = LookupAtomState(rt, lookup);
     MOZ_ASSERT(p);
 
     return p->isPinned();
@@ -362,7 +380,7 @@ AtomizeAndCopyChars(JSContext* cx, const CharT* tbchars, size_t length, PinningB
         // is dead.
         if (!p) {
             if (AtomSet::AddPtr p2 = atoms.lookupForAdd(lookup)) {
-                JSAtom* atom = p2->asPtr(cx);
+                JSAtom* atom = p2->asPtrUnbarriered();
                 if (!IsAboutToBeFinalizedUnbarriered(&atom))
                     p = p2;
             }
@@ -400,7 +418,8 @@ AtomizeAndCopyChars(JSContext* cx, const CharT* tbchars, size_t length, PinningB
         // We have held the lock since looking up p, and the operations we've done
         // since then can't GC; therefore the atoms table has not been modified and
         // p is still valid.
-        if (!atoms.add(p, AtomStateEntry(atom, bool(pin)))) {
+        AtomSet* addSet = atomsAddedWhileSweeping ? atomsAddedWhileSweeping : &atoms;
+        if (!addSet->add(p, AtomStateEntry(atom, bool(pin)))) {
             ReportOutOfMemory(cx); /* SystemAllocPolicy does not report OOM. */
             return nullptr;
         }
@@ -440,7 +459,7 @@ js::AtomizeString(JSContext* cx, JSString* str,
 
         AutoLockForExclusiveAccess lock(cx);
 
-        p = cx->atoms(lock).lookup(lookup);
+        p = LookupAtomState(cx->runtime(), lookup);
         MOZ_ASSERT(p); /* Non-static atom must exist in atom state set. */
         MOZ_ASSERT(p->asPtrUnbarriered() == &atom);
         MOZ_ASSERT(pin == PinAtom);

js/src/jsgc.cpp

@@ -5276,6 +5276,9 @@ GCRuntime::beginSweepingSweepGroup()
             joinTask(task, PhaseKind::SWEEP_WEAK_CACHES, lock);
     }
 
+    if (sweepingAtoms)
+        startSweepingAtomsTable();
+
     // Queue all GC things in all zones for sweeping, either on the foreground
     // or on the background thread.
 
@@ -5493,6 +5496,27 @@ GCRuntime::mergeSweptObjectArenas(GCRuntime* gc, FreeOp* fop, Zone* zone, SliceB
     return Finished;
 }
 
+void
+GCRuntime::startSweepingAtomsTable()
+{
+    auto& maybeAtoms = maybeAtomsToSweep.ref();
+    MOZ_ASSERT(maybeAtoms.isNothing());
+
+    AtomSet* atomsTable = rt->atomsForSweeping();
+    if (!atomsTable)
+        return;
+
+    // Create a secondary table to hold new atoms added while we're sweeping
+    // the main table incrementally.
+    if (!rt->createAtomsAddedWhileSweepingTable()) {
+        atomsTable->sweep();
+        return;
+    }
+
+    // Initialize remaining atoms to sweep.
+    maybeAtoms.emplace(*atomsTable);
+}
+
 /* static */ IncrementalProgress
 GCRuntime::sweepAtomsTable(GCRuntime* gc, FreeOp* fop, Zone* zone, SliceBudget& budget,
                            AllocKind kind)
@@ -5503,33 +5527,22 @@ GCRuntime::sweepAtomsTable(GCRuntime* gc, FreeOp* fop, Zone* zone, SliceBudget&
     return gc->sweepAtomsTable(budget);
 }
 
+
 IncrementalProgress
 GCRuntime::sweepAtomsTable(SliceBudget& budget)
 {
     gcstats::AutoPhase ap(stats(), gcstats::PhaseKind::SWEEP_ATOMS_TABLE);
 
     auto& maybeAtoms = maybeAtomsToSweep.ref();
-    MOZ_ASSERT_IF(maybeAtoms.isSome(), !maybeAtoms.ref().empty());
-
-    AtomSet* atomsTable = rt->atomsForSweeping();
-    if (!atomsTable)
+    if (!maybeAtoms)
         return Finished;
 
-    if (maybeAtoms.isNothing()) {
-        // Create a secondary table to hold new atoms added while we're sweeping
-        // the main table incrementally.
-        if (!rt->createAtomsAddedWhileSweepingTable()) {
-            atomsTable->sweep();
-            return Finished;
-        }
-
-        // Initialize remaining atoms to sweep.
-        maybeAtoms.emplace(*atomsTable);
-    }
+    MOZ_ASSERT(rt->atomsAddedWhileSweeping());
 
     // Sweep the table incrementally until we run out of work or budget.
     auto& atomsToSweep = *maybeAtoms;
     while (!atomsToSweep.empty()) {
+        budget.step();
         if (budget.isOverBudget())
             return NotFinished;
 
@@ -5541,6 +5554,8 @@ GCRuntime::sweepAtomsTable(SliceBudget& budget)
 
     // Add any new atoms from the secondary table.
     AutoEnterOOMUnsafeRegion oomUnsafe;
+    AtomSet* atomsTable = rt->atomsForSweeping();
+    MOZ_ASSERT(atomsTable);
     for (auto r = rt->atomsAddedWhileSweeping()->all(); !r.empty(); r.popFront()) {
         if (!atomsTable->putNew(AtomHasher::Lookup(r.front().asPtrUnbarriered()), r.front()))
             oomUnsafe.crash("Adding atom from secondary table after sweep");