Reland "[heap] String::MakeThin can get away without NotifyObjectLayoutChange"

This is a reland of 6e621f84

Original change's description:
> [heap] String::MakeThin can get away without NotifyObjectLayoutChange
>
> String::MakeThin doesn't need to invoke NotifyObjectLayoutChange because
> ThinString will only introduce tagged values and hence will not
> overwrite recorded slots with untagged values.
>
> Bug: v8:10315
> Change-Id: Iaff9c06cef763462eb57bf3debc5183ae8db6fa0
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2448792
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#70321}

The reland fixes the failures by first updating actual and only then
updating the map with a release store. This ensures that concurrent
markers will observe the pointer when visiting a ThinString and not some
stale data from before String::MakeThin.

Bug: v8:10315
Change-Id: I42db7ba9f44a48c920c997bf1258c561f3dd9bcc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2450066
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70346}

src/heap/heap.cc

@@ -3678,13 +3678,19 @@ class SlotCollectingVisitor final : public ObjectVisitor {
 void Heap::VerifyObjectLayoutChange(HeapObject object, Map new_map) {
   if (!FLAG_verify_heap) return;
 
-  // Check that Heap::NotifyObjectLayout was called for object transitions
+  // Check that Heap::NotifyObjectLayoutChange was called for object transitions
   // that are not safe for concurrent marking.
   // If you see this check triggering for a freshly allocated object,
   // use object->set_map_after_allocation() to initialize its map.
   if (pending_layout_change_object_.is_null()) {
     if (object.IsJSObject()) {
       DCHECK(!object.map().TransitionRequiresSynchronizationWithGC(new_map));
+    } else if (object.IsString() &&
+               (new_map == ReadOnlyRoots(this).thin_string_map() ||
+                new_map == ReadOnlyRoots(this).thin_one_byte_string_map())) {
+      // When transitioning a string to ThinString,
+      // Heap::NotifyObjectLayoutChange doesn't need to be invoked because only
+      // tagged fields are introduced.
     } else {
       // Check that the set of slots before and after the transition match.
       SlotCollectingVisitor old_visitor;

src/objects/string.cc

@@ -117,17 +117,16 @@ void String::MakeThin(Isolate* isolate, String internalized) {
   bool has_pointers = StringShape(*this).IsIndirect();
 
   int old_size = this->Size();
-  // Slot invalidation is not necessary here: ThinString only stores tagged
-  // value, so it can't store an untagged value in a recorded slot.
-  isolate->heap()->NotifyObjectLayoutChange(*this, no_gc,
-                                            InvalidateRecordedSlots::kNo);
   bool one_byte = internalized.IsOneByteRepresentation();
   Handle<Map> map = one_byte ? isolate->factory()->thin_one_byte_string_map()
                              : isolate->factory()->thin_string_map();
+  // Update actual first and then do release store on the map word. This ensures
+  // that the concurrent marker will read the pointer when visiting a
+  // ThinString.
+  ThinString thin = ThinString::unchecked_cast(*this);
+  thin.set_actual(internalized);
   DCHECK_GE(old_size, ThinString::kSize);
   this->synchronized_set_map(*map);
-  ThinString thin = ThinString::cast(*this);
-  thin.set_actual(internalized);
   Address thin_end = thin.address() + ThinString::kSize;
   int size_delta = old_size - ThinString::kSize;
   if (size_delta != 0) {