[string] Fix stale pointer crash in String.p.split

ToDirectStringAssembler::PointerToData returns a raw pointer, which
is invalidated when GC moves the original string and hence must not
be accessed after any allocations. This fixes the bug introduced in
b4ebbc57 / r53260.

Bug: chromium:845060
Tbr: jgruber@chromium.org
Change-Id: I248d0dd2a275bf9308269b3f65d00c4c4c3d4292
Reviewed-on: https://chromium-review.googlesource.com/1068213
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53272}

src/builtins/builtins-string-gen.cc

@@ -1804,7 +1804,8 @@ TNode<JSArray> StringBuiltinsAssembler::StringToArray(
     TNode<Smi> subject_length, TNode<Number> limit_number) {
   CSA_ASSERT(this, SmiGreaterThan(subject_length, SmiConstant(0)));
 
-  Label done(this), call_runtime(this, Label::kDeferred);
+  Label done(this), call_runtime(this, Label::kDeferred),
+      fill_thehole_and_call_runtime(this, Label::kDeferred);
   TVARIABLE(JSArray, result_array);
 
   TNode<Int32T> instance_type = LoadInstanceType(subject_string);
@@ -1820,18 +1821,22 @@ TNode<JSArray> StringBuiltinsAssembler::StringToArray(
 
     ToDirectStringAssembler to_direct(state(), subject_string);
     to_direct.TryToDirect(&call_runtime);
-    TNode<RawPtrT> string_data =
-        UncheckedCast<RawPtrT>(to_direct.PointerToData(&call_runtime));
-    TNode<IntPtrT> string_data_offset = to_direct.offset();
-    TNode<Object> cache = LoadRoot(Heap::kSingleCharacterStringCacheRootIndex);
-
-    Label fill_thehole_and_call_runtime(this, Label::kDeferred);
     TNode<FixedArray> elements =
         AllocateFixedArray(PACKED_ELEMENTS, length_smi,
                            AllocationFlag::kAllowLargeObjectAllocation);
+    // Don't allocate anything while {string_data} is live!
+    TNode<RawPtrT> string_data = UncheckedCast<RawPtrT>(
+        to_direct.PointerToData(&fill_thehole_and_call_runtime));
+    TNode<IntPtrT> string_data_offset = to_direct.offset();
+    TNode<Object> cache = LoadRoot(Heap::kSingleCharacterStringCacheRootIndex);
+
     BuildFastLoop(
         IntPtrConstant(0), length,
         [&](Node* index) {
+          // TODO(jkummerow): Implement a CSA version of DisallowHeapAllocation
+          // and use that to guard ToDirectStringAssembler.PointerToData().
+          CSA_ASSERT(this, WordEqual(to_direct.PointerToData(&call_runtime),
+                                     string_data));
           TNode<Int32T> char_code =
               UncheckedCast<Int32T>(Load(MachineType::Uint8(), string_data,
                                          IntPtrAdd(index, string_data_offset)));

test/cctest/heap/heap-tester.h

@@ -34,15 +34,16 @@
   V(StressHandles)                                        \
   V(TestMemoryReducerSampleJsCalls)                       \
   V(TestSizeOfObjects)                                    \
-  V(Regress587004)                                        \
+  V(Regress5831)                                          \
   V(Regress538257)                                        \
+  V(Regress587004)                                        \
   V(Regress589413)                                        \
   V(Regress658718)                                        \
   V(Regress670675)                                        \
-  V(Regress5831)                                          \
   V(Regress777177)                                        \
-  V(Regress791582)                                        \
   V(Regress779503)                                        \
+  V(Regress791582)                                        \
+  V(Regress845060)                                        \
   V(RegressMissingWriteBarrierInAllocate)                 \
   V(WriteBarriersInCopyJSObject)
 

test/cctest/heap/test-heap.cc

@@ -2183,6 +2183,33 @@ HEAP_TEST(GCFlags) {
   CHECK_EQ(Heap::kNoGCFlags, heap->current_gc_flags_);
 }
 
+HEAP_TEST(Regress845060) {
+  // Regression test for crbug.com/845060, where a raw pointer to a string's
+  // data was kept across an allocation. If the allocation causes GC and
+  // moves the string, such raw pointers become invalid.
+  FLAG_allow_natives_syntax = true;
+  FLAG_stress_incremental_marking = false;
+  FLAG_stress_compaction = false;
+  CcTest::InitializeVM();
+  LocalContext context;
+  v8::HandleScope scope(CcTest::isolate());
+  Heap* heap = CcTest::heap();
+
+  // Preparation: create a string in new space.
+  Local<Value> str = CompileRun("var str = (new Array(10000)).join('x'); str");
+  CHECK(heap->InNewSpace(*v8::Utils::OpenHandle(*str)));
+
+  // Idle incremental marking sets the "kReduceMemoryFootprint" flag, which
+  // causes from_space to be unmapped after scavenging.
+  heap->StartIdleIncrementalMarking(GarbageCollectionReason::kTesting);
+  CHECK(heap->ShouldReduceMemory());
+
+  // Run the test (which allocates results) until the original string was
+  // promoted to old space. Unmapping of from_space causes accesses to any
+  // stale raw pointers to crash.
+  CompileRun("while (%InNewSpace(str)) { str.split(''); }");
+  CHECK(!heap->InNewSpace(*v8::Utils::OpenHandle(*str)));
+}
 
 TEST(IdleNotificationFinishMarking) {
   if (!FLAG_incremental_marking) return;