Bug 1706694 - When flattening ropes, store the parent pointer in the left child field rather than overwriting the cell header r=jandem

This moves the part that sets the charater data pointer to finish_node as this
also shares this field. We pass the parent pointer (and flags) into
first_visit_node and set these after we've extracted the left child pointer.

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

Author: jonco3

js/src/gc/Cell.h

@@ -142,17 +142,9 @@ struct Cell {
   // compacting GC and is now a RelocationOverlay.
   static constexpr uintptr_t FORWARD_BIT = Bit(0);
 
-  // Indicates whether the cell header has been temporarily replaced by calling
-  // setTemporaryGCUnsafeData(). This is currently only used during rope
-  // flattening.
-  static constexpr uintptr_t TEMP_DATA_BIT = Bit(1);
-
-  // For use by derived cell classes. This is currently only used during rope
-  // flattening.
-  static constexpr uintptr_t USER_BIT = Bit(2);
+  // Bits 1 and 2 are reserved for future use by the GC.
 
   bool isForwarded() const { return header_ & FORWARD_BIT; }
-  bool hasTempHeaderData() const { return header_ & TEMP_DATA_BIT; }
   uintptr_t flags() const { return header_ & RESERVED_MASK; }
 
   MOZ_ALWAYS_INLINE bool isTenured() const { return !IsInsideNursery(this); }
@@ -649,23 +641,6 @@ class alignas(gc::CellAlignBytes) CellWithLengthAndFlags : public Cell {
 #endif
   }
 
-  // Subclasses can store temporary data in the flags word. This is not GC safe
-  // and users must ensure flags/length are never checked (including by asserts)
-  // while this data is stored. Use of this method is strongly discouraged!
-  void setTemporaryGCUnsafeData(uintptr_t data) {
-    MOZ_ASSERT((data & TEMP_DATA_BIT) == 0);
-    header_ = data | TEMP_DATA_BIT;
-  }
-
-  // To get back the data, values to safely re-initialize clobbered length and
-  // flags must be provided.
-  uintptr_t unsetTemporaryGCUnsafeData(uint32_t len, uint32_t flags) {
-    MOZ_ASSERT(hasTempHeaderData());
-    uintptr_t data = header_;
-    setHeaderLengthAndFlags(len, flags);
-    return data & ~TEMP_DATA_BIT;
-  }
-
  public:
   // Returns the offset of header_. JIT code should use offsetOfFlags
   // below.

js/src/gc/Marking.cpp

@@ -4158,7 +4158,6 @@ void BarrierTracer::performBarrier(JS::GCCellPtr cell) {
   MOZ_ASSERT(CurrentThreadCanAccessRuntime(runtime()));
   MOZ_ASSERT(!runtime()->gc.isBackgroundMarking());
   MOZ_ASSERT(!cell.asCell()->isForwarded());
-  MOZ_ASSERT(!cell.asCell()->hasTempHeaderData());
 
   // Mark the cell here to prevent us recording it again.
   if (!cell.asCell()->asTenured().markIfUnmarked()) {
@@ -4215,7 +4214,7 @@ void GCMarker::traceBarrieredCell(JS::GCCellPtr cell) {
     MOZ_ASSERT(thing->isMarkedBlack());
 
     if constexpr (std::is_same_v<decltype(thing), JSString*>) {
-      if (thing->isBeingFlattened()) {
+      if (thing->isRope() && thing->asRope().isBeingFlattened()) {
         // This string is an interior node of a rope that is currently being
         // flattened. The flattening process invokes the barrier on all nodes in
         // the tree, so interior nodes need not be traversed.

js/src/vm/StringType.cpp

@@ -8,6 +8,7 @@
 
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/Casting.h"
+#include "mozilla/DebugOnly.h"
 #include "mozilla/EndianUtils.h"
 #include "mozilla/HashFunctions.h"
 #include "mozilla/Latin1.h"
@@ -723,11 +724,6 @@ JSLinearString* JSRope::flattenInternal(JSRope* root) {
   size_t wholeCapacity;
   CharT* wholeChars;
 
-  // JSString::setFlattenData() is used to store a tagged pointer to the parent
-  // node. These flags indicates what to do when we return to the parent.
-  static constexpr uintptr_t Flag_FinishNode = 0;
-  static constexpr uintptr_t Flag_VisitRightChild = Cell::USER_BIT;
-
   AutoCheckCannotGC nogc;
 
   Nursery& nursery = root->runtimeFromMainThread()->gc.nursery();
@@ -796,19 +792,30 @@ JSLinearString* JSRope::flattenInternal(JSRope* root) {
   JSString* str = root;
   CharT* pos = wholeChars;
 
+  JSString* parent = nullptr;
+  uint32_t parentFlag = 0;
+
 first_visit_node : {
+  MOZ_ASSERT_IF(str != root, parent && parentFlag);
+  MOZ_ASSERT(!str->asRope().isBeingFlattened());
+
+  ropeBarrierDuringFlattening<usingBarrier>(str);
+
+  JSString& left = *str->d.s.u2.left;
+  str->d.s.u2.parent = parent;
+  str->setFlagBit(parentFlag);
+  parent = nullptr;
+  parentFlag = 0;
+
   if (reuseLeftmostBuffer && str == leftmostRope) {
     // Left child has already been overwritten.
     pos += leftmostChildLength;
     goto visit_right_child;
   }
-
-  JSString& left = *str->d.s.u2.left;
-  ropeBarrierDuringFlattening<usingBarrier>(str);
-  str->setNonInlineChars(pos);
   if (left.isRope()) {
     /* Return to this node when 'left' done, then goto visit_right_child. */
-    left.setFlattenData(str, Flag_VisitRightChild);
+    parent = str;
+    parentFlag = FLATTEN_VISIT_RIGHT;
     str = &left;
     goto first_visit_node;
   }
@@ -820,7 +827,8 @@ visit_right_child : {
   JSString& right = *str->d.s.u3.right;
   if (right.isRope()) {
     /* Return to this node when 'right' done, then goto finish_node. */
-    right.setFlattenData(str, Flag_FinishNode);
+    parent = str;
+    parentFlag = FLATTEN_FINISH_NODE;
     str = &right;
     goto first_visit_node;
   }
@@ -833,11 +841,19 @@ finish_node : {
     goto finish_root;
   }
 
-  JSString* parent;
-  uintptr_t flattenFlags;
-  uint32_t len = pos - str->nonInlineCharsRaw<CharT>();
-  parent = str->unsetFlattenData(
-      len, StringFlagsForCharType<CharT>(INIT_DEPENDENT_FLAGS), &flattenFlags);
+  MOZ_ASSERT(pos >= wholeChars);
+  CharT* chars = pos - str->length();
+  JSString* strParent = str->d.s.u2.parent;
+  str->setNonInlineChars(chars);
+
+  MOZ_ASSERT(str->asRope().isBeingFlattened());
+  mozilla::DebugOnly<bool> visitRight = str->flags() & FLATTEN_VISIT_RIGHT;
+  bool finishNode = str->flags() & FLATTEN_FINISH_NODE;
+  MOZ_ASSERT(visitRight != finishNode);
+
+  // This also clears the flags related to flattening.
+  str->setLengthAndFlags(str->length(),
+                         StringFlagsForCharType<CharT>(INIT_DEPENDENT_FLAGS));
   str->d.s.u3.base = (JSLinearString*)root; /* will be true on exit */
 
   // Every interior (rope) node in the rope's tree will be visited during
@@ -852,12 +868,12 @@ finish_node : {
     root->storeBuffer()->putWholeCell(str);
   }
 
-  str = parent;
-  if (flattenFlags == Flag_VisitRightChild) {
-    goto visit_right_child;
+  str = strParent;
+  if (finishNode) {
+    goto finish_node;
   }
-  MOZ_ASSERT(flattenFlags == Flag_FinishNode);
-  goto finish_node;
+  MOZ_ASSERT(visitRight);
+  goto visit_right_child;
 }
 
 finish_root:
@@ -881,6 +897,8 @@ template <JSRope::UsingBarrier usingBarrier>
 inline void JSRope::ropeBarrierDuringFlattening(JSString* str) {
   // |str| is a rope but its flags maybe have been overwritten by temporary data
   // at this point.
+  MOZ_ASSERT(str->isRope());
+  MOZ_ASSERT(!str->asRope().isBeingFlattened());
   if constexpr (usingBarrier) {
     gc::PreWriteBarrierDuringFlattening(str->d.s.u2.left);
     gc::PreWriteBarrierDuringFlattening(str->d.s.u3.right);

js/src/vm/StringType.h

@@ -219,6 +219,7 @@ class JSString : public js::gc::CellWithLengthAndFlags {
           const char16_t* nonInlineCharsTwoByte;      /* JSLinearString, except
                                                          JS(Fat)InlineString */
           JSString* left;                             /* JSRope */
+          JSString* parent;                           /* Used in flattening */
         } u2;
         union {
           JSLinearString* base; /* JSDependentString */
@@ -333,6 +334,13 @@ class JSString : public js::gc::CellWithLengthAndFlags {
   // clearing this bit.
   static const uint32_t IN_STRING_TO_ATOM_CACHE = js::Bit(13);
 
+  // Flags used during rope flattening that indicate what action to perform when
+  // returning to the rope's parent rope.
+  static const uint32_t FLATTEN_VISIT_RIGHT = js::Bit(14);
+  static const uint32_t FLATTEN_FINISH_NODE = js::Bit(15);
+  static const uint32_t FLATTEN_MASK =
+      FLATTEN_VISIT_RIGHT | FLATTEN_FINISH_NODE;
+
   static const uint32_t MAX_LENGTH = JS::MaxStringLength;
 
   static const JS::Latin1Char MAX_LATIN1_CHAR = 0xff;
@@ -422,19 +430,6 @@ class JSString : public js::gc::CellWithLengthAndFlags {
 #endif
   }
 
-  void setFlattenData(JSString* parent, uintptr_t flags) {
-    MOZ_ASSERT((uintptr_t(parent) & RESERVED_MASK) == 0);
-    MOZ_ASSERT((flags & ~RESERVED_MASK) == 0);
-    setTemporaryGCUnsafeData(uintptr_t(parent) | flags);
-  }
-
-  JSString* unsetFlattenData(uint32_t len, uint32_t newFlags,
-                             uintptr_t* oldFlagsOut) {
-    uintptr_t data = unsetTemporaryGCUnsafeData(len, newFlags);
-    *oldFlagsOut = data & RESERVED_MASK;
-    return reinterpret_cast<JSString*>(data & ~RESERVED_MASK);
-  }
-
   // Get correct non-inline chars enum arm for given type
   template <typename CharT>
   MOZ_ALWAYS_INLINE const CharT* nonInlineCharsRaw() const;
@@ -615,8 +610,6 @@ class JSString : public js::gc::CellWithLengthAndFlags {
   mozilla::Maybe<mozilla::Tuple<size_t, size_t>> encodeUTF8Partial(
       const JS::AutoRequireNoGC& nogc, mozilla::Span<char> buffer) const;
 
-  bool isBeingFlattened() const { return hasTempHeaderData(); }
-
  private:
   // To help avoid writing Spectre-unsafe code, we only allow MacroAssembler
   // to call the method below.
@@ -758,8 +751,13 @@ class JSRope : public JSString {
   // Returns the same value as if this were a linear string being hashed.
   [[nodiscard]] bool hash(uint32_t* outhHash) const;
 
+  // The process of flattening a rope temporarily overwrites the left pointer of
+  // interior nodes in the rope DAG with the parent pointer.
+  bool isBeingFlattened() const { return flags() & FLATTEN_MASK; }
+
   JSString* leftChild() const {
     MOZ_ASSERT(isRope());
+    MOZ_ASSERT(!isBeingFlattened());  // Flattening overwrites this field.
     return d.s.u2.left;
   }