Bug 1706694 - Don't overwrite leftmost rope's internals at the start when reusing the leftmost child buffer in string flattening r=jandem

This is unnecessary and was causing problems. We can leave the leftmost rope to
be handled in the same way as all the other ropes, and move the special case to
just avoid copying the chars when reusing the leftmost buffer.

The problem was that now we always barrier ropes, before overwriting the left
child pointer with the parent pointer. If this happens during incremental GC we
must ensure we haven't already overwritten this pointer we the buffer pointer.

This patch also moves changing the leftmost string into a dependent string
until the end.

Depends on D113316

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

Author: jonco3

js/src/vm/StringType.cpp

@@ -622,15 +622,15 @@ static bool UpdateNurseryBuffersOnTransfer(js::Nursery& nursery, JSString* from,
   return true;
 }
 
-static bool CanReuseLeftmostBuffer(JSRope* leftmostRope, size_t wholeLength,
+static bool CanReuseLeftmostBuffer(JSString* leftmostChild, size_t wholeLength,
                                    bool hasTwoByteChars) {
-  if (!leftmostRope->leftChild()->isExtensible()) {
+  if (!leftmostChild->isExtensible()) {
     return false;
   }
 
-  JSExtensibleString& leftmostChild = leftmostRope->leftChild()->asExtensible();
-  return leftmostChild.capacity() >= wholeLength &&
-         leftmostChild.hasTwoByteChars() == hasTwoByteChars;
+  JSExtensibleString& str = leftmostChild->asExtensible();
+  return str.capacity() >= wholeLength &&
+         str.hasTwoByteChars() == hasTwoByteChars;
 }
 
 JSLinearString* JSRope::flatten(JSContext* maybecx) {
@@ -733,49 +733,24 @@ JSLinearString* JSRope::flattenInternal(JSRope* root) {
   while (leftmostRope->leftChild()->isRope()) {
     leftmostRope = &leftmostRope->leftChild()->asRope();
   }
+  JSString* leftmostChild = leftmostRope->leftChild();
 
   bool reuseLeftmostBuffer = CanReuseLeftmostBuffer(
-      leftmostRope, wholeLength, std::is_same_v<CharT, char16_t>);
-
-  uint32_t leftmostChildLength = 0;  // Only set if we reuse leftmost buffer.
+      leftmostChild, wholeLength, std::is_same_v<CharT, char16_t>);
 
   if (reuseLeftmostBuffer) {
-    JSExtensibleString& left = leftmostRope->leftChild()->asExtensible();
-    size_t capacity = left.capacity();
+    JSExtensibleString& left = leftmostChild->asExtensible();
+    wholeCapacity = left.capacity();
     wholeChars = const_cast<CharT*>(left.nonInlineChars<CharT>(nogc));
-    wholeCapacity = capacity;
 
     // Nursery::registerMallocedBuffer is fallible, so attempt it first before
     // doing anything irreversible.
     if (!UpdateNurseryBuffersOnTransfer(nursery, &left, root, wholeChars,
                                         wholeCapacity * sizeof(CharT))) {
       return nullptr;
     }
-
-    ropeBarrierDuringFlattening<usingBarrier>(leftmostRope);
-    leftmostRope->setNonInlineChars(wholeChars);
-    leftmostChildLength = left.length();
-
-    // Remove memory association for left node we're about to make into a
-    // dependent string.
-    if (left.isTenured()) {
-      RemoveCellMemory(&left, left.allocSize(), MemoryUse::StringContents);
-    }
-
-    uint32_t flags = INIT_DEPENDENT_FLAGS;
-    if (left.inStringToAtomCache()) {
-      flags |= IN_STRING_TO_ATOM_CACHE;
-    }
-    left.setLengthAndFlags(leftmostChildLength,
-                           StringFlagsForCharType<CharT>(flags));
-    left.d.s.u3.base = (JSLinearString*)root; /* will be true on exit */
-    if (left.isTenured() && !root->isTenured()) {
-      // leftmost child -> root is a tenured -> nursery edge.
-      root->storeBuffer()->putWholeCell(&left);
-    }
   } else {
     // If we can't reuse the leftmost child's buffer, allocate a new one.
-
     if (!AllocChars(root, wholeLength, &wholeChars, &wholeCapacity)) {
       return nullptr;
     }
@@ -807,19 +782,16 @@ first_visit_node : {
   parent = nullptr;
   parentFlag = 0;
 
-  if (reuseLeftmostBuffer && str == leftmostRope) {
-    // Left child has already been overwritten.
-    pos += leftmostChildLength;
-    goto visit_right_child;
-  }
   if (left.isRope()) {
     /* Return to this node when 'left' done, then goto visit_right_child. */
     parent = str;
     parentFlag = FLATTEN_VISIT_RIGHT;
     str = &left;
     goto first_visit_node;
   }
-  CopyChars(pos, left.asLinear());
+  if (!(reuseLeftmostBuffer && &left == leftmostChild)) {
+    CopyChars(pos, left.asLinear());
+  }
   pos += left.length();
 }
 
@@ -881,12 +853,33 @@ finish_node : {
   MOZ_ASSERT(str == root);
   MOZ_ASSERT(pos == wholeChars + wholeLength);
 
-  str->setLengthAndFlags(wholeLength,
-                         StringFlagsForCharType<CharT>(EXTENSIBLE_FLAGS));
-  str->setNonInlineChars(wholeChars);
-  str->d.s.u3.capacity = wholeCapacity;
-  if (str->isTenured()) {
-    AddCellMemory(str, str->asLinear().allocSize(), MemoryUse::StringContents);
+  root->setLengthAndFlags(wholeLength,
+                          StringFlagsForCharType<CharT>(EXTENSIBLE_FLAGS));
+  root->setNonInlineChars(wholeChars);
+  root->d.s.u3.capacity = wholeCapacity;
+  if (root->isTenured()) {
+    AddCellMemory(root, root->asLinear().allocSize(),
+                  MemoryUse::StringContents);
+  }
+
+  if (reuseLeftmostBuffer) {
+    // Remove memory association for left node we're about to make into a
+    // dependent string.
+    JSString& left = *leftmostChild;
+    if (left.isTenured()) {
+      RemoveCellMemory(&left, left.allocSize(), MemoryUse::StringContents);
+    }
+
+    uint32_t flags = INIT_DEPENDENT_FLAGS;
+    if (left.inStringToAtomCache()) {
+      flags |= IN_STRING_TO_ATOM_CACHE;
+    }
+    left.setLengthAndFlags(left.length(), StringFlagsForCharType<CharT>(flags));
+    left.d.s.u3.base = &root->asLinear();
+    if (left.isTenured() && !root->isTenured()) {
+      // leftmost child -> root is a tenured -> nursery edge.
+      root->storeBuffer()->putWholeCell(&left);
+    }
   }
 
   return &root->asLinear();