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

src/isolate.cc

Index: src/isolate.cc
diff --git a/src/isolate.cc b/src/isolate.cc
index aa798086705e0831e40b845c3a64cc323078f871..785b60bd0bf2eb38d842595c4ca3f62140503562 100644
--- a/src/isolate.cc
+++ b/src/isolate.cc
@@ -2022,6 +2022,55 @@ Isolate::ThreadDataTable::~ThreadDataTable() {
   // DCHECK_NULL(list_);
 }
 
+void Isolate::ManagedLifeline::CallDeleter() {
+  DCHECK_NOT_NULL(value_);
+  // deleter_ may be null in test scenarios.
+  if (deleter_ != nullptr) {
+    deleter_(value_);
+  }
+}
+
+Isolate::ManagedLifeline::~ManagedLifeline() {
+  // when we delete the Isolate, the destructor will be called for the
+  // managed_list_root_;
+  if (prev_ == nullptr) return;
+  DCHECK_NOT_NULL(value_);
+
+  prev_->next_ = next_;
+  if (next_ != nullptr) next_->prev_ = prev_;
+  // Used in test - where the memory is allocated by the test.
+  CallDeleter();
+}
+
+void Isolate::ManagedLifeline::Clear() {

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

Something seems weird with this. Why are we using a both a destructor and an
explicit clear? Why not just remove the destructor and put it all into clear?

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

This clears the whole list. The destructor deletes a node and may be invoked
either by finalizer or when the list is cleared at Teardown

[titzer, 2017/02/09 04:25:18]

Sure, but I don't think it's necessary to have a destructor do either of those
jobs. You can just have a struct that forms the linked list. In the call site in
the code below, you can just iterate over the whole list and delete() both the
managed object and the link in the list.

+  // Should be called from the head of the dlinked list. We don't have a reason
+  // to do otherwise.
+  DCHECK_NULL(prev_);
+
+  for (Isolate::ManagedLifeline* current = next_; current != nullptr;) {
+    Isolate::ManagedLifeline* next = current->next_;
+    // This also relinks the list, which is redundant in this case,
+    // but that's at most 2 assignments  per node.
+    delete current;
+    current = next;
+  }
+}
+
+Isolate::ManagedLifeline* Isolate::AddManagedObject(
+    void* value, Isolate::ManagedLifeline::Deleter deleter) {
+  Isolate::ManagedLifeline* ret = new Isolate::ManagedLifeline();
+  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::ManagedLifeline* next = managed_lifelines_root_.next_;
+  managed_lifelines_root_.next_ = ret;
+  ret->prev_ = &managed_lifelines_root_;
+  ret->next_ = next;
+  if (next != nullptr) next->prev_ = ret;
+  return ret;
+}
 
 Isolate::PerIsolateThreadData::~PerIsolateThreadData() {
 #if defined(USE_SIMULATOR)
@@ -2391,6 +2440,7 @@ void Isolate::Deinit() {
   root_index_map_ = NULL;
 
   ClearSerializerData();
+  managed_lifelines_root_.Clear();
 }