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

src/wasm/managed.h

 // Copyright 2016 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef V8_WASM_MANAGED_H_
 #define V8_WASM_MANAGED_H_
 
 #include "src/factory.h"
 #include "src/global-handles.h"
 #include "src/handles.h"
 #include "src/isolate.h"
 #include "src/objects-inl.h"
 
 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.
 template <class CppType>
 class Managed : public Foreign {
  public:
   V8_INLINE CppType* get() {
-    return reinterpret_cast<CppType*>(foreign_address());
+    return reinterpret_cast<CppType*>(
+        reinterpret_cast<Isolate::ManagedLifeline*>(foreign_address())
+            ->value());
   }
 
   static Managed<CppType>* cast(Object* obj) {
     SLOW_DCHECK(obj->IsForeign());
     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::ManagedLifeline* node =
+        isolate->AddManagedObject(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;
   }
 
+  static Handle<Managed<CppType>> NewForTesting(Isolate* isolate,

[titzer, 2017/02/08 23:32:48]

Do we really need to have a separate constructor for testing? What is the harm
of registering the weak callback for delete for testing too?

[Mircea Trofin, 2017/02/08 23:52:36]

Not super convinced of the cleanliness of the case that uses the Testing API
(the non-deleting case), to be honest. We should take a look seaparately. In the
meantime, I wanted to separate that one case in cctest out.

My specific concern is: what is the role of Managed<T>, if auto-cleanup isn't
it?

+                                                CppType* ptr) {
+    Isolate::ManagedLifeline* node = isolate->AddManagedObject(ptr, nullptr);
+    Handle<Managed<CppType>> handle = Handle<Managed<CppType>>::cast(
+        isolate->factory()->NewForeign(reinterpret_cast<Address>(node)));
+    return handle;
+  }
+
  private:
   static void RegisterWeakCallbackForDelete(Isolate* isolate,
                                             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();
+    delete (
+        reinterpret_cast<Isolate::ManagedLifeline*>((*p)->foreign_address()));
     (*p)->set_foreign_address(0);
     GlobalHandles::Destroy(reinterpret_cast<Object**>(p));
   }
+
+  static void NativeDelete(void* value) {
+    CppType* typed_value = reinterpret_cast<CppType*>(value);
+    delete typed_value;
+  }
 };
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_WASM_MANAGED_H_