electron/patches/v8/revert_heap_add_checks_position_info.patch

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Keeley Hammond <khammond@slack-corp.com>
Date: Tue, 7 May 2024 15:50:31 -0700
Subject: Revert "[heap] Add checks for ensuring position info slow path is not
 used in heap snapshot"

These two CLs were causing several heap-related tests to fail, and
the issue seems to be under construction in future Chrome rolls.
This patch can be removed when the issue is fixed upstream.

CR Bug: https://issues.chromium.org/issues/42204564

This reverts commit ca51566b53be5507ff184704767c7a9220a68733 and commit
6e70dc9e73af2a930074f4f000864fbc65515b74.

diff --git a/src/common/assert-scope.cc b/src/common/assert-scope.cc
index 0ea1a04fed062658fa36c239d323e70d0358b57d..1b996b14e490a6fe1b3636f9147728f07cca141f 100644
--- a/src/common/assert-scope.cc
+++ b/src/common/assert-scope.cc
@@ -108,8 +108,6 @@ template class PerThreadAssertScope<CODE_DEPENDENCY_CHANGE_ASSERT, true>;
 template class PerThreadAssertScope<CODE_ALLOCATION_ASSERT, false>;
 template class PerThreadAssertScope<CODE_ALLOCATION_ASSERT, true>;
 template class PerThreadAssertScope<GC_MOLE, false>;
-template class PerThreadAssertScope<POSITION_INFO_SLOW_ASSERT, false>;
-template class PerThreadAssertScope<POSITION_INFO_SLOW_ASSERT, true>;
 
 }  // namespace internal
 }  // namespace v8
diff --git a/src/common/assert-scope.h b/src/common/assert-scope.h
index 27fbc04bc880f2ea68dc7a126b8091a3b5ab4025..42d9649c5250c06a4d39ba0306cbdea0b0e5a8c3 100644
--- a/src/common/assert-scope.h
+++ b/src/common/assert-scope.h
@@ -30,7 +30,6 @@ enum PerThreadAssertType {
   CODE_ALLOCATION_ASSERT,
   // Dummy type for disabling GC mole.
   GC_MOLE,
-  POSITION_INFO_SLOW_ASSERT,
 };
 
 template <PerThreadAssertType kType, bool kAllow>
@@ -258,14 +257,6 @@ using DisallowGarbageCollection =
 // work.
 using DisableGCMole = PerThreadAssertScopeDebugOnly<GC_MOLE, false>;
 
-// Scope to ensure slow path for obtaining position info is not called
-using DisallowPositionInfoSlow =
-    PerThreadAssertScopeDebugOnly<POSITION_INFO_SLOW_ASSERT, false>;
-
-// Scope to add an exception to disallowing position info slow path
-using AllowPositionInfoSlow =
-    PerThreadAssertScopeDebugOnly<POSITION_INFO_SLOW_ASSERT, true>;
-
 // The DISALLOW_GARBAGE_COLLECTION macro can be used to define a
 // DisallowGarbageCollection field in classes that isn't present in release
 // builds.
@@ -337,8 +328,6 @@ extern template class PerThreadAssertScope<CODE_DEPENDENCY_CHANGE_ASSERT, true>;
 extern template class PerThreadAssertScope<CODE_ALLOCATION_ASSERT, false>;
 extern template class PerThreadAssertScope<CODE_ALLOCATION_ASSERT, true>;
 extern template class PerThreadAssertScope<GC_MOLE, false>;
-extern template class PerThreadAssertScope<POSITION_INFO_SLOW_ASSERT, false>;
-extern template class PerThreadAssertScope<POSITION_INFO_SLOW_ASSERT, true>;
 
 }  // namespace internal
 }  // namespace v8
