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

src/isolate.h

 // Copyright 2012 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_ISOLATE_H_
 #define V8_ISOLATE_H_
 
 #include <memory>
 #include <queue>
 
@@ skipping 513 matching lines @@
   bool Init(Deserializer* des);
 
   // True if at least one thread Enter'ed this isolate.
   bool IsInUse() { return entry_stack_ != NULL; }
 
   // Destroys the non-default isolates.
   // Sets default isolate into "has_been_disposed" state rather then destroying,
   // for legacy API reasons.
   void TearDown();
 
+  void ReleaseManagedLifelines();
+
   static void GlobalTearDown();
 
   void ClearSerializerData();
 
   // Find the PerThread for this particular (isolate, thread) combination
   // If one does not yet exist, return null.
   PerIsolateThreadData* FindPerThreadDataForThisThread();
 
   // Find the PerThread for given (isolate, thread) combination
   // If one does not yet exist, return null.
@@ skipping 655 matching lines @@
 
   PRINTF_FORMAT(2, 3) void PrintWithTimestamp(const char* format, ...);
 
 #ifdef USE_SIMULATOR
   base::Mutex* simulator_i_cache_mutex() { return &simulator_i_cache_mutex_; }
 #endif
 
   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 ManagedLifeline final {

[titzer, 2017/02/13 21:58:25]

Can we just make this a struct and hide it in the .cc file? Then the members
don't even need to be private, since it can't be seen outside the .cc file.

[Mircea Trofin, 2017/02/14 04:50:55]

That would mean:
- changing managed_lifelines_root_ to a ManagedLifeline*
- figuring out its allocation/release. We want it to be an empty head, 
because it makes the insertion/removal logic simpler. But now we need 
to track its lifetime. Not that it's difficult, but adds some 
complexity
- adding a forward declaration. The C++ style guide discourages that: 
https://google.github.io/styleguide/cppguide.html#Forward_Declarations
- adding an Isolate member to the tune of "void 
DisposeLifeline(ManagedLifeline* value)" or a bool option to 
UnregisterFromReleaseAtTeardown to say "also delete" or just make that 
latter method also delete.


The current CL fits into the style of isolate.h - there are 
similar other inner classes. The proposed new pattern would:
- change that for this one case;
- add some complexity, albeit small (lines 1&2 above)
- sidestep a style guide

Unless there's a strong motivation for doing this, I'd rather not.
Also, if there is a strong motivation for going in the private, should
it be also applied to the other inner types in this file, or 
elsewhere?

+   public:
+    typedef void (*Deleter)(void*);
+    void Dispose() { deleter_(value_); }
+
+   private:
+    friend class Isolate;
+
+    ManagedLifeline() {
+      DCHECK_EQ(reinterpret_cast<void*>(this),
+                reinterpret_cast<void*>(&value_));
+    }
+
+    // value_ must be the first member
+    void* value_ = nullptr;
+    Deleter deleter_ = nullptr;
+    ManagedLifeline* prev_ = nullptr;
+    ManagedLifeline* next_ = nullptr;
+  };
+
+  // Register a native value for destruction at isolate teardown.
+  ManagedLifeline* RegisterForReleaseAtTeardown(
+      void* value, ManagedLifeline::Deleter deleter);
+
+  // Unregister a previously registered value from release at
+  // isolate teardown, deleting the ManagedLifeline.
+  // This transfers the responsibility of the previously managed value's
+  // deletion to the caller.
+  void UnregisterFromReleaseAtTeardown(ManagedLifeline*& lifeline);
+
  protected:
   explicit Isolate(bool enable_serializer);
   bool IsArrayOrObjectPrototype(Object* object);
 
  private:
   friend struct GlobalState;
   friend struct InitializeGlobalState;
 
   // These fields are accessed through the API, offsets must be kept in sync
   // with v8::internal::Internals (in include/v8.h) constants. This is also
@@ skipping 261 matching lines @@
 
   v8::Isolate::AbortOnUncaughtExceptionCallback
       abort_on_uncaught_exception_callback_;
 
 #ifdef USE_SIMULATOR
   base::Mutex simulator_i_cache_mutex_;
 #endif
 
   bool allow_atomics_wait_;
 
+  ManagedLifeline managed_lifelines_root_;
+
   size_t total_regexp_code_generated_;
 
   friend class ExecutionAccess;
   friend class HandleScopeImplementer;
   friend class HeapTester;
   friend class OptimizingCompileDispatcher;
   friend class SweeperThread;
   friend class ThreadManager;
   friend class Simulator;
   friend class StackGuard;
@@ skipping 218 matching lines @@
 
   EmbeddedVector<char, 128> filename_;
   FILE* file_;
   int scope_depth_;
 };
 
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_ISOLATE_H_