[wasm][interpreter] Fix memory leak with the reference stack

This CL fixes a memory leak in the interpreter. The leak was caused by
a cycle the object graph that was rooted with a global object. The
cycle was the following:

A global handle, owned by the interpreter -> reference stack of the
Interpreter -> ref.func element (WasmExportedFunction) ->
WasmInstanceObject -> WasmDebugInfo -> InterpreterHandle -> Interpreter

With this CL we get rid of the global handle. Instead we store the stack
in the WasmDebugInfo. We then have to load the reference stack every time
we enter the Interpreter and want access the reference stack.

R=mstarzinger@chromium.org

Bug: chromium:1000610
Change-Id: If8995725f7ec35862b2f99a07582c861027daaf1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1800582
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63953}

src/builtins/base.tq

@@ -1153,6 +1153,7 @@ extern class WasmIndirectFunctionTable extends Struct {
 extern class WasmDebugInfo extends Struct {
   instance: WasmInstanceObject;
   interpreter_handle: Foreign | Undefined;
+  interpreter_reference_stack: Cell;
   locals_names: FixedArray | Undefined;
   c_wasm_entries: FixedArray | Undefined;
   c_wasm_entry_map: Foreign | Undefined;  // Managed<wasm::SignatureMap>

src/wasm/wasm-debug.cc

@@ -503,9 +503,11 @@ wasm::InterpreterHandle* GetInterpreterHandleOrNull(WasmDebugInfo debug_info) {
 Handle<WasmDebugInfo> WasmDebugInfo::New(Handle<WasmInstanceObject> instance) {
   DCHECK(!instance->has_debug_info());
   Factory* factory = instance->GetIsolate()->factory();
+  Handle<Cell> stack_cell = factory->NewCell(factory->empty_fixed_array());
   Handle<WasmDebugInfo> debug_info = Handle<WasmDebugInfo>::cast(
       factory->NewStruct(WASM_DEBUG_INFO_TYPE, AllocationType::kOld));
   debug_info->set_wasm_instance(*instance);
+  debug_info->set_interpreter_reference_stack(*stack_cell);
   instance->set_debug_info(*debug_info);
   return debug_info;
 }

src/wasm/wasm-interpreter.cc

@@ -1128,13 +1128,41 @@ class ThreadImpl {
   };
 
  public:
+  // The {ReferenceStackScope} sets up the reference stack in the interpreter.
+  // The handle to the reference stack has to be re-initialized everytime we
+  // call into the interpreter because there is no HandleScope that could
+  // contain that handle. A global handle is not an option because it can lead
+  // to a memory leak if a reference to the {WasmInstanceObject} is put onto the
+  // reference stack and thereby transitively keeps the interpreter alive.
+  class ReferenceStackScope {
+   public:
+    explicit ReferenceStackScope(ThreadImpl* impl) : impl_(impl) {
+      // The reference stack is already initialized, we don't have to do
+      // anything.
+      if (!impl_->reference_stack_cell_.is_null()) return;
+      impl_->reference_stack_cell_ = handle(
+          impl_->instance_object_->debug_info().interpreter_reference_stack(),
+          impl_->isolate_);
+      // We initialized the reference stack, so we also have to reset it later.
+      do_reset_stack_ = true;
+    }
+
+    ~ReferenceStackScope() {
+      if (do_reset_stack_) {
+        impl_->reference_stack_cell_ = Handle<Cell>();
+      }
+    }
+
+   private:
+    ThreadImpl* impl_;
+    bool do_reset_stack_ = false;
+  };
+
   ThreadImpl(Zone* zone, CodeMap* codemap,
-             Handle<WasmInstanceObject> instance_object,
-             Handle<Cell> reference_stack_cell)
+             Handle<WasmInstanceObject> instance_object)
       : codemap_(codemap),
         isolate_(instance_object->GetIsolate()),
         instance_object_(instance_object),
-        reference_stack_cell_(reference_stack_cell),
         frames_(zone),
         activations_(zone) {}
 
@@ -1394,6 +1422,7 @@ class ThreadImpl {
   };
 
   friend class InterpretedFrameImpl;
+  friend class ReferenceStackScope;
 
   CodeMap* codemap_;
   Isolate* isolate_;
@@ -3928,12 +3957,14 @@ class InterpretedFrameImpl {
   }
 
   WasmValue GetLocalValue(int index) const {
+    ThreadImpl::ReferenceStackScope stack_scope(thread_);
     DCHECK_LE(0, index);
     DCHECK_GT(GetLocalCount(), index);
     return thread_->GetStackValue(static_cast<int>(frame()->sp) + index);
   }
 
   WasmValue GetStackValue(int index) const {
+    ThreadImpl::ReferenceStackScope stack_scope(thread_);
     DCHECK_LE(0, index);
     // Index must be within the number of stack values of this frame.
     DCHECK_GT(GetStackHeight(), index);
@@ -3981,21 +4012,33 @@ const InterpretedFrameImpl* ToImpl(const InterpretedFrame* frame) {
 // translation unit anyway.
 //============================================================================
 WasmInterpreter::State WasmInterpreter::Thread::state() {
-  return ToImpl(this)->state();
+  ThreadImpl* impl = ToImpl(this);
+  ThreadImpl::ReferenceStackScope stack_scope(impl);
+  return impl->state();
 }
 void WasmInterpreter::Thread::InitFrame(const WasmFunction* function,
                                         WasmValue* args) {
-  ToImpl(this)->InitFrame(function, args);
+  ThreadImpl* impl = ToImpl(this);
+  ThreadImpl::ReferenceStackScope stack_scope(impl);
+  impl->InitFrame(function, args);
 }
 WasmInterpreter::State WasmInterpreter::Thread::Run(int num_steps) {
-  return ToImpl(this)->Run(num_steps);
+  ThreadImpl* impl = ToImpl(this);
+  ThreadImpl::ReferenceStackScope stack_scope(impl);
+  return impl->Run(num_steps);
 }
 void WasmInterpreter::Thread::Pause() { return ToImpl(this)->Pause(); }
-void WasmInterpreter::Thread::Reset() { return ToImpl(this)->Reset(); }
+void WasmInterpreter::Thread::Reset() {
+  ThreadImpl* impl = ToImpl(this);
+  ThreadImpl::ReferenceStackScope stack_scope(impl);
+  return impl->Reset();
+}
 WasmInterpreter::Thread::ExceptionHandlingResult
 WasmInterpreter::Thread::RaiseException(Isolate* isolate,
                                         Handle<Object> exception) {
-  return ToImpl(this)->RaiseException(isolate, exception);
+  ThreadImpl* impl = ToImpl(this);
+  ThreadImpl::ReferenceStackScope stack_scope(impl);
+  return impl->RaiseException(isolate, exception);
 }
 pc_t WasmInterpreter::Thread::GetBreakpointPc() {
   return ToImpl(this)->GetBreakpointPc();
@@ -4009,7 +4052,9 @@ WasmInterpreter::FramePtr WasmInterpreter::Thread::GetFrame(int index) {
   return FramePtr(ToFrame(new InterpretedFrameImpl(ToImpl(this), index)));
 }
 WasmValue WasmInterpreter::Thread::GetReturnValue(int index) {
-  return ToImpl(this)->GetReturnValue(index);
+  ThreadImpl* impl = ToImpl(this);
+  ThreadImpl::ReferenceStackScope stack_scope(impl);
+  return impl->GetReturnValue(index);
 }
 TrapReason WasmInterpreter::Thread::GetTrapReason() {
   return ToImpl(this)->GetTrapReason();
@@ -4036,13 +4081,19 @@ uint32_t WasmInterpreter::Thread::NumActivations() {
   return ToImpl(this)->NumActivations();
 }
 uint32_t WasmInterpreter::Thread::StartActivation() {
-  return ToImpl(this)->StartActivation();
+  ThreadImpl* impl = ToImpl(this);
+  ThreadImpl::ReferenceStackScope stack_scope(impl);
+  return impl->StartActivation();
 }
 void WasmInterpreter::Thread::FinishActivation(uint32_t id) {
-  ToImpl(this)->FinishActivation(id);
+  ThreadImpl* impl = ToImpl(this);
+  ThreadImpl::ReferenceStackScope stack_scope(impl);
+  impl->FinishActivation(id);
 }
 uint32_t WasmInterpreter::Thread::ActivationFrameBase(uint32_t id) {
-  return ToImpl(this)->ActivationFrameBase(id);
+  ThreadImpl* impl = ToImpl(this);
+  ThreadImpl::ReferenceStackScope stack_scope(impl);
+  return impl->ActivationFrameBase(id);
 }
 
 //============================================================================
@@ -4061,15 +4112,7 @@ class WasmInterpreterInternals {
                            Handle<WasmInstanceObject> instance_object)
       : module_bytes_(wire_bytes.start(), wire_bytes.end(), zone),
         codemap_(module, module_bytes_.data(), zone) {
-    Isolate* isolate = instance_object->GetIsolate();
-    Handle<Cell> reference_stack = isolate->global_handles()->Create(
-        *isolate->factory()->NewCell(isolate->factory()->empty_fixed_array()));
-    threads_.emplace_back(zone, &codemap_, instance_object, reference_stack);
-  }
-
-  ~WasmInterpreterInternals() {
-    DCHECK_EQ(1, threads_.size());
-    GlobalHandles::Destroy(threads_[0].reference_stack_cell().location());
+    threads_.emplace_back(zone, &codemap_, instance_object);
   }
 };
 

src/wasm/wasm-objects-inl.h

@@ -382,6 +382,8 @@ ACCESSORS(WasmIndirectFunctionTable, refs, FixedArray, kRefsOffset)
 // WasmDebugInfo
 ACCESSORS(WasmDebugInfo, wasm_instance, WasmInstanceObject, kInstanceOffset)
 ACCESSORS(WasmDebugInfo, interpreter_handle, Object, kInterpreterHandleOffset)
+ACCESSORS(WasmDebugInfo, interpreter_reference_stack, Cell,
+          kInterpreterReferenceStackOffset)
 OPTIONAL_ACCESSORS(WasmDebugInfo, locals_names, FixedArray, kLocalsNamesOffset)
 OPTIONAL_ACCESSORS(WasmDebugInfo, c_wasm_entries, FixedArray,
                    kCWasmEntriesOffset)

src/wasm/wasm-objects.h

@@ -819,6 +819,7 @@ class WasmDebugInfo : public Struct {
   NEVER_READ_ONLY_SPACE
   DECL_ACCESSORS(wasm_instance, WasmInstanceObject)
   DECL_ACCESSORS(interpreter_handle, Object)  // Foreign or undefined
+  DECL_ACCESSORS(interpreter_reference_stack, Cell)
   DECL_OPTIONAL_ACCESSORS(locals_names, FixedArray)
   DECL_OPTIONAL_ACCESSORS(c_wasm_entries, FixedArray)
   DECL_OPTIONAL_ACCESSORS(c_wasm_entry_map, Managed<wasm::SignatureMap>)