diff --git a/src/objects/objects.cc b/src/objects/objects.cc
index d83783a5560a355684a866e1aca2cc081835919b..f8622e1311838df9548bc1839cd40d17af02a7d2 100644
--- a/src/objects/objects.cc
+++ b/src/objects/objects.cc
@@ -4361,7 +4361,6 @@ namespace {
 template <typename Char>
 bool GetPositionInfoSlowImpl(base::Vector<Char> source, int position,
                              Script::PositionInfo* info) {
-  DCHECK(DisallowPositionInfoSlow::IsAllowed());
   if (position < 0) {
     position = 0;
   }
@@ -4408,11 +4407,25 @@ int GetLength(const String::LineEndsVector& vector) {
 }
 
 int GetLength(const Tagged<FixedArray>& array) { return array->length(); }
+}  // namespace
+
+void Script::AddPositionInfoOffset(PositionInfo* info,
+                                   OffsetFlag offset_flag) const {
+  // Add offsets if requested.
+  if (offset_flag == OffsetFlag::kWithOffset) {
+    if (info->line == 0) {
+      info->column += column_offset();
+    }
+    info->line += line_offset();
+  } else {
+    DCHECK_EQ(offset_flag, OffsetFlag::kNoOffset);
+  }
+}
 
 template <typename LineEndsContainer>
-bool GetLineEndsContainerPositionInfo(const LineEndsContainer& ends,
-                                      int position, Script::PositionInfo* info,
-                                      const DisallowGarbageCollection& no_gc) {
+bool Script::GetPositionInfoInternal(
+    const LineEndsContainer& ends, int position, Script::PositionInfo* info,
+    const DisallowGarbageCollection& no_gc) const {
   const int ends_len = GetLength(ends);
   if (ends_len == 0) return false;
 
@@ -4451,31 +4464,6 @@ bool GetLineEndsContainerPositionInfo(const LineEndsContainer& ends,
     info->column = position - info->line_start;
   }
 
-  return true;
-}
-
-}  // namespace
-
-void Script::AddPositionInfoOffset(PositionInfo* info,
-                                   OffsetFlag offset_flag) const {
-  // Add offsets if requested.
-  if (offset_flag == OffsetFlag::kWithOffset) {
-    if (info->line == 0) {
-      info->column += column_offset();
-    }
-    info->line += line_offset();
-  } else {
-    DCHECK_EQ(offset_flag, OffsetFlag::kNoOffset);
-  }
-}
-
-template <typename LineEndsContainer>
-bool Script::GetPositionInfoInternal(
-    const LineEndsContainer& ends, int position, Script::PositionInfo* info,
-    const DisallowGarbageCollection& no_gc) const {
-  if (!GetLineEndsContainerPositionInfo(ends, position, info, no_gc))
-    return false;
-
   // Line end is position of the linebreak character.
   info->line_end = GetLineEnd(ends, info->line);
   if (info->line_end > 0) {
@@ -4544,23 +4532,6 @@ bool Script::GetPositionInfoWithLineEnds(
   return true;
 }
 
-bool Script::GetLineColumnWithLineEnds(
-    int position, int& line, int& column,
-    const String::LineEndsVector& line_ends) {
-  DisallowGarbageCollection no_gc;
-  PositionInfo info;
-  if (!GetLineEndsContainerPositionInfo(line_ends, position, &info, no_gc)) {
-    line = -1;
-    column = -1;
-    return false;
-  }
-
-  line = info.line;
-  column = info.column;
-
-  return true;
-}
-
 int Script::GetColumnNumber(Handle<Script> script, int code_pos) {
   PositionInfo info;
   GetPositionInfo(script, code_pos, &info);
diff --git a/src/objects/script.h b/src/objects/script.h
index bfa0dc0e9b7625636c21620563ef0a19fcf20900..8b89639dd78a80cfae570a4746f391b1359d65a7 100644
--- a/src/objects/script.h
+++ b/src/objects/script.h
@@ -205,9 +205,6 @@ class Script : public TorqueGeneratedScript<Script, Struct> {
   static bool GetPositionInfo(Handle<Script> script, int position,
                               PositionInfo* info,
                               OffsetFlag offset_flag = OffsetFlag::kWithOffset);
-  static bool GetLineColumnWithLineEnds(
-      int position, int& line, int& column,
-      const String::LineEndsVector& line_ends);
   V8_EXPORT_PRIVATE bool GetPositionInfo(
       int position, PositionInfo* info,
       OffsetFlag offset_flag = OffsetFlag::kWithOffset) const;
diff --git a/src/profiler/allocation-tracker.cc b/src/profiler/allocation-tracker.cc
index 4f9e3bb8a5d2b20bba2cc09770872b6ab2a546e8..eac8c082aee0a122282f70956816a25c1ee30112 100644
--- a/src/profiler/allocation-tracker.cc
+++ b/src/profiler/allocation-tracker.cc
@@ -96,7 +96,10 @@ AllocationTracker::FunctionInfo::FunctionInfo()
       function_id(0),
       script_name(""),
       script_id(0),
-      start_position(-1) {}
+      line(-1),
+      column(-1) {
+}
+
 
 void AddressToTraceMap::AddRange(Address start, int size,
                                  unsigned trace_node_id) {
@@ -178,10 +181,23 @@ AllocationTracker::AllocationTracker(HeapObjectsMap* ids, StringsStorage* names)
   function_info_list_.push_back(info);
 }
 
+
 AllocationTracker::~AllocationTracker() {
+  for (UnresolvedLocation* location : unresolved_locations_) delete location;
   for (FunctionInfo* info : function_info_list_) delete info;
 }
 
+
+void AllocationTracker::PrepareForSerialization() {
+  for (UnresolvedLocation* location : unresolved_locations_) {
+    location->Resolve();
+    delete location;
+  }
+  unresolved_locations_.clear();
+  unresolved_locations_.shrink_to_fit();
+}
+
+
 void AllocationTracker::AllocationEvent(Address addr, int size) {
   DisallowGarbageCollection no_gc;
   Heap* heap = ids_->heap();
@@ -235,7 +251,10 @@ unsigned AllocationTracker::AddFunctionInfo(Tagged<SharedFunctionInfo> shared,
         info->script_name = names_->GetName(name);
       }
       info->script_id = script->id();
-      info->start_position = shared->StartPosition();
+      // Converting start offset into line and column may cause heap
+      // allocations so we postpone them until snapshot serialization.
+      unresolved_locations_.push_back(
+          new UnresolvedLocation(script, shared->StartPosition(), info));
     }
     entry->value = reinterpret_cast<void*>(function_info_list_.size());
     function_info_list_.push_back(info);
@@ -255,5 +274,39 @@ unsigned AllocationTracker::functionInfoIndexForVMState(StateTag state) {
   return info_index_for_other_state_;
 }
 
+AllocationTracker::UnresolvedLocation::UnresolvedLocation(Tagged<Script> script,
+                                                          int start,
+                                                          FunctionInfo* info)
+    : start_position_(start), info_(info) {
+  script_ = script->GetIsolate()->global_handles()->Create(script);
+  GlobalHandles::MakeWeak(script_.location(), this, &HandleWeakScript,
+                          v8::WeakCallbackType::kParameter);
+}
+
+AllocationTracker::UnresolvedLocation::~UnresolvedLocation() {
+  if (!script_.is_null()) {
+    GlobalHandles::Destroy(script_.location());
+  }
+}
+
+
+void AllocationTracker::UnresolvedLocation::Resolve() {
+  if (script_.is_null()) return;
+  HandleScope scope(script_->GetIsolate());
+  Script::PositionInfo pos_info;
+  Script::GetPositionInfo(script_, start_position_, &pos_info);
+  info_->line = pos_info.line;
+  info_->column = pos_info.column;
+}
+
+void AllocationTracker::UnresolvedLocation::HandleWeakScript(
+    const v8::WeakCallbackInfo<void>& data) {
+  UnresolvedLocation* loc =
+      reinterpret_cast<UnresolvedLocation*>(data.GetParameter());
+  GlobalHandles::Destroy(loc->script_.location());
+  loc->script_ = Handle<Script>::null();
+}
+
+
 }  // namespace internal
 }  // namespace v8
diff --git a/src/profiler/allocation-tracker.h b/src/profiler/allocation-tracker.h
index d04bb9bf73b4b5725c99dade26510f4964ec0d4b..bb59c92a780160c4168349bfc5d523dffff5684f 100644
--- a/src/profiler/allocation-tracker.h
+++ b/src/profiler/allocation-tracker.h
@@ -104,7 +104,8 @@ class AllocationTracker {
     SnapshotObjectId function_id;
     const char* script_name;
     int script_id;
-    int start_position;
+    int line;
+    int column;
   };
 
   AllocationTracker(HeapObjectsMap* ids, StringsStorage* names);
@@ -112,6 +113,7 @@ class AllocationTracker {
   AllocationTracker(const AllocationTracker&) = delete;
   AllocationTracker& operator=(const AllocationTracker&) = delete;
 
+  V8_EXPORT_PRIVATE void PrepareForSerialization();
   void AllocationEvent(Address addr, int size);
 
   AllocationTraceTree* trace_tree() { return &trace_tree_; }
@@ -125,6 +127,20 @@ class AllocationTracker {
                            SnapshotObjectId id);
   unsigned functionInfoIndexForVMState(StateTag state);
 
+  class UnresolvedLocation {
+   public:
+    UnresolvedLocation(Tagged<Script> script, int start, FunctionInfo* info);
+    ~UnresolvedLocation();
+    void Resolve();
+
+   private:
+    static void HandleWeakScript(const v8::WeakCallbackInfo<void>& data);
+
+    Handle<Script> script_;
+    int start_position_;
+    FunctionInfo* info_;
+  };
+
   static const int kMaxAllocationTraceLength = 64;
   HeapObjectsMap* ids_;
   StringsStorage* names_;
@@ -132,6 +148,7 @@ class AllocationTracker {
   unsigned allocation_trace_buffer_[kMaxAllocationTraceLength];
   std::vector<FunctionInfo*> function_info_list_;
   base::HashMap id_to_function_info_index_;
+  std::vector<UnresolvedLocation*> unresolved_locations_;
   unsigned info_index_for_other_state_;
   AddressToTraceMap address_to_trace_;
 };
diff --git a/src/profiler/heap-snapshot-generator.cc b/src/profiler/heap-snapshot-generator.cc
index 1b7170bd84c29d9e6228e92a8e3ad46d50967a15..417a487bd6767b15bb177394f4273af21bd64883 100644
--- a/src/profiler/heap-snapshot-generator.cc
+++ b/src/profiler/heap-snapshot-generator.cc
@@ -2999,6 +2999,7 @@ bool HeapSnapshotGenerator::GenerateSnapshot() {
   IsolateSafepointScope scope(heap_);
 
   Isolate* isolate = heap_->isolate();
+  v8_heap_explorer_.PopulateLineEnds();
   auto temporary_global_object_tags =
       v8_heap_explorer_.CollectTemporaryGlobalObjectsTags();
 
@@ -3010,7 +3011,6 @@ bool HeapSnapshotGenerator::GenerateSnapshot() {
   // DisallowGarbageCollection scope as the HeapObjectIterator used during
   // snapshot creation enters a safepoint as well. However, in practice we
   // already enter a safepoint above so that should never trigger a GC.
-  DisallowPositionInfoSlow no_position_info_slow;
 
   NullContextForSnapshotScope null_context_scope(isolate);
 
@@ -3021,7 +3021,6 @@ bool HeapSnapshotGenerator::GenerateSnapshot() {
 
   snapshot_->AddSyntheticRootEntries();
 
-  v8_heap_explorer_.PopulateLineEnds();
   if (!FillReferences()) return false;
 
   snapshot_->FillChildren();
@@ -3092,9 +3091,12 @@ bool HeapSnapshotGenerator::FillReferences() {
 const int HeapSnapshotJSONSerializer::kNodeFieldsCount = 7;
 
 void HeapSnapshotJSONSerializer::Serialize(v8::OutputStream* stream) {
-  DisallowHeapAllocation no_heap_allocation;
   v8::base::ElapsedTimer timer;
   timer.Start();
+  if (AllocationTracker* allocation_tracker =
+          snapshot_->profiler()->allocation_tracker()) {
+    allocation_tracker->PrepareForSerialization();
+  }
   DCHECK_NULL(writer_);
   writer_ = new OutputStreamWriter(stream);
   SerializeImpl();
@@ -3452,17 +3454,10 @@ void HeapSnapshotJSONSerializer::SerializeTraceNodeInfos() {
     // The cast is safe because script id is a non-negative Smi.
     buffer_pos =
         utoa(static_cast<unsigned>(info->script_id), buffer, buffer_pos);
-
-    auto& line_ends = snapshot_->GetScriptLineEnds(info->script_id);
-    int line = -1;
-    int column = -1;
-    Script::GetLineColumnWithLineEnds(info->start_position, line, column,
-                                      line_ends);
-
     buffer[buffer_pos++] = ',';
-    buffer_pos = SerializePosition(line, buffer, buffer_pos);
+    buffer_pos = SerializePosition(info->line, buffer, buffer_pos);
     buffer[buffer_pos++] = ',';
-    buffer_pos = SerializePosition(column, buffer, buffer_pos);
+    buffer_pos = SerializePosition(info->column, buffer, buffer_pos);
     buffer[buffer_pos++] = '\n';
     buffer[buffer_pos++] = '\0';
     writer_->AddString(buffer.begin());
diff --git a/test/cctest/test-heap-profiler.cc b/test/cctest/test-heap-profiler.cc
index 4885cab74f95378a9ff884d58a8c089624985a8f..f1e869f28e00c0449e85b00f3f4626e0941e4c43 100644
--- a/test/cctest/test-heap-profiler.cc
+++ b/test/cctest/test-heap-profiler.cc
@@ -2841,6 +2841,8 @@ TEST(ArrayGrowLeftTrim) {
   AllocationTracker* tracker =
       reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
   CHECK(tracker);
+  // Resolve all function locations.
+  tracker->PrepareForSerialization();
   // Print for better diagnostics in case of failure.
   tracker->trace_tree()->Print(tracker);
 
@@ -2863,6 +2865,8 @@ TEST(TrackHeapAllocationsWithInlining) {
   AllocationTracker* tracker =
       reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
   CHECK(tracker);
+  // Resolve all function locations.
+  tracker->PrepareForSerialization();
   // Print for better diagnostics in case of failure.
   tracker->trace_tree()->Print(tracker);
 
@@ -2895,6 +2899,8 @@ TEST(TrackHeapAllocationsWithoutInlining) {
   AllocationTracker* tracker =
       reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
   CHECK(tracker);
+  // Resolve all function locations.
+  tracker->PrepareForSerialization();
   // Print for better diagnostics in case of failure.
   tracker->trace_tree()->Print(tracker);
 
@@ -2943,6 +2949,8 @@ TEST(TrackBumpPointerAllocations) {
     AllocationTracker* tracker =
         reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
     CHECK(tracker);
+    // Resolve all function locations.
+    tracker->PrepareForSerialization();
     // Print for better diagnostics in case of failure.
     tracker->trace_tree()->Print(tracker);
 
@@ -2967,6 +2975,8 @@ TEST(TrackBumpPointerAllocations) {
     AllocationTracker* tracker =
         reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
     CHECK(tracker);
+    // Resolve all function locations.
+    tracker->PrepareForSerialization();
     // Print for better diagnostics in case of failure.
     tracker->trace_tree()->Print(tracker);
 
@@ -2994,6 +3004,8 @@ TEST(TrackV8ApiAllocation) {
   AllocationTracker* tracker =
       reinterpret_cast<i::HeapProfiler*>(heap_profiler)->allocation_tracker();
   CHECK(tracker);
+  // Resolve all function locations.
+  tracker->PrepareForSerialization();
   // Print for better diagnostics in case of failure.
   tracker->trace_tree()->Print(tracker);