From 9cf9e82a2af10827c3ff3e0faf0a2be101b52b93 Mon Sep 17 00:00:00 2001 From: Andreas Haas Date: Tue, 24 Sep 2019 18:36:06 +0200 Subject: [PATCH] [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 Reviewed-by: Michael Starzinger Cr-Commit-Position: refs/heads/master@{#63953} --- src/builtins/base.tq | 1 + src/wasm/wasm-debug.cc | 2 + src/wasm/wasm-interpreter.cc | 85 +++++++++++++++++++++++++++--------- src/wasm/wasm-objects-inl.h | 2 + src/wasm/wasm-objects.h | 1 + 5 files changed, 70 insertions(+), 21 deletions(-) diff --git a/src/builtins/base.tq b/src/builtins/base.tq index cf6a083c7c..39470c135a 100644 --- a/src/builtins/base.tq +++ b/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 diff --git a/src/wasm/wasm-debug.cc b/src/wasm/wasm-debug.cc index bd6b1a0426..7c354d1163 100644 --- a/src/wasm/wasm-debug.cc +++ b/src/wasm/wasm-debug.cc @@ -503,9 +503,11 @@ wasm::InterpreterHandle* GetInterpreterHandleOrNull(WasmDebugInfo debug_info) { Handle WasmDebugInfo::New(Handle instance) { DCHECK(!instance->has_debug_info()); Factory* factory = instance->GetIsolate()->factory(); + Handle stack_cell = factory->NewCell(factory->empty_fixed_array()); Handle debug_info = Handle::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; } diff --git a/src/wasm/wasm-interpreter.cc b/src/wasm/wasm-interpreter.cc index 49bda1dbaa..3abacdcf6f 100644 --- a/src/wasm/wasm-interpreter.cc +++ b/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(); + } + } + + private: + ThreadImpl* impl_; + bool do_reset_stack_ = false; + }; + ThreadImpl(Zone* zone, CodeMap* codemap, - Handle instance_object, - Handle reference_stack_cell) + Handle 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(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 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 instance_object) : module_bytes_(wire_bytes.start(), wire_bytes.end(), zone), codemap_(module, module_bytes_.data(), zone) { - Isolate* isolate = instance_object->GetIsolate(); - Handle 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); } }; diff --git a/src/wasm/wasm-objects-inl.h b/src/wasm/wasm-objects-inl.h index 66d3a2716e..4457813851 100644 --- a/src/wasm/wasm-objects-inl.h +++ b/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) diff --git a/src/wasm/wasm-objects.h b/src/wasm/wasm-objects.h index a79fcf856c..086aa61a36 100644 --- a/src/wasm/wasm-objects.h +++ b/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)