[wasm] [interpreter] Fix GC issue

Make sure that we call the destructors on all embedded object by
replacing the WasmInterpreterInternals::Delete method by an actual
destructor. This way, the compiler automatically calls destructors on
all embedded objects, in particular the IdentityMap in the CodeMap.

This change also requires to release managed objects *before*
tearing down the heap, because the wasm interpreter, referenced via
Managed<>, contains global handles. When those are destroyed, the
isolate still needs to be intact.

Drive-by: Fix include guard in managed.h.

R=ahaas@chromium.org, ulan@chromium.org, mvstanton@chromium.org
BUG=v8:5822

Change-Id: I9a067f037e013c84e4d697a1e913b27c683bb529
Reviewed-on: https://chromium-review.googlesource.com/466187Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#44368}

src/isolate.cc

@@ -2001,12 +2001,15 @@ Isolate::ThreadDataTable::~ThreadDataTable() {
 void Isolate::ReleaseManagedObjects() {
   Isolate::ManagedObjectFinalizer* current =
       managed_object_finalizers_list_.next_;
+  managed_object_finalizers_list_.next_ = nullptr;
   while (current != nullptr) {
     Isolate::ManagedObjectFinalizer* next = current->next_;
     current->Dispose();
     delete current;
     current = next;
   }
+  // No new managed objects should pop up during finalization.
+  DCHECK_NULL(managed_object_finalizers_list_.next_);
 }
 
 Isolate::ManagedObjectFinalizer* Isolate::RegisterForReleaseAtTeardown(
@@ -2345,6 +2348,9 @@ void Isolate::Deinit() {
   debug()->Unload();
 
   FreeThreadResources();
+  // Release managed objects before shutting down the heap. The finalizer might
+  // need to access heap objects.
+  ReleaseManagedObjects();
 
   if (concurrent_recompilation_enabled()) {
     optimizing_compile_dispatcher_->Stop();
@@ -2408,7 +2414,6 @@ void Isolate::Deinit() {
   root_index_map_ = NULL;
 
   ClearSerializerData();
-  ReleaseManagedObjects();
 }
 
 

src/managed.h

@@ -2,8 +2,8 @@
 // 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_
+#ifndef V8_MANAGED_H_
+#define V8_MANAGED_H_
 
 #include "src/factory.h"
 #include "src/global-handles.h"
@@ -78,4 +78,4 @@ class Managed : public Foreign {
 }  // namespace internal
 }  // namespace v8
 
-#endif  // V8_WASM_MANAGED_H_
+#endif  // V8_MANAGED_H_

src/wasm/wasm-interpreter.cc

@@ -926,10 +926,12 @@ class CodeMap {
   }
 
   ~CodeMap() {
-    // Destroy the global handles (passing nullptr is ok).
-    GlobalHandles::Destroy(Handle<Object>::cast(instance_).location());
+    // Destroy the global handles.
+    // Cast the location, not the handle, because the handle cast might access
+    // the object behind the handle.
+    GlobalHandles::Destroy(reinterpret_cast<Object**>(instance_.location()));
     GlobalHandles::Destroy(
-        Handle<Object>::cast(imported_functions_).location());
+        reinterpret_cast<Object**>(imported_functions_.location()));
   }
 
   const WasmModule* module() const { return module_; }
@@ -2353,8 +2355,6 @@ class WasmInterpreterInternals : public ZoneObject {
         threads_(zone) {
     threads_.emplace_back(zone, &codemap_, env.module_env.instance);
   }
-
-  void Delete() { threads_.clear(); }
 };
 
 //============================================================================
@@ -2364,7 +2364,7 @@ WasmInterpreter::WasmInterpreter(Isolate* isolate, const ModuleBytesEnv& env)
     : zone_(isolate->allocator(), ZONE_NAME),
       internals_(new (&zone_) WasmInterpreterInternals(isolate, &zone_, env)) {}
 
-WasmInterpreter::~WasmInterpreter() { internals_->Delete(); }
+WasmInterpreter::~WasmInterpreter() { internals_->~WasmInterpreterInternals(); }
 
 void WasmInterpreter::Run() { internals_->threads_[0].Run(); }
 

test/mjsunit/wasm/interpreter.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --wasm-interpret-all --allow-natives-syntax
+// Flags: --wasm-interpret-all --allow-natives-syntax --expose-gc
 
 load('test/mjsunit/wasm/wasm-constants.js');
 load('test/mjsunit/wasm/wasm-module-builder.js');
@@ -365,3 +365,36 @@ function checkStack(stack, expected_lines) {
   assertEquals(2 * (11 + 2), instance.exports.main());
   assertEquals(interpreted_before + 2, % WasmNumInterpretedCalls(instance));
 })();
+
+(function testInterpreterGC() {
+  function run(f) {
+    // wrap the creation in a closure so that the only thing returned is
+    // the module (i.e. the underlying array buffer of WASM wire bytes dies).
+    var module = (() => {
+      var builder = new WasmModuleBuilder();
+      var imp = builder.addImport('mod', 'the_name_of_my_import', kSig_i_i);
+      builder.addFunction('main', kSig_i_i)
+          .addBody([kExprGetLocal, 0, kExprCallFunction, imp])
+          .exportAs('main');
+      print('module');
+      return new WebAssembly.Module(builder.toBuffer());
+    })();
+
+    gc();
+    for (var i = 0; i < 10; i++) {
+      print('  instance ' + i);
+      var instance =
+          new WebAssembly.Instance(module, {'mod': {the_name_of_my_import: f}});
+      var g = instance.exports.main;
+      assertEquals('function', typeof g);
+      for (var j = 0; j < 10; j++) {
+        assertEquals(f(j), g(j));
+      }
+    }
+  }
+
+  for (var i = 0; i < 3; i++) {
+    run(x => (x + 19));
+    run(x => (x - 18));
+  }
+})();