Bug 1875030 - Update element ranges on the mark stack before and after marking to take account of shifted elements. r=sfink,jandem a=RyanVM

Differential Revision: https://phabricator.services.mozilla.com/D211336

js/src/gc/GC.cpp

@@ -3100,20 +3100,14 @@ GCRuntime::MarkQueueProgress GCRuntime::processTestMarkQueue() {
         return QueueSuspended;
       }
 
-      // Mark the object and push it onto the stack.
-      size_t oldPosition = marker().stack.position();
-      marker().markAndTraverse<NormalMarkingOptions>(obj);
-
-      // If we overflow the stack here and delay marking, then we won't be
-      // testing what we think we're testing.
-      if (marker().stack.position() == oldPosition) {
+      // Mark the object.
+      AutoEnterOOMUnsafeRegion oomUnsafe;
+      if (!marker().markOneObjectForTest(obj)) {
+        // If we overflowed the stack here and delayed marking, then we won't be
+        // testing what we think we're testing.
         MOZ_ASSERT(obj->asTenured().arena()->onDelayedMarkingList());
-        AutoEnterOOMUnsafeRegion oomUnsafe;
         oomUnsafe.crash("Overflowed stack while marking test queue");
       }
-
-      SliceBudget unlimited = SliceBudget::unlimited();
-      marker().processMarkStackTop<NormalMarkingOptions>(unlimited);
     } else if (val.isString()) {
       JSLinearString* str = &val.toString()->asLinear();
       if (js::StringEqualsLiteral(str, "yield") && isIncrementalGc()) {

js/src/gc/GCMarker.h

@@ -38,7 +38,9 @@ namespace gc {
 enum IncrementalProgress { NotFinished = 0, Finished };
 
 class AutoSetMarkColor;
+class AutoUpdateMarkStackRanges;
 struct Cell;
+class MarkStackIter;
 class ParallelMarker;
 class UnmarkGrayTracer;
 
@@ -136,10 +138,12 @@ class MarkStack {
     size_t start() const;
     TaggedPtr ptr() const;
 
+    void setStart(size_t newStart);
+
+   private:
     static constexpr size_t StartShift = 2;
     static constexpr size_t KindMask = (1 << StartShift) - 1;
 
-   private:
     uintptr_t startAndKind_;
     TaggedPtr ptr_;
   };
@@ -224,6 +228,13 @@ class MarkStack {
   // The maximum stack capacity to grow to.
   MainThreadOrGCTaskData<size_t> maxCapacity_{SIZE_MAX};
 #endif
+
+#ifdef DEBUG
+  MainThreadOrGCTaskData<bool> elementsRangesAreValid;
+  friend class js::GCMarker;
+#endif
+
+  friend class MarkStackIter;
 };
 
 static_assert(unsigned(SlotsOrElementsKind::Unused) ==
@@ -232,6 +243,25 @@ static_assert(unsigned(SlotsOrElementsKind::Unused) ==
               "difference between SlotsOrElementsRange::startAndKind_ and a "
               "tagged SlotsOrElementsRange");
 
+class MOZ_STACK_CLASS MarkStackIter {
+  MarkStack& stack_;
+  size_t pos_;
+
+ public:
+  explicit MarkStackIter(MarkStack& stack);
+
+  bool done() const;
+  void next();
+
+  MarkStack::Tag peekTag() const;
+  bool isSlotsOrElementsRange() const;
+  MarkStack::SlotsOrElementsRange& slotsOrElementsRange();
+
+ private:
+  size_t position() const;
+  MarkStack::TaggedPtr peekPtr() const;
+};
+
 // Bitmask of options to parameterize MarkingTracerT.
 namespace MarkingOptions {
 enum : uint32_t {
@@ -361,6 +391,8 @@ class GCMarker {
   void setCheckAtomMarking(bool check);
 
   bool shouldCheckCompartments() { return strictCompartmentChecking; }
+
+  bool markOneObjectForTest(JSObject* obj);
 #endif
 
   bool markCurrentColorInParallel(SliceBudget& budget);
@@ -403,6 +435,13 @@ class GCMarker {
   template <typename Tracer>
   void setMarkingStateAndTracer(MarkingState prev, MarkingState next);
 
+  // The mutator can shift object elements which could invalidate any elements
+  // index on the mark stack. Change the index to be relative to the elements
+  // allocation (to ignore shifted elements) while the mutator is running.
+  void updateRangesAtStartOfSlice();
+  void updateRangesAtEndOfSlice();
+  friend class gc::AutoUpdateMarkStackRanges;
+
   template <uint32_t markingOptions>
   bool processMarkStackTop(SliceBudget& budget);
   friend class gc::GCRuntime;

js/src/gc/Marking.cpp

@@ -1307,9 +1307,20 @@ bool GCMarker::doMarking(SliceBudget& budget, ShouldReportMarkTime reportTime) {
   return true;
 }
 
+class MOZ_RAII gc::AutoUpdateMarkStackRanges {
+  GCMarker& marker_;
+
+ public:
+  explicit AutoUpdateMarkStackRanges(GCMarker& marker) : marker_(marker) {
+    marker_.updateRangesAtStartOfSlice();
+  }
+  ~AutoUpdateMarkStackRanges() { marker_.updateRangesAtEndOfSlice(); }
+};
+
 template <uint32_t opts, MarkColor color>
 bool GCMarker::markOneColor(SliceBudget& budget) {
   AutoSetMarkColor setColor(*this, color);
+  AutoUpdateMarkStackRanges updateRanges(*this);
 
   while (processMarkStackTop<opts>(budget)) {
     if (stack.isEmpty()) {
@@ -1321,6 +1332,8 @@ bool GCMarker::markOneColor(SliceBudget& budget) {
 }
 
 bool GCMarker::markCurrentColorInParallel(SliceBudget& budget) {
+  AutoUpdateMarkStackRanges updateRanges(*this);
+
   ParallelMarker::AtomicCount& waitingTaskCount =
       parallelMarker_->waitingTaskCountRef();
 
@@ -1340,6 +1353,26 @@ bool GCMarker::markCurrentColorInParallel(SliceBudget& budget) {
   return false;
 }
 
+#ifdef DEBUG
+bool GCMarker::markOneObjectForTest(JSObject* obj) {
+  MOZ_ASSERT(obj->zone()->isGCMarking());
+  MOZ_ASSERT(!obj->isMarked(markColor()));
+
+  size_t oldPosition = stack.position();
+  markAndTraverse<NormalMarkingOptions>(obj);
+  if (stack.position() == oldPosition) {
+    return false;
+  }
+
+  AutoUpdateMarkStackRanges updateRanges(*this);
+
+  SliceBudget unlimited = SliceBudget::unlimited();
+  processMarkStackTop<NormalMarkingOptions>(unlimited);
+
+  return true;
+}
+#endif
+
 static inline void CheckForCompartmentMismatch(JSObject* obj, JSObject* obj2) {
 #ifdef DEBUG
   if (MOZ_UNLIKELY(obj->compartment() != obj2->compartment())) {
@@ -1366,6 +1399,44 @@ static inline size_t NumUsedDynamicSlots(NativeObject* obj) {
   return nslots - nfixed;
 }
 
+void GCMarker::updateRangesAtStartOfSlice() {
+  for (MarkStackIter iter(stack); !iter.done(); iter.next()) {
+    if (iter.isSlotsOrElementsRange()) {
+      MarkStack::SlotsOrElementsRange& range = iter.slotsOrElementsRange();
+      if (range.kind() == SlotsOrElementsKind::Elements) {
+        NativeObject* obj = &range.ptr().asRangeObject()->as<NativeObject>();
+        size_t index = range.start();
+        size_t numShifted = obj->getElementsHeader()->numShiftedElements();
+        index -= std::min(numShifted, index);
+        range.setStart(index);
+      }
+    }
+  }
+
+#ifdef DEBUG
+  MOZ_ASSERT(!stack.elementsRangesAreValid);
+  stack.elementsRangesAreValid = true;
+#endif
+}
+
+void GCMarker::updateRangesAtEndOfSlice() {
+  for (MarkStackIter iter(stack); !iter.done(); iter.next()) {
+    if (iter.isSlotsOrElementsRange()) {
+      MarkStack::SlotsOrElementsRange& range = iter.slotsOrElementsRange();
+      if (range.kind() == SlotsOrElementsKind::Elements) {
+        NativeObject* obj = &range.ptr().asRangeObject()->as<NativeObject>();
+        size_t numShifted = obj->getElementsHeader()->numShiftedElements();
+        range.setStart(range.start() + numShifted);
+      }
+    }
+  }
+
+#ifdef DEBUG
+  MOZ_ASSERT(stack.elementsRangesAreValid);
+  stack.elementsRangesAreValid = false;
+#endif
+}
+
 template <uint32_t opts>
 inline bool GCMarker::processMarkStackTop(SliceBudget& budget) {
   /*
@@ -1379,6 +1450,7 @@ inline bool GCMarker::processMarkStackTop(SliceBudget& budget) {
    */
 
   MOZ_ASSERT(!stack.isEmpty());
+  MOZ_ASSERT(stack.elementsRangesAreValid);
   MOZ_ASSERT_IF(markColor() == MarkColor::Gray, !hasBlackEntries());
 
   JSObject* obj;             // The object being scanned.
@@ -1409,12 +1481,7 @@ inline bool GCMarker::processMarkStackTop(SliceBudget& budget) {
 
       case SlotsOrElementsKind::Elements: {
         base = nobj->getDenseElements();
-
-        // Account for shifted elements.
-        size_t numShifted = nobj->getElementsHeader()->numShiftedElements();
-        size_t initlen = nobj->getDenseInitializedLength();
-        index = std::max(index, numShifted) - numShifted;
-        end = initlen;
+        end = nobj->getDenseInitializedLength();
         break;
       }
 
@@ -1590,11 +1657,9 @@ struct MapTypeToMarkStackTag<BaseScript*> {
   static const auto value = MarkStack::ScriptTag;
 };
 
-#ifdef DEBUG
 static inline bool TagIsRangeTag(MarkStack::Tag tag) {
   return tag == MarkStack::SlotsOrElementsRangeTag;
 }
-#endif
 
 inline MarkStack::TaggedPtr::TaggedPtr(Tag tag, Cell* ptr)
     : bits(tag | uintptr_t(ptr)) {
@@ -1661,6 +1726,11 @@ inline size_t MarkStack::SlotsOrElementsRange::start() const {
   return startAndKind_ >> StartShift;
 }
 
+inline void MarkStack::SlotsOrElementsRange::setStart(size_t newStart) {
+  startAndKind_ = (newStart << StartShift) | uintptr_t(kind());
+  MOZ_ASSERT(start() == newStart);
+}
+
 inline MarkStack::TaggedPtr MarkStack::SlotsOrElementsRange::ptr() const {
   return ptr_;
 }
@@ -1931,6 +2001,45 @@ size_t MarkStack::sizeOfExcludingThis(
   return stack().sizeOfExcludingThis(mallocSizeOf);
 }
 
+MarkStackIter::MarkStackIter(MarkStack& stack)
+    : stack_(stack), pos_(stack.position()) {}
+
+inline size_t MarkStackIter::position() const { return pos_; }
+
+inline bool MarkStackIter::done() const { return position() == 0; }
+
+inline void MarkStackIter::next() {
+  if (isSlotsOrElementsRange()) {
+    MOZ_ASSERT(position() >= ValueRangeWords);
+    pos_ -= ValueRangeWords;
+    return;
+  }
+
+  MOZ_ASSERT(!done());
+  pos_--;
+}
+
+inline bool MarkStackIter::isSlotsOrElementsRange() const {
+  return TagIsRangeTag(peekTag());
+}
+
+inline MarkStack::Tag MarkStackIter::peekTag() const { return peekPtr().tag(); }
+
+inline MarkStack::TaggedPtr MarkStackIter::peekPtr() const {
+  MOZ_ASSERT(!done());
+  return stack_.stack()[pos_ - 1];
+}
+
+inline MarkStack::SlotsOrElementsRange& MarkStackIter::slotsOrElementsRange() {
+  MOZ_ASSERT(TagIsRangeTag(peekTag()));
+  MOZ_ASSERT(position() >= ValueRangeWords);
+
+  MarkStack::TaggedPtr* ptr = &stack_.stack()[pos_ - ValueRangeWords];
+  auto& range = *reinterpret_cast<MarkStack::SlotsOrElementsRange*>(ptr);
+  range.assertValid();
+  return range;
+}
+
 /*** GCMarker ***************************************************************/
 
 /*
@@ -2244,6 +2353,7 @@ void GCRuntime::processDelayedMarkingList(MarkColor color) {
   // were added.
 
   AutoSetMarkColor setColor(marker(), color);
+  AutoUpdateMarkStackRanges updateRanges(marker());
 
   do {
     delayedMarkingWorkAdded = false;