Remove the stack frame cache to avoid memory leaks

The cache adds a strong pointer from a code object to closures and
thus can leak arbitrary objects.

Bug: chromium:1030043
Tbr: yangguo@chromium.org
Change-Id: I8ce90119fa97eaea59d42e7fae5acd336b5fe5d9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1954392
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65363}

src/api/api.cc

@@ -8800,15 +8800,6 @@ void Isolate::LowMemoryNotification() {
     isolate->heap()->CollectAllAvailableGarbage(
         i::GarbageCollectionReason::kLowMemoryNotification);
   }
-  {
-    i::HeapObjectIterator iterator(isolate->heap());
-    for (i::HeapObject obj = iterator.Next(); !obj.is_null();
-         obj = iterator.Next()) {
-      if (obj.IsAbstractCode()) {
-        i::AbstractCode::cast(obj).DropStackFrameCache();
-      }
-    }
-  }
 }
 
 int Isolate::ContextDisposedNotification(bool dependant_context) {

src/compiler/types.cc

@@ -349,7 +349,6 @@ Type::bitset BitsetType::Lub(const MapRefLike& map) {
     case TUPLE2_TYPE:
     case TUPLE3_TYPE:
     case ENUM_CACHE_TYPE:
-    case SOURCE_POSITION_TABLE_WITH_FRAME_CACHE_TYPE:
     case WASM_CAPI_FUNCTION_DATA_TYPE:
     case WASM_INDIRECT_FUNCTION_TABLE_TYPE:
     case WASM_DEBUG_INFO_TYPE:

src/diagnostics/objects-debug.cc

@@ -295,8 +295,7 @@ void BytecodeArray::BytecodeArrayVerify(Isolate* isolate) {
   VerifyHeapPointer(isolate, constant_pool());
   CHECK(source_position_table().IsUndefined() ||
         source_position_table().IsException() ||
-        source_position_table().IsByteArray() ||
-        source_position_table().IsSourcePositionTableWithFrameCache());
+        source_position_table().IsByteArray());
   CHECK(handler_table().IsByteArray());
 }
 

src/execution/isolate.cc

@@ -529,46 +529,6 @@ StackTraceFailureMessage::StackTraceFailureMessage(Isolate* isolate, void* ptr1,
   }
 }
 
-namespace {
-
-class StackFrameCacheHelper : public AllStatic {
- public:
-  static MaybeHandle<StackTraceFrame> LookupCachedFrame(
-      Isolate* isolate, Handle<AbstractCode> code, int code_offset) {
-    if (FLAG_optimize_for_size) return MaybeHandle<StackTraceFrame>();
-
-    const auto maybe_cache = handle(code->stack_frame_cache(), isolate);
-    if (!maybe_cache->IsSimpleNumberDictionary())
-      return MaybeHandle<StackTraceFrame>();
-
-    const auto cache = Handle<SimpleNumberDictionary>::cast(maybe_cache);
-    const InternalIndex entry = cache->FindEntry(isolate, code_offset);
-    if (entry.is_found()) {
-      return handle(StackTraceFrame::cast(cache->ValueAt(entry)), isolate);
-    }
-    return MaybeHandle<StackTraceFrame>();
-  }
-
-  static void CacheFrameAndUpdateCache(Isolate* isolate,
-                                       Handle<AbstractCode> code,
-                                       int code_offset,
-                                       Handle<StackTraceFrame> frame) {
-    if (FLAG_optimize_for_size) return;
-
-    const auto maybe_cache = handle(code->stack_frame_cache(), isolate);
-    const auto cache = maybe_cache->IsSimpleNumberDictionary()
-                           ? Handle<SimpleNumberDictionary>::cast(maybe_cache)
-                           : SimpleNumberDictionary::New(isolate, 1);
-    Handle<SimpleNumberDictionary> new_cache =
-        SimpleNumberDictionary::Set(isolate, cache, code_offset, frame);
-    if (*new_cache != *cache || !maybe_cache->IsSimpleNumberDictionary()) {
-      AbstractCode::SetStackFrameCache(code, new_cache);
-    }
-  }
-};
-
-}  // anonymous namespace
-
 class FrameArrayBuilder {
  public:
   enum FrameFilterMode { ALL, CURRENT_SECURITY_CONTEXT };
@@ -739,40 +699,16 @@ class FrameArrayBuilder {
   }
 
   // Creates a StackTraceFrame object for each frame in the FrameArray.
-  Handle<FixedArray> GetElementsAsStackTraceFrameArray(
-      bool enable_frame_caching) {
+  Handle<FixedArray> GetElementsAsStackTraceFrameArray() {
     elements_->ShrinkToFit(isolate_);
     const int frame_count = elements_->FrameCount();
     Handle<FixedArray> stack_trace =
         isolate_->factory()->NewFixedArray(frame_count);
 
     for (int i = 0; i < frame_count; ++i) {
-      // Caching stack frames only happens for user JS frames.
-      const bool cache_frame =
-          enable_frame_caching && !isolate_->serializer_enabled() &&
-          !elements_->IsAnyWasmFrame(i) &&
-          elements_->Function(i).shared().IsUserJavaScript();
-      if (cache_frame) {
-        MaybeHandle<StackTraceFrame> maybe_frame =
-            StackFrameCacheHelper::LookupCachedFrame(
-                isolate_, handle(elements_->Code(i), isolate_),
-                Smi::ToInt(elements_->Offset(i)));
-        if (!maybe_frame.is_null()) {
-          Handle<StackTraceFrame> frame = maybe_frame.ToHandleChecked();
-          stack_trace->set(i, *frame);
-          continue;
-        }
-      }
-
       Handle<StackTraceFrame> frame =
           isolate_->factory()->NewStackTraceFrame(elements_, i);
       stack_trace->set(i, *frame);
-
-      if (cache_frame) {
-        StackFrameCacheHelper::CacheFrameAndUpdateCache(
-            isolate_, handle(elements_->Code(i), isolate_),
-            Smi::ToInt(elements_->Offset(i)), frame);
-      }
     }
     return stack_trace;
   }
@@ -989,7 +925,6 @@ struct CaptureStackTraceOptions {
   bool capture_builtin_exit_frames;
   bool capture_only_frames_subject_to_debugging;
   bool async_stack_trace;
-  bool enable_frame_caching;
 };
 
 Handle<Object> CaptureStackTrace(Isolate* isolate, Handle<Object> caller,
@@ -1121,8 +1056,7 @@ Handle<Object> CaptureStackTrace(Isolate* isolate, Handle<Object> caller,
   }
 
   // TODO(yangguo): Queue this structured stack trace for preprocessing on GC.
-  return builder.GetElementsAsStackTraceFrameArray(
-      options.enable_frame_caching);
+  return builder.GetElementsAsStackTraceFrameArray();
 }
 
 }  // namespace
@@ -1140,7 +1074,6 @@ Handle<Object> Isolate::CaptureSimpleStackTrace(Handle<JSReceiver> error_object,
   options.async_stack_trace = FLAG_async_stack_traces;
   options.filter_mode = FrameArrayBuilder::CURRENT_SECURITY_CONTEXT;
   options.capture_only_frames_subject_to_debugging = false;
-  options.enable_frame_caching = false;
 
   return CaptureStackTrace(this, caller, options);
 }
@@ -1236,7 +1169,6 @@ Handle<FixedArray> Isolate::CaptureCurrentStackTrace(
           ? FrameArrayBuilder::ALL
           : FrameArrayBuilder::CURRENT_SECURITY_CONTEXT;
   options.capture_only_frames_subject_to_debugging = true;
-  options.enable_frame_caching = true;
 
   return Handle<FixedArray>::cast(
       CaptureStackTrace(this, factory()->undefined_value(), options));
@@ -2046,7 +1978,6 @@ void Isolate::PrintCurrentStackTrace(FILE* out) {
   options.async_stack_trace = FLAG_async_stack_traces;
   options.filter_mode = FrameArrayBuilder::CURRENT_SECURITY_CONTEXT;
   options.capture_only_frames_subject_to_debugging = false;
-  options.enable_frame_caching = false;
 
   Handle<FixedArray> frames = Handle<FixedArray>::cast(
       CaptureStackTrace(this, this->factory()->undefined_value(), options));

src/heap/factory.cc

@@ -3798,22 +3798,6 @@ Handle<StackFrameInfo> Factory::NewStackFrameInfo(
   return info;
 }
 
-Handle<SourcePositionTableWithFrameCache>
-Factory::NewSourcePositionTableWithFrameCache(
-    Handle<ByteArray> source_position_table,
-    Handle<SimpleNumberDictionary> stack_frame_cache) {
-  Handle<SourcePositionTableWithFrameCache>
-      source_position_table_with_frame_cache =
-          Handle<SourcePositionTableWithFrameCache>::cast(
-              NewStruct(SOURCE_POSITION_TABLE_WITH_FRAME_CACHE_TYPE,
-                        AllocationType::kOld));
-  source_position_table_with_frame_cache->set_source_position_table(
-      *source_position_table);
-  source_position_table_with_frame_cache->set_stack_frame_cache(
-      *stack_frame_cache);
-  return source_position_table_with_frame_cache;
-}
-
 Handle<JSObject> Factory::NewArgumentsObject(Handle<JSFunction> callee,
                                              int length) {
   bool strict_mode_callee = is_strict(callee->shared().language_mode()) ||

src/heap/factory.h

@@ -444,10 +444,6 @@ class V8_EXPORT_PRIVATE Factory {
                                              int index);
   Handle<StackFrameInfo> NewStackFrameInfo(Handle<FrameArray> frame_array,
                                            int index);
-  Handle<SourcePositionTableWithFrameCache>
-  NewSourcePositionTableWithFrameCache(
-      Handle<ByteArray> source_position_table,
-      Handle<SimpleNumberDictionary> stack_frame_cache);
 
   // Allocate various microtasks.
   Handle<CallableTask> NewCallableTask(Handle<JSReceiver> callable,

src/heap/object-stats.cc

@@ -1047,13 +1047,7 @@ void ObjectStatsCollectorImpl::RecordVirtualCodeDetails(Code code) {
   RecordSimpleVirtualObjectStats(code, code.relocation_info(),
                                  ObjectStats::RELOC_INFO_TYPE);
   Object source_position_table = code.source_position_table();
-  if (source_position_table.IsSourcePositionTableWithFrameCache()) {
-    RecordSimpleVirtualObjectStats(
-        code,
-        SourcePositionTableWithFrameCache::cast(source_position_table)
-            .source_position_table(),
-        ObjectStats::SOURCE_POSITION_TABLE_TYPE);
-  } else if (source_position_table.IsHeapObject()) {
+  if (source_position_table.IsHeapObject()) {
     RecordSimpleVirtualObjectStats(code,
                                    HeapObject::cast(source_position_table),
                                    ObjectStats::SOURCE_POSITION_TABLE_TYPE);

src/objects/code-inl.h

@@ -29,7 +29,6 @@ OBJECT_CONSTRUCTORS_IMPL(BytecodeArray, FixedArrayBase)
 OBJECT_CONSTRUCTORS_IMPL(AbstractCode, HeapObject)
 OBJECT_CONSTRUCTORS_IMPL(DependentCode, WeakFixedArray)
 OBJECT_CONSTRUCTORS_IMPL(CodeDataContainer, HeapObject)
-TQ_OBJECT_CONSTRUCTORS_IMPL(SourcePositionTableWithFrameCache)
 
 NEVER_READ_ONLY_SPACE_IMPL(AbstractCode)
 
@@ -64,20 +63,6 @@ ByteArray AbstractCode::source_position_table() {
   }
 }
 
-Object AbstractCode::stack_frame_cache() {
-  Object maybe_table;
-  if (IsCode()) {
-    maybe_table = GetCode().source_position_table();
-  } else {
-    maybe_table = GetBytecodeArray().source_position_table();
-  }
-  if (maybe_table.IsSourcePositionTableWithFrameCache()) {
-    return SourcePositionTableWithFrameCache::cast(maybe_table)
-        .stack_frame_cache();
-  }
-  return Smi::zero();
-}
-
 int AbstractCode::SizeIncludingMetadata() {
   if (IsCode()) {
     return GetCode().SizeIncludingMetadata();
@@ -238,10 +223,8 @@ ByteArray Code::SourcePositionTableIfCollected() const {
 ByteArray Code::SourcePositionTable() const {
   Object maybe_table = source_position_table();
   DCHECK(!maybe_table.IsUndefined() && !maybe_table.IsException());
-  if (maybe_table.IsByteArray()) return ByteArray::cast(maybe_table);
-  DCHECK(maybe_table.IsSourcePositionTableWithFrameCache());
-  return SourcePositionTableWithFrameCache::cast(maybe_table)
-      .source_position_table();
+  DCHECK(maybe_table.IsByteArray());
+  return ByteArray::cast(maybe_table);
 }
 
 Object Code::next_code_link() const {
@@ -720,12 +703,8 @@ ByteArray BytecodeArray::SourcePositionTable() const {
   Object maybe_table = source_position_table();
   if (maybe_table.IsByteArray()) return ByteArray::cast(maybe_table);
   ReadOnlyRoots roots = GetReadOnlyRoots();
-  if (maybe_table.IsException(roots)) return roots.empty_byte_array();
-
-  DCHECK(!maybe_table.IsUndefined(roots));
-  DCHECK(maybe_table.IsSourcePositionTableWithFrameCache());
-  return SourcePositionTableWithFrameCache::cast(maybe_table)
-      .source_position_table();
+  DCHECK(maybe_table.IsException(roots));
+  return roots.empty_byte_array();
 }
 
 ByteArray BytecodeArray::SourcePositionTableIfCollected() const {
@@ -734,16 +713,6 @@ ByteArray BytecodeArray::SourcePositionTableIfCollected() const {
   return SourcePositionTable();
 }
 
-void BytecodeArray::ClearFrameCacheFromSourcePositionTable() {
-  Object maybe_table = source_position_table();
-  if (maybe_table.IsUndefined() || maybe_table.IsByteArray() ||
-      maybe_table.IsException())
-    return;
-  DCHECK(maybe_table.IsSourcePositionTableWithFrameCache());
-  set_source_position_table(SourcePositionTableWithFrameCache::cast(maybe_table)
-                                .source_position_table());
-}
-
 int BytecodeArray::BytecodeArraySize() { return SizeFor(this->length()); }
 
 int BytecodeArray::SizeIncludingMetadata() {

src/objects/code.cc

@@ -154,63 +154,6 @@ Address Code::OffHeapInstructionEnd() const {
          d.InstructionSizeOfBuiltin(builtin_index());
 }
 
-namespace {
-template <typename Code>
-void SetStackFrameCacheCommon(Isolate* isolate, Handle<Code> code,
-                              Handle<SimpleNumberDictionary> cache) {
-  Handle<Object> maybe_table(code->source_position_table(), isolate);
-  if (maybe_table->IsException(isolate) || maybe_table->IsUndefined()) return;
-  if (maybe_table->IsSourcePositionTableWithFrameCache()) {
-    Handle<SourcePositionTableWithFrameCache>::cast(maybe_table)
-        ->set_stack_frame_cache(*cache);
-    return;
-  }
-  DCHECK(maybe_table->IsByteArray());
-  Handle<ByteArray> table(Handle<ByteArray>::cast(maybe_table));
-  Handle<SourcePositionTableWithFrameCache> table_with_cache =
-      isolate->factory()->NewSourcePositionTableWithFrameCache(table, cache);
-  code->set_source_position_table(*table_with_cache);
-}
-}  // namespace
-
-// static
-void AbstractCode::SetStackFrameCache(Handle<AbstractCode> abstract_code,
-                                      Handle<SimpleNumberDictionary> cache) {
-  if (abstract_code->IsCode()) {
-    SetStackFrameCacheCommon(
-        abstract_code->GetIsolate(),
-        handle(abstract_code->GetCode(), abstract_code->GetIsolate()), cache);
-  } else {
-    SetStackFrameCacheCommon(
-        abstract_code->GetIsolate(),
-        handle(abstract_code->GetBytecodeArray(), abstract_code->GetIsolate()),
-        cache);
-  }
-}
-
-namespace {
-template <typename Code>
-void DropStackFrameCacheCommon(Code code) {
-  i::Object maybe_table = code.source_position_table();
-  if (maybe_table.IsUndefined() || maybe_table.IsByteArray() ||
-      maybe_table.IsException()) {
-    return;
-  }
-  DCHECK(maybe_table.IsSourcePositionTableWithFrameCache());
-  code.set_source_position_table(
-      i::SourcePositionTableWithFrameCache::cast(maybe_table)
-          .source_position_table());
-}
-}  // namespace
-
-void AbstractCode::DropStackFrameCache() {
-  if (IsCode()) {
-    DropStackFrameCacheCommon(GetCode());
-  } else {
-    DropStackFrameCacheCommon(GetBytecodeArray());
-  }
-}
-
 int AbstractCode::SourcePosition(int offset) {
   Object maybe_table = source_position_table();
   if (maybe_table.IsException()) return kNoSourcePosition;

src/objects/code.h

@@ -88,8 +88,7 @@ class Code : public HeapObject {
   // [deoptimization_data]: Array containing data for deopt.
   DECL_ACCESSORS(deoptimization_data, FixedArray)
 
-  // [source_position_table]: ByteArray for the source positions table or
-  // SourcePositionTableWithFrameCache.
+  // [source_position_table]: ByteArray for the source positions table.
   DECL_ACCESSORS(source_position_table, Object)
   inline ByteArray SourcePositionTable() const;
   inline ByteArray SourcePositionTableIfCollected() const;
@@ -579,9 +578,6 @@ class AbstractCode : public HeapObject {
   // Return the source position table.
   inline ByteArray source_position_table();
 
-  inline Object stack_frame_cache();
-  static void SetStackFrameCache(Handle<AbstractCode> abstract_code,
-                                 Handle<SimpleNumberDictionary> cache);
   void DropStackFrameCache();
 
   // Returns the size of instructions and the metadata.
@@ -785,7 +781,6 @@ class BytecodeArray : public FixedArrayBase {
   // * empty_byte_array (for bytecode generated for functions that will never
   // have source positions, e.g. native functions).
   // * ByteArray (when source positions have been collected for the bytecode)
-  // * SourcePositionTableWithFrameCache (as above but with a frame cache)
   // * exception (when an error occurred while explicitly collecting source
   // positions for pre-existing bytecode).
   DECL_ACCESSORS(source_position_table, Object)
@@ -799,7 +794,6 @@ class BytecodeArray : public FixedArrayBase {
   inline ByteArray SourcePositionTableIfCollected() const;
   inline bool HasSourcePositionTable() const;
   inline bool DidSourcePositionGenerationFail() const;
-  inline void ClearFrameCacheFromSourcePositionTable();
 
   // Indicates that an attempt was made to collect source positions, but that it
   // failed most likely due to stack exhaustion. When in this state
@@ -941,13 +935,6 @@ class DeoptimizationData : public FixedArray {
   OBJECT_CONSTRUCTORS(DeoptimizationData, FixedArray);
 };
 
-class SourcePositionTableWithFrameCache
-    : public TorqueGeneratedSourcePositionTableWithFrameCache<
-          SourcePositionTableWithFrameCache, Struct> {
- public:
-  TQ_OBJECT_CONSTRUCTORS(SourcePositionTableWithFrameCache)
-};
-
 }  // namespace internal
 }  // namespace v8
 

src/objects/code.tq

@@ -2,20 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-@generatePrint
-@generateCppClass
-extern class SourcePositionTableWithFrameCache extends Struct {
-  source_position_table: ByteArray;
-  stack_frame_cache: Object;
-}
-
 type DependentCode extends WeakFixedArray;
 
 extern class BytecodeArray extends FixedArrayBase {
   // TODO(v8:8983): bytecode array object sizes vary based on their contents.
   constant_pool: FixedArray;
   handler_table: ByteArray;
-  source_position_table: Undefined|ByteArray|SourcePositionTableWithFrameCache;
+  source_position_table: Undefined|ByteArray;
   frame_size: int32;
   parameter_size: int32;
   incoming_new_target_or_generator_register: int32;

src/objects/objects-definitions.h

@@ -136,8 +136,6 @@ namespace internal {
   V(_, PROMISE_REACTION_TYPE, PromiseReaction, promise_reaction)               \
   V(_, PROTOTYPE_INFO_TYPE, PrototypeInfo, prototype_info)                     \
   V(_, SCRIPT_TYPE, Script, script)                                            \
-  V(_, SOURCE_POSITION_TABLE_WITH_FRAME_CACHE_TYPE,                            \
-    SourcePositionTableWithFrameCache, source_position_table_with_frame_cache) \
   V(_, SOURCE_TEXT_MODULE_INFO_ENTRY_TYPE, SourceTextModuleInfoEntry,          \
     module_info_entry)                                                         \
   V(_, STACK_FRAME_INFO_TYPE, StackFrameInfo, stack_frame_info)                \

src/objects/objects.h

@@ -165,7 +165,6 @@
 //       - BreakPointInfo
 //       - StackFrameInfo
 //       - StackTraceFrame
-//       - SourcePositionTableWithFrameCache
 //       - CodeCache
 //       - PrototypeInfo
 //       - Microtask

src/snapshot/code-serializer.cc

@@ -197,11 +197,6 @@ void CodeSerializer::SerializeObject(HeapObject obj) {
   }
 #endif  // V8_TARGET_ARCH_ARM
 
-  if (obj.IsBytecodeArray()) {
-    // Clear the stack frame cache if present
-    BytecodeArray::cast(obj).ClearFrameCacheFromSourcePositionTable();
-  }
-
   // Past this point we should not see any (context-specific) maps anymore.
   CHECK(!obj.IsMap());
   // There should be no references to the global object embedded.