[wasm] Managed<T> ensures T's lifetime does not leak past Isolate's

Native resources allocated by v8, as internal implementation detail,
and held by a Foreign object, must be released when the Isolate is
torn down. Example: wasm::WasmModule allocated by wasm compile, and
held throughout the lifetime of the WebAssembly.Module object.

This change:
- Extends Managed<CppType> with a mechanism for doing just that
- Separates the role of Managed<CppType> to be strictly an owner of
the lifetime of the native resource. For cases where that's not
desirable, we can polymorphically use Foregin.
- moves managed.h out of wasm, since it's not wasm-specific.

BUG=680065

Review-Url: https://codereview.chromium.org/2676513008
Cr-Commit-Position: refs/heads/master@{#43350}

BUILD.gn

@@ -1571,6 +1571,7 @@ v8_source_set("v8_base") {
     "src/machine-type.cc",
     "src/machine-type.h",
     "src/macro-assembler.h",
+    "src/managed.h",
     "src/map-updater.cc",
     "src/map-updater.h",
     "src/messages.cc",
@@ -1802,7 +1803,6 @@ v8_source_set("v8_base") {
     "src/wasm/function-body-decoder.cc",
     "src/wasm/function-body-decoder.h",
     "src/wasm/leb-helper.h",
-    "src/wasm/managed.h",
     "src/wasm/module-decoder.cc",
     "src/wasm/module-decoder.h",
     "src/wasm/signature-map.cc",

src/isolate.cc

@@ -2030,6 +2030,47 @@ Isolate::ThreadDataTable::~ThreadDataTable() {
   // DCHECK_NULL(list_);
 }
 
+void Isolate::ReleaseManagedObjects() {
+  Isolate::ManagedObjectFinalizer* current =
+      managed_object_finalizers_list_.next_;
+  while (current != nullptr) {
+    Isolate::ManagedObjectFinalizer* next = current->next_;
+    current->Dispose();
+    delete current;
+    current = next;
+  }
+}
+
+Isolate::ManagedObjectFinalizer* Isolate::RegisterForReleaseAtTeardown(
+    void* value, Isolate::ManagedObjectFinalizer::Deleter deleter) {
+  DCHECK_NOT_NULL(value);
+  DCHECK_NOT_NULL(deleter);
+
+  Isolate::ManagedObjectFinalizer* ret = new Isolate::ManagedObjectFinalizer();
+  ret->value_ = value;
+  ret->deleter_ = deleter;
+  // Insert at head. We keep the head alive for the lifetime of the Isolate
+  // because otherwise we can't reset the head, should we delete it before
+  // the isolate expires
+  Isolate::ManagedObjectFinalizer* next = managed_object_finalizers_list_.next_;
+  managed_object_finalizers_list_.next_ = ret;
+  ret->prev_ = &managed_object_finalizers_list_;
+  ret->next_ = next;
+  if (next != nullptr) next->prev_ = ret;
+  return ret;
+}
+
+void Isolate::UnregisterFromReleaseAtTeardown(
+    Isolate::ManagedObjectFinalizer** finalizer_ptr) {
+  DCHECK_NOT_NULL(finalizer_ptr);
+  Isolate::ManagedObjectFinalizer* finalizer = *finalizer_ptr;
+  DCHECK_NOT_NULL(finalizer->prev_);
+
+  finalizer->prev_->next_ = finalizer->next_;
+  if (finalizer->next_ != nullptr) finalizer->next_->prev_ = finalizer->prev_;
+  delete finalizer;
+  *finalizer_ptr = nullptr;
+}
 
 Isolate::PerIsolateThreadData::~PerIsolateThreadData() {
 #if defined(USE_SIMULATOR)
@@ -2399,6 +2440,7 @@ void Isolate::Deinit() {
   root_index_map_ = NULL;
 
   ClearSerializerData();
+  ReleaseManagedObjects();
 }
 
 

src/isolate.h

@@ -533,6 +533,8 @@ class Isolate {
   // for legacy API reasons.
   void TearDown();
 
+  void ReleaseManagedObjects();
+
   static void GlobalTearDown();
 
   void ClearSerializerData();
@@ -1208,6 +1210,39 @@ class Isolate {
   void set_allow_atomics_wait(bool set) { allow_atomics_wait_ = set; }
   bool allow_atomics_wait() { return allow_atomics_wait_; }
 
+  // List of native heap values allocated by the runtime as part of its
+  // implementation that must be freed at isolate deinit.
+  class ManagedObjectFinalizer final {
+   public:
+    typedef void (*Deleter)(void*);
+    void Dispose() { deleter_(value_); }
+
+   private:
+    friend class Isolate;
+
+    ManagedObjectFinalizer() {
+      DCHECK_EQ(reinterpret_cast<void*>(this),
+                reinterpret_cast<void*>(&value_));
+    }
+
+    // value_ must be the first member
+    void* value_ = nullptr;
+    Deleter deleter_ = nullptr;
+    ManagedObjectFinalizer* prev_ = nullptr;
+    ManagedObjectFinalizer* next_ = nullptr;
+  };
+
+  // Register a native value for destruction at isolate teardown.
+  ManagedObjectFinalizer* RegisterForReleaseAtTeardown(
+      void* value, ManagedObjectFinalizer::Deleter deleter);
+
+  // Unregister a previously registered value from release at
+  // isolate teardown, deleting the ManagedObjectFinalizer.
+  // This transfers the responsibility of the previously managed value's
+  // deletion to the caller. Pass by pointer, because *finalizer_ptr gets
+  // reset to nullptr.
+  void UnregisterFromReleaseAtTeardown(ManagedObjectFinalizer** finalizer_ptr);
+
  protected:
   explicit Isolate(bool enable_serializer);
   bool IsArrayOrObjectPrototype(Object* object);
@@ -1489,6 +1524,8 @@ class Isolate {
 
   bool allow_atomics_wait_;
 
+  ManagedObjectFinalizer managed_object_finalizers_list_;
+
   size_t total_regexp_code_generated_;
 
   friend class ExecutionAccess;

src/wasm/managed.h → src/managed.h

@@ -12,13 +12,19 @@
 
 namespace v8 {
 namespace internal {
-// An object that wraps a pointer to a C++ object and optionally deletes it
-// when the managed wrapper object is garbage collected.
+// An object that wraps a pointer to a C++ object and manages its lifetime.
+// The C++ object will be deleted when the managed wrapper object is
+// garbage collected, or, last resort, if the isolate is torn down before GC,
+// as part of Isolate::Dispose().
+// Managed<CppType> may be used polymorphically as Foreign, where the held
+// address is typed as CppType**. The double indirection is due to the
+// use, by Managed, of Isolate::ManagedObjectFinalizer, which has a CppType*
+// first field.
 template <class CppType>
 class Managed : public Foreign {
  public:
   V8_INLINE CppType* get() {
-    return reinterpret_cast<CppType*>(foreign_address());
+    return *(reinterpret_cast<CppType**>(foreign_address()));
   }
 
   static Managed<CppType>* cast(Object* obj) {
@@ -26,13 +32,13 @@ class Managed : public Foreign {
     return reinterpret_cast<Managed<CppType>*>(obj);
   }
 
-  static Handle<Managed<CppType>> New(Isolate* isolate, CppType* ptr,
-                                      bool delete_on_gc = true) {
+  static Handle<Managed<CppType>> New(Isolate* isolate, CppType* ptr) {
+    Isolate::ManagedObjectFinalizer* node =
+        isolate->RegisterForReleaseAtTeardown(ptr,
+                                              Managed<CppType>::NativeDelete);
     Handle<Managed<CppType>> handle = Handle<Managed<CppType>>::cast(
-        isolate->factory()->NewForeign(reinterpret_cast<Address>(ptr)));
-    if (delete_on_gc) {
-      RegisterWeakCallbackForDelete(isolate, handle);
-    }
+        isolate->factory()->NewForeign(reinterpret_cast<Address>(node)));
+    RegisterWeakCallbackForDelete(isolate, handle);
     return handle;
   }
 
@@ -41,16 +47,33 @@ class Managed : public Foreign {
                                             Handle<Managed<CppType>> handle) {
     Handle<Object> global_handle = isolate->global_handles()->Create(*handle);
     GlobalHandles::MakeWeak(global_handle.location(), global_handle.location(),
-                            &Managed<CppType>::Delete,
+                            &Managed<CppType>::GCDelete,
                             v8::WeakCallbackType::kFinalizer);
   }
-  static void Delete(const v8::WeakCallbackInfo<void>& data) {
+
+  static void GCDelete(const v8::WeakCallbackInfo<void>& data) {
     Managed<CppType>** p =
         reinterpret_cast<Managed<CppType>**>(data.GetParameter());
-    delete (*p)->get();
-    (*p)->set_foreign_address(0);
+
+    Isolate::ManagedObjectFinalizer* finalizer = (*p)->GetFinalizer();
+
+    Isolate* isolate = reinterpret_cast<Isolate*>(data.GetIsolate());
+    finalizer->Dispose();
+    isolate->UnregisterFromReleaseAtTeardown(&finalizer);
+
+    (*p)->set_foreign_address(static_cast<Address>(nullptr));
     GlobalHandles::Destroy(reinterpret_cast<Object**>(p));
   }
+
+  static void NativeDelete(void* value) {
+    CppType* typed_value = reinterpret_cast<CppType*>(value);
+    delete typed_value;
+  }
+
+  Isolate::ManagedObjectFinalizer* GetFinalizer() {
+    return reinterpret_cast<Isolate::ManagedObjectFinalizer*>(
+        foreign_address());
+  }
 };
 }  // namespace internal
 }  // namespace v8

src/v8.gyp

@@ -1079,6 +1079,7 @@
         'macro-assembler.h',
         'machine-type.cc',
         'machine-type.h',
+        'managed.h',
         'messages.cc',
         'messages.h',
         'msan.h',
@@ -1310,7 +1311,6 @@
         'wasm/function-body-decoder.h',
         'wasm/function-body-decoder-impl.h',
         'wasm/leb-helper.h',
-        'wasm/managed.h',
         'wasm/module-decoder.cc',
         'wasm/module-decoder.h',
         'wasm/signature-map.cc',

src/wasm/wasm-module.h

@@ -11,9 +11,9 @@
 #include "src/debug/debug-interface.h"
 #include "src/globals.h"
 #include "src/handles.h"
+#include "src/managed.h"
 #include "src/parsing/preparse-data.h"
 
-#include "src/wasm/managed.h"
 #include "src/wasm/signature-map.h"
 #include "src/wasm/wasm-opcodes.h"
 

src/wasm/wasm-objects.cc

@@ -531,7 +531,13 @@ WasmSharedModuleData* WasmSharedModuleData::cast(Object* object) {
 }
 
 wasm::WasmModule* WasmSharedModuleData::module() {
-  return reinterpret_cast<WasmModuleWrapper*>(get(kModuleWrapper))->get();
+  // We populate the kModuleWrapper field with a Foreign holding the
+  // address to the address of a WasmModule. This is because we can
+  // handle both cases when the WasmModule's lifetime is managed through
+  // a Managed<WasmModule> object, as well as cases when it's managed
+  // by the embedder. CcTests fall into the latter case.
+  return *(reinterpret_cast<wasm::WasmModule**>(
+      Foreign::cast(get(kModuleWrapper))->foreign_address()));
 }
 
 DEFINE_OPTIONAL_ARR_ACCESSORS(WasmSharedModuleData, module_bytes, kModuleBytes,

src/wasm/wasm-objects.h

@@ -9,7 +9,6 @@
 #include "src/debug/interface-types.h"
 #include "src/objects.h"
 #include "src/trap-handler/trap-handler.h"
-#include "src/wasm/managed.h"
 #include "src/wasm/wasm-limits.h"
 
 namespace v8 {

test/cctest/BUILD.gn

@@ -151,6 +151,7 @@ v8_executable("cctest") {
     "test-liveedit.cc",
     "test-lockers.cc",
     "test-log.cc",
+    "test-managed.cc",
     "test-mementos.cc",
     "test-modules.cc",
     "test-object.cc",
@@ -186,7 +187,6 @@ v8_executable("cctest") {
     "trace-extension.h",
     "types-fuzz.h",
     "unicode-helpers.h",
-    "wasm/test-managed.cc",
     "wasm/test-run-wasm-64.cc",
     "wasm/test-run-wasm-asmjs.cc",
     "wasm/test-run-wasm-interpreter.cc",

test/cctest/cctest.gyp

@@ -171,6 +171,7 @@
       'test-liveedit.cc',
       'test-lockers.cc',
       'test-log.cc',
+      'test-managed.cc',
       'test-mementos.cc',
       'test-modules.cc',
       'test-object.cc',
@@ -207,7 +208,6 @@
       'trace-extension.h',
       'types-fuzz.h',
       'unicode-helpers.h',
-      'wasm/test-managed.cc',
       'wasm/test-run-wasm.cc',
       'wasm/test-run-wasm-64.cc',
       'wasm/test-run-wasm-asmjs.cc',

test/cctest/wasm/test-managed.cc → test/cctest/test-managed.cc

@@ -6,11 +6,10 @@
 #include <stdlib.h>
 #include <string.h>
 
-#include "src/wasm/managed.h"
+#include "src/managed.h"
 
 #include "src/objects-inl.h"
 #include "test/cctest/cctest.h"
-#include "test/common/wasm/test-signatures.h"
 
 using namespace v8::base;
 using namespace v8::internal;
@@ -21,6 +20,9 @@ class DeleteRecorder {
     *deleted_ = false;
   }
   ~DeleteRecorder() { *deleted_ = true; }
+  static void Deleter(void* value) {
+    delete reinterpret_cast<DeleteRecorder*>(value);
+  }
 
  private:
   bool* deleted_;
@@ -28,33 +30,49 @@ class DeleteRecorder {
 
 TEST(ManagedCollect) {
   Isolate* isolate = CcTest::InitIsolateOnce();
-  bool deleted = false;
-  DeleteRecorder* d = new DeleteRecorder(&deleted);
-
+  bool deleted1 = false;
+  bool deleted2 = false;
+  DeleteRecorder* d1 = new DeleteRecorder(&deleted1);
+  DeleteRecorder* d2 = new DeleteRecorder(&deleted2);
+  Isolate::ManagedObjectFinalizer* finalizer =
+      isolate->RegisterForReleaseAtTeardown(d2, DeleteRecorder::Deleter);
   {
     HandleScope scope(isolate);
-    auto handle = Managed<DeleteRecorder>::New(isolate, d);
+    auto handle = Managed<DeleteRecorder>::New(isolate, d1);
     USE(handle);
   }
 
   CcTest::CollectAllAvailableGarbage();
 
-  CHECK(deleted);
+  CHECK(deleted1);
+  CHECK(!deleted2);
+  isolate->UnregisterFromReleaseAtTeardown(&finalizer);
+  CHECK_NULL(finalizer);
+  delete d2;
+  CHECK(deleted2);
 }
 
-TEST(ManagedCollectNoDelete) {
-  Isolate* isolate = CcTest::InitIsolateOnce();
-  bool deleted = false;
-  DeleteRecorder* d = new DeleteRecorder(&deleted);
+TEST(DisposeCollect) {
+  v8::Isolate::CreateParams create_params;
+  create_params.array_buffer_allocator =
+      CcTest::InitIsolateOnce()->array_buffer_allocator();
 
+  v8::Isolate* isolate = v8::Isolate::New(create_params);
+  isolate->Enter();
+  Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  bool deleted1 = false;
+  bool deleted2 = false;
+  DeleteRecorder* d1 = new DeleteRecorder(&deleted1);
+  DeleteRecorder* d2 = new DeleteRecorder(&deleted2);
   {
-    HandleScope scope(isolate);
-    auto handle = Managed<DeleteRecorder>::New(isolate, d, false);
+    HandleScope scope(i_isolate);
+    auto handle = Managed<DeleteRecorder>::New(i_isolate, d1);
     USE(handle);
   }
+  i_isolate->RegisterForReleaseAtTeardown(d2, DeleteRecorder::Deleter);
 
-  CcTest::CollectAllAvailableGarbage();
-
-  CHECK(!deleted);
-  delete d;
+  isolate->Exit();
+  isolate->Dispose();
+  CHECK(deleted1);
+  CHECK(deleted2);
 }

test/cctest/wasm/wasm-run-utils.h

@@ -331,8 +331,10 @@ class TestingModule : public ModuleEnv {
   Handle<WasmInstanceObject> InitInstanceObject() {
     Handle<SeqOneByteString> empty_string = Handle<SeqOneByteString>::cast(
         isolate_->factory()->NewStringFromOneByte({}).ToHandleChecked());
-    Handle<Managed<wasm::WasmModule>> module_wrapper =
-        Managed<wasm::WasmModule>::New(isolate_, &module_, false);
+    // The lifetime of the wasm module is tied to this object's, and we cannot
+    // rely on the mechanics of Managed<T>.
+    Handle<Foreign> module_wrapper =
+        isolate_->factory()->NewForeign(reinterpret_cast<Address>(&module));
     Handle<Script> script =
         isolate_->factory()->NewScript(isolate_->factory()->empty_string());
     script->set_type(Script::TYPE_WASM);