[debug] Hold on to promises weakly from the debugger's promise stack.

The debugger maintains a stack of promises used for catch prediction
with promise builtins and async functions. Previously this stack would
hold on to the individual promises strongly, and subtle bugs that lead
to not properly cleaning up the stack in some corner cases would often
lead to significant memory issues (e.g. leaking whole iframes).

This refactors the PromiseOnStack to be

  (a) on the V8 heap, rather than allocating C++ structs with global
      handles pointing to the promises, and
  (b) hold on to the promises only weakly.

While this will not guarantee proper promise stack management, it will
at least ensure that edge cases don't lead to catastrophic (debugger
only) leaks.

Bug: chromium:1292063
Change-Id: I9c293ca2032de3a59e1e9624f132d37187805567
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3545176
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79594}

src/debug/debug.cc

@@ -390,6 +390,7 @@ void Debug::ThreadInit() {
                       static_cast<base::AtomicWord>(0));
   thread_local_.break_on_next_function_call_ = false;
   UpdateHookOnFunctionCall();
+  thread_local_.promise_stack_ = Smi::zero();
 }
 
 char* Debug::ArchiveDebug(char* storage) {
@@ -448,6 +449,8 @@ void Debug::Iterate(RootVisitor* v, ThreadLocal* thread_local_data) {
   v->VisitRootPointer(
       Root::kDebug, nullptr,
       FullObjectSlot(&thread_local_data->ignore_step_into_function_));
+  v->VisitRootPointer(Root::kDebug, nullptr,
+                      FullObjectSlot(&thread_local_data->promise_stack_));
 }
 
 DebugInfoListNode::DebugInfoListNode(Isolate* isolate, DebugInfo debug_info)
@@ -469,7 +472,6 @@ void Debug::Unload() {
   ClearStepping();
   RemoveAllCoverageInfos();
   ClearAllDebuggerHints();
-  ClearGlobalPromiseStack();
   debug_delegate_ = nullptr;
 }
 
@@ -2067,11 +2069,6 @@ void Debug::FreeDebugInfoListNode(DebugInfoListNode* prev,
   delete node;
 }
 
-void Debug::ClearGlobalPromiseStack() {
-  while (isolate_->PopPromise()) {
-  }
-}
-
 bool Debug::IsBreakAtReturn(JavaScriptFrame* frame) {
   RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger);
   HandleScope scope(isolate_);

src/debug/debug.h

@@ -490,8 +490,6 @@ class V8_EXPORT_PRIVATE Debug {
                      DebugInfoListNode** curr);
   void FreeDebugInfoListNode(DebugInfoListNode* prev, DebugInfoListNode* node);
 
-  void ClearGlobalPromiseStack();
-
   void SetTemporaryObjectTrackingDisabled(bool disabled);
   bool GetTemporaryObjectTrackingDisabled() const;
 
@@ -569,6 +567,10 @@ class V8_EXPORT_PRIVATE Debug {
     // This flag is true when SetBreakOnNextFunctionCall is called and it forces
     // debugger to break on next function call.
     bool break_on_next_function_call_;
+
+    // Throwing an exception may cause a Promise rejection.  For this purpose
+    // we keep track of a stack of nested promises.
+    Object promise_stack_;
   };
 
   static void Iterate(RootVisitor* v, ThreadLocal* thread_local_data);

src/execution/isolate.cc

@@ -2600,21 +2600,22 @@ bool Isolate::OptionalRescheduleException(bool clear_exception) {
 }
 
 void Isolate::PushPromise(Handle<JSObject> promise) {
-  ThreadLocalTop* tltop = thread_local_top();
-  PromiseOnStack* prev = tltop->promise_on_stack_;
-  Handle<JSObject> global_promise = global_handles()->Create(*promise);
-  tltop->promise_on_stack_ = new PromiseOnStack(global_promise, prev);
-}
-
-bool Isolate::PopPromise() {
-  ThreadLocalTop* tltop = thread_local_top();
-  if (tltop->promise_on_stack_ == nullptr) return false;
-  PromiseOnStack* prev = tltop->promise_on_stack_->prev();
-  Handle<Object> global_promise = tltop->promise_on_stack_->promise();
-  delete tltop->promise_on_stack_;
-  tltop->promise_on_stack_ = prev;
-  global_handles()->Destroy(global_promise.location());
-  return true;
+  Handle<Object> promise_on_stack(debug()->thread_local_.promise_stack_, this);
+  promise_on_stack = factory()->NewPromiseOnStack(promise_on_stack, promise);
+  debug()->thread_local_.promise_stack_ = *promise_on_stack;
+}
+
+void Isolate::PopPromise() {
+  if (!IsPromiseStackEmpty()) {
+    debug()->thread_local_.promise_stack_ =
+        PromiseOnStack::cast(debug()->thread_local_.promise_stack_).prev();
+  }
+}
+
+bool Isolate::IsPromiseStackEmpty() const {
+  DCHECK_IMPLIES(!debug()->thread_local_.promise_stack_.IsSmi(),
+                 debug()->thread_local_.promise_stack_.IsPromiseOnStack());
+  return debug()->thread_local_.promise_stack_.IsSmi();
 }
 
 namespace {
@@ -2688,15 +2689,14 @@ bool Isolate::PromiseHasUserDefinedRejectHandler(Handle<JSPromise> promise) {
 
 Handle<Object> Isolate::GetPromiseOnStackOnThrow() {
   Handle<Object> undefined = factory()->undefined_value();
-  ThreadLocalTop* tltop = thread_local_top();
-  if (tltop->promise_on_stack_ == nullptr) return undefined;
+  if (IsPromiseStackEmpty()) return undefined;
   // Find the top-most try-catch or try-finally handler.
   CatchType prediction = PredictExceptionCatcher();
   if (prediction == NOT_CAUGHT || prediction == CAUGHT_BY_EXTERNAL) {
     return undefined;
   }
   Handle<Object> retval = undefined;
-  PromiseOnStack* promise_on_stack = tltop->promise_on_stack_;
+  Handle<Object> promise_stack(debug()->thread_local_.promise_stack_, this);
   for (StackFrameIterator it(this); !it.done(); it.Advance()) {
     StackFrame* frame = it.frame();
     HandlerTable::CatchPrediction catch_prediction;
@@ -2728,10 +2728,16 @@ Handle<Object> Isolate::GetPromiseOnStackOnThrow() {
           Handle<JSPromise>::cast(retval)->set_handled_hint(true);
         }
         return retval;
-      case HandlerTable::PROMISE:
-        return promise_on_stack
-                   ? Handle<Object>::cast(promise_on_stack->promise())
-                   : undefined;
+      case HandlerTable::PROMISE: {
+        Handle<JSObject> promise;
+        if (promise_stack->IsPromiseOnStack() &&
+            PromiseOnStack::GetPromise(
+                Handle<PromiseOnStack>::cast(promise_stack))
+                .ToHandle(&promise)) {
+          return promise;
+        }
+        return undefined;
+      }
       case HandlerTable::UNCAUGHT_ASYNC_AWAIT:
       case HandlerTable::ASYNC_AWAIT: {
         // If in the initial portion of async/await, continue the loop to pop up
@@ -2739,15 +2745,21 @@ Handle<Object> Isolate::GetPromiseOnStackOnThrow() {
         // dependents is found, or a non-async stack frame is encountered, in
         // order to handle the synchronous async/await catch prediction case:
         // assume that async function calls are awaited.
-        if (!promise_on_stack) return retval;
-        retval = promise_on_stack->promise();
+        if (!promise_stack->IsPromiseOnStack()) {
+          return retval;
+        }
+        Handle<PromiseOnStack> promise_on_stack =
+            Handle<PromiseOnStack>::cast(promise_stack);
+        if (!PromiseOnStack::GetPromise(promise_on_stack).ToHandle(&retval)) {
+          return retval;
+        }
         if (retval->IsJSPromise()) {
           if (PromiseHasUserDefinedRejectHandler(
                   Handle<JSPromise>::cast(retval))) {
             return retval;
           }
         }
-        promise_on_stack = promise_on_stack->prev();
+        promise_stack = handle(promise_on_stack->prev(), this);
         continue;
       }
     }
@@ -5039,8 +5051,7 @@ void Isolate::OnTerminationDuringRunMicrotasks() {
   heap()->set_current_microtask(ReadOnlyRoots(this).undefined_value());
 
   // Empty the promise stack.
-  while (PopPromise()) {
-  }
+  debug()->thread_local_.promise_stack_ = Smi::zero();
 
   if (current_microtask->IsPromiseReactionJobTask()) {
     Handle<PromiseReactionJobTask> promise_reaction_job_task =

src/execution/isolate.h

@@ -859,7 +859,8 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
 
   // Push and pop a promise and the current try-catch handler.
   void PushPromise(Handle<JSObject> promise);
-  bool PopPromise();
+  void PopPromise();
+  bool IsPromiseStackEmpty() const;
 
   // Return the relevant Promise that a throw/rejection pertains to, based
   // on the contents of the Promise stack
@@ -2410,18 +2411,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
 #undef FIELD_ACCESSOR
 #undef THREAD_LOCAL_TOP_ACCESSOR
 
-class PromiseOnStack {
- public:
-  PromiseOnStack(Handle<JSObject> promise, PromiseOnStack* prev)
-      : promise_(promise), prev_(prev) {}
-  Handle<JSObject> promise() { return promise_; }
-  PromiseOnStack* prev() { return prev_; }
-
- private:
-  Handle<JSObject> promise_;
-  PromiseOnStack* prev_;
-};
-
 // SaveContext scopes save the current context on the Isolate on creation, and
 // restore it on destruction.
 class V8_EXPORT_PRIVATE SaveContext {

src/execution/thread-local-top.cc

@@ -27,7 +27,6 @@ void ThreadLocalTop::Clear() {
   c_entry_fp_ = kNullAddress;
   handler_ = kNullAddress;
   c_function_ = kNullAddress;
-  promise_on_stack_ = nullptr;
   simulator_ = nullptr;
   js_entry_sp_ = kNullAddress;
   external_callback_scope_ = nullptr;
@@ -57,10 +56,7 @@ void ThreadLocalTop::Initialize(Isolate* isolate) {
 #endif
 }
 
-void ThreadLocalTop::Free() {
-  // Match unmatched PopPromise calls.
-  while (promise_on_stack_) isolate_->PopPromise();
-}
+void ThreadLocalTop::Free() {}
 
 #if defined(USE_SIMULATOR)
 void ThreadLocalTop::StoreCurrentStackPosition() {

src/execution/thread-local-top.h

@@ -26,7 +26,6 @@ namespace internal {
 class EmbedderState;
 class ExternalCallbackScope;
 class Isolate;
-class PromiseOnStack;
 class Simulator;
 
 class ThreadLocalTop {
@@ -35,9 +34,9 @@ class ThreadLocalTop {
   // refactor this to really consist of just Addresses and 32-bit
   // integer fields.
 #ifdef V8_ENABLE_CONSERVATIVE_STACK_SCANNING
-  static constexpr uint32_t kSizeInBytes = 27 * kSystemPointerSize;
-#else
   static constexpr uint32_t kSizeInBytes = 26 * kSystemPointerSize;
+#else
+  static constexpr uint32_t kSizeInBytes = 25 * kSystemPointerSize;
 #endif
 
   // Does early low-level initialization that does not depend on the
@@ -140,11 +139,6 @@ class ThreadLocalTop {
   // C function that was called at c entry.
   Address c_function_;
 
-  // Throwing an exception may cause a Promise rejection.  For this purpose
-  // we keep track of a stack of nested promises and the corresponding
-  // try-catch handlers.
-  PromiseOnStack* promise_on_stack_;
-
   // Simulator field is always present to get predictable layout.
   Simulator* simulator_;
 

src/heap/factory.cc

@@ -3460,6 +3460,16 @@ Handle<StackFrameInfo> Factory::NewStackFrameInfo(
   return handle(info, isolate());
 }
 
+Handle<PromiseOnStack> Factory::NewPromiseOnStack(Handle<Object> prev,
+                                                  Handle<JSObject> promise) {
+  PromiseOnStack promise_on_stack = NewStructInternal<PromiseOnStack>(
+      PROMISE_ON_STACK_TYPE, AllocationType::kYoung);
+  DisallowGarbageCollection no_gc;
+  promise_on_stack.set_prev(*prev, SKIP_WRITE_BARRIER);
+  promise_on_stack.set_promise(*MaybeObjectHandle::Weak(promise));
+  return handle(promise_on_stack, isolate());
+}
+
 Handle<JSObject> Factory::NewArgumentsObject(Handle<JSFunction> callee,
                                              int length) {
   bool strict_mode_callee = is_strict(callee->shared().language_mode()) ||

src/heap/factory.h

@@ -407,6 +407,9 @@ class V8_EXPORT_PRIVATE Factory : public FactoryBase<Factory> {
       int bytecode_offset_or_source_position, Handle<String> function_name,
       bool is_constructor);
 
+  Handle<PromiseOnStack> NewPromiseOnStack(Handle<Object> prev,
+                                           Handle<JSObject> promise);
+
   // Allocate various microtasks.
   Handle<CallableTask> NewCallableTask(Handle<JSReceiver> callable,
                                        Handle<Context> context);

src/heap/objects-visiting.h

@@ -41,6 +41,7 @@ namespace internal {
   V(Map)                                \
   V(NativeContext)                      \
   V(PreparseData)                       \
+  V(PromiseOnStack)                     \
   V(PropertyArray)                      \
   V(PropertyCell)                       \
   V(PrototypeInfo)                      \

src/objects/debug-objects-inl.h

@@ -85,6 +85,9 @@ ACCESSORS_RELAXED_CHECKED(ErrorStackData, call_site_infos, FixedArray,
                           kCallSiteInfosOrFormattedStackOffset,
                           !HasFormattedStack())
 
+NEVER_READ_ONLY_SPACE_IMPL(PromiseOnStack)
+TQ_OBJECT_CONSTRUCTORS_IMPL(PromiseOnStack)
+
 }  // namespace internal
 }  // namespace v8
 

src/objects/debug-objects.cc

@@ -457,5 +457,16 @@ void ErrorStackData::EnsureStackFrameInfos(Isolate* isolate,
   error_stack->set_limit_or_stack_frame_infos(*stack_frame_infos);
 }
 
+// static
+MaybeHandle<JSObject> PromiseOnStack::GetPromise(
+    Handle<PromiseOnStack> promise_on_stack) {
+  HeapObject promise;
+  Isolate* isolate = promise_on_stack->GetIsolate();
+  if (promise_on_stack->promise()->GetHeapObjectIfWeak(isolate, &promise)) {
+    return handle(JSObject::cast(promise), isolate);
+  }
+  return {};
+}
+
 }  // namespace internal
 }  // namespace v8

src/objects/debug-objects.h

@@ -251,6 +251,19 @@ class ErrorStackData
   TQ_OBJECT_CONSTRUCTORS(ErrorStackData)
 };
 
+class PromiseOnStack
+    : public TorqueGeneratedPromiseOnStack<PromiseOnStack, Struct> {
+ public:
+  NEVER_READ_ONLY_SPACE
+
+  static MaybeHandle<JSObject> GetPromise(
+      Handle<PromiseOnStack> promise_on_stack);
+
+  class BodyDescriptor;
+
+  TQ_OBJECT_CONSTRUCTORS(PromiseOnStack)
+};
+
 }  // namespace internal
 }  // namespace v8
 

src/objects/debug-objects.tq

@@ -127,3 +127,8 @@ extern class ErrorStackData extends Struct {
   // stack traces.
   limit_or_stack_frame_infos: Smi|FixedArray;
 }
+
+extern class PromiseOnStack extends Struct {
+  prev: PromiseOnStack|Zero;
+  promise: Weak<JSObject>;
+}

src/objects/map.cc

@@ -348,6 +348,9 @@ VisitorId Map::GetVisitorId(Map map) {
 #define MAKE_STRUCT_CASE(TYPE, Name, name) case TYPE:
       STRUCT_LIST(MAKE_STRUCT_CASE)
 #undef MAKE_STRUCT_CASE
+      if (instance_type == PROMISE_ON_STACK_TYPE) {
+        return kVisitPromiseOnStack;
+      }
       if (instance_type == PROTOTYPE_INFO_TYPE) {
         return kVisitPrototypeInfo;
       }

src/objects/map.h

@@ -57,6 +57,7 @@ enum InstanceType : uint16_t;
   V(Map)                                \
   V(NativeContext)                      \
   V(PreparseData)                       \
+  V(PromiseOnStack)                     \
   V(PropertyArray)                      \
   V(PropertyCell)                       \
   V(PrototypeInfo)                      \

src/objects/objects-body-descriptors-inl.h

@@ -606,6 +606,25 @@ class PreparseData::BodyDescriptor final : public BodyDescriptorBase {
   }
 };
 
+class PromiseOnStack::BodyDescriptor final : public BodyDescriptorBase {
+ public:
+  static bool IsValidSlot(Map map, HeapObject obj, int offset) {
+    return offset >= HeapObject::kHeaderSize;
+  }
+
+  template <typename ObjectVisitor>
+  static inline void IterateBody(Map map, HeapObject obj, int object_size,
+                                 ObjectVisitor* v) {
+    IteratePointers(obj, Struct::kHeaderSize, kPromiseOffset, v);
+    IterateMaybeWeakPointer(obj, kPromiseOffset, v);
+    STATIC_ASSERT(kPromiseOffset + kTaggedSize == kHeaderSize);
+  }
+
+  static inline int SizeOf(Map map, HeapObject obj) {
+    return obj.SizeFromMap(map);
+  }
+};
+
 class PrototypeInfo::BodyDescriptor final : public BodyDescriptorBase {
  public:
   static bool IsValidSlot(Map map, HeapObject obj, int offset) {

src/objects/objects-definitions.h

@@ -151,6 +151,7 @@ namespace internal {
   V(_, INTERPRETER_DATA_TYPE, InterpreterData, interpreter_data)               \
   V(_, MODULE_REQUEST_TYPE, ModuleRequest, module_request)                     \
   V(_, PROMISE_CAPABILITY_TYPE, PromiseCapability, promise_capability)         \
+  V(_, PROMISE_ON_STACK_TYPE, PromiseOnStack, promise_on_stack)                \
   V(_, PROMISE_REACTION_TYPE, PromiseReaction, promise_reaction)               \
   V(_, PROPERTY_DESCRIPTOR_OBJECT_TYPE, PropertyDescriptorObject,              \
     property_descriptor_object)                                                \

src/objects/objects.h

@@ -183,6 +183,7 @@
 //       - CallSiteInfo
 //       - CodeCache
 //       - PropertyDescriptorObject
+//       - PromiseOnStack
 //       - PrototypeInfo
 //       - Microtask
 //         - CallbackTask

test/cctest/test-debug.cc

@@ -5764,7 +5764,7 @@ TEST(AwaitCleansUpGlobalPromiseStack) {
       "})();\n");
   CompileRun(source);
 
-  CHECK_EQ(CcTest::i_isolate()->thread_local_top()->promise_on_stack_, nullptr);
+  CHECK(CcTest::i_isolate()->IsPromiseStackEmpty());
 
   v8::debug::SetDebugDelegate(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded();