[wasm] Weaken global handles used for indirect tables

The previous design assumed we can't possibly have a cycle involving
an instance, however, we can. For example: a script can reference
an instance, which ends up referencing the native context because
of how we generate wasm-to-js wrappers; that references the global
object, which then references the script. A global handle to the
indirect function table can then root such a cycle. That means
the instance is never collected, which never deletes the global
handle.

This change addresses that by making the handles weak.

Bug: 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ief7263af83974bf96505a4fba65d162474fe7c7c
Reviewed-on: https://chromium-review.googlesource.com/653852
Commit-Queue: Mircea Trofin <mtrofin@chromium.org>
Reviewed-by: Brad Nelson <bradnelson@chromium.org>
Reviewed-by: Aseem Garg <aseemgarg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47909}

src/wasm/module-compiler.cc

@@ -516,23 +516,28 @@ double MonotonicallyIncreasingTimeInMs() {
          base::Time::kMillisecondsPerSecond;
 }
 
+void FunctionTableFinalizer(const v8::WeakCallbackInfo<void>& data) {
+  GlobalHandles::Destroy(reinterpret_cast<Object**>(
+      reinterpret_cast<JSObject**>(data.GetParameter())));
+}
+
 std::unique_ptr<compiler::ModuleEnv> CreateDefaultModuleEnv(
-    Isolate* isolate, WasmModule* module, Handle<Code> illegal_builtin,
-    GlobalHandleLifetimeManager* lifetime_manager) {
+    Isolate* isolate, WasmModule* module, Handle<Code> illegal_builtin) {
   std::vector<GlobalHandleAddress> function_tables;
   std::vector<GlobalHandleAddress> signature_tables;
   std::vector<SignatureMap*> signature_maps;
 
   for (size_t i = 0; i < module->function_tables.size(); i++) {
-    // We need *some* value for each table. We'll reuse this value when
-    // we want to reset a {WasmCompiledModule}. We could just insert
-    // bogus values (e.g. 0, 1, etc), but to keep things consistent, we'll
-    // create a valid global handle for the undefined value.
-    // These global handles are deleted when finalizing the module object.
     Handle<Object> func_table =
         isolate->global_handles()->Create(isolate->heap()->undefined_value());
     Handle<Object> sig_table =
         isolate->global_handles()->Create(isolate->heap()->undefined_value());
+    GlobalHandles::MakeWeak(func_table.location(), func_table.location(),
+                            &FunctionTableFinalizer,
+                            v8::WeakCallbackType::kFinalizer);
+    GlobalHandles::MakeWeak(sig_table.location(), sig_table.location(),
+                            &FunctionTableFinalizer,
+                            v8::WeakCallbackType::kFinalizer);
     function_tables.push_back(func_table.address());
     signature_tables.push_back(sig_table.address());
     signature_maps.push_back(&module->function_tables[i].map);
@@ -593,9 +598,7 @@ MaybeHandle<WasmModuleObject> ModuleCompiler::CompileToModuleObjectInternal(
                                   ? BUILTIN_CODE(isolate_, WasmCompileLazy)
                                   : BUILTIN_CODE(isolate_, Illegal);
 
-  GlobalHandleLifetimeManager globals_manager;
-  auto env = CreateDefaultModuleEnv(isolate_, module_.get(), init_builtin,
-                                    &globals_manager);
+  auto env = CreateDefaultModuleEnv(isolate_, module_.get(), init_builtin);
 
   // The {code_table} array contains import wrappers and functions (which
   // are both included in {functions.size()}, and export wrappers).
@@ -715,11 +718,6 @@ MaybeHandle<WasmModuleObject> ModuleCompiler::CompileToModuleObjectInternal(
     RecordStats(*wrapper_code, counters());
     ++wrapper_index;
   }
-
-  // Now we can relinquish control to the global handles, because the
-  // {WasmModuleObject} will take care of them in its finalizer, which it'll
-  // setup in {New}.
-  globals_manager.ReleaseWithoutDestroying();
   return WasmModuleObject::New(isolate_, compiled_module);
 }
 
@@ -1169,7 +1167,6 @@ MaybeHandle<WasmInstanceObject> InstanceBuilder::Build() {
   DCHECK(!isolate_->has_pending_exception());
   TRACE("Finishing instance %d\n", compiled_module_->instance_id());
   TRACE_CHAIN(module_object_->compiled_module());
-  globals_manager_.ReleaseWithoutDestroying();
   return instance;
 }
 
@@ -1767,6 +1764,15 @@ void InstanceBuilder::InitializeTables(
   Handle<FixedArray> old_signature_tables =
       compiled_module_->signature_tables();
 
+  // These go on the instance.
+  Handle<FixedArray> rooted_function_tables =
+      isolate_->factory()->NewFixedArray(function_table_count, TENURED);
+  Handle<FixedArray> rooted_signature_tables =
+      isolate_->factory()->NewFixedArray(function_table_count, TENURED);
+
+  instance->set_function_tables(*rooted_function_tables);
+  instance->set_signature_tables(*rooted_signature_tables);
+
   DCHECK_EQ(old_function_tables->length(), new_function_tables->length());
   DCHECK_EQ(old_signature_tables->length(), new_signature_tables->length());
 
@@ -1794,20 +1800,32 @@ void InstanceBuilder::InitializeTables(
     }
     int int_index = static_cast<int>(index);
 
-    // We create a global handle here and delete it when finalizing the
-    // instance. Even if the same table is shared accross many instances, each
-    // will have its own private global handle to it. Meanwhile, we the global
-    // handles root the respective objects (the tables).
     Handle<FixedArray> global_func_table =
         isolate_->global_handles()->Create(*table_instance.function_table);
     Handle<FixedArray> global_sig_table =
         isolate_->global_handles()->Create(*table_instance.signature_table);
+    // Make the handles weak. The table objects are rooted on the instance, as
+    // they belong to it. We need the global handles in order to have stable
+    // pointers to embed in the instance's specialization (wasm compiled code).
+    // The order of finalization doesn't matter, in that the instance finalizer
+    // may be called before each table's finalizer, or vice-versa.
+    // This is because values used for embedding are only interesting should we
+    // {Reset} a specialization, in which case they are interesting as values,
+    // they are not dereferenced.
+    GlobalHandles::MakeWeak(
+        reinterpret_cast<Object**>(global_func_table.location()),
+        global_func_table.location(), &FunctionTableFinalizer,
+        v8::WeakCallbackType::kFinalizer);
+    GlobalHandles::MakeWeak(
+        reinterpret_cast<Object**>(global_sig_table.location()),
+        global_sig_table.location(), &FunctionTableFinalizer,
+        v8::WeakCallbackType::kFinalizer);
+
+    rooted_function_tables->set(int_index, *global_func_table);
+    rooted_signature_tables->set(int_index, *global_sig_table);
 
     GlobalHandleAddress new_func_table_addr = global_func_table.address();
     GlobalHandleAddress new_sig_table_addr = global_sig_table.address();
-    globals_manager_.Add(new_func_table_addr);
-    globals_manager_.Add(new_sig_table_addr);
-
     WasmCompiledModule::SetTableValue(isolate_, new_function_tables, int_index,
                                       new_func_table_addr);
     WasmCompiledModule::SetTableValue(isolate_, new_signature_tables, int_index,
@@ -2107,8 +2125,8 @@ class AsyncCompileJob::PrepareAndStartCompile : public CompileStep {
 
     Factory* factory = isolate->factory();
     Handle<Code> illegal_builtin = BUILTIN_CODE(isolate, Illegal);
-    job_->module_env_ = CreateDefaultModuleEnv(
-        isolate, module_.get(), illegal_builtin, &job_->globals_manager_);
+    job_->module_env_ =
+        CreateDefaultModuleEnv(isolate, module_.get(), illegal_builtin);
 
     // The {code_table} array contains import wrappers and functions (which
     // are both included in {functions.size()}.
@@ -2381,7 +2399,6 @@ class AsyncCompileJob::FinishModule : public CompileStep {
     Handle<WasmModuleObject> result =
         WasmModuleObject::New(job_->isolate_, job_->compiled_module_);
     // {job_} is deleted in AsyncCompileSucceeded, therefore the {return}.
-    job_->globals_manager_.ReleaseWithoutDestroying();
     return job_->AsyncCompileSucceeded(result);
   }
 };

src/wasm/module-compiler.h

@@ -235,7 +235,6 @@ class InstanceBuilder {
   std::vector<Handle<JSFunction>> js_wrappers_;
   JSToWasmWrapperCache js_to_wasm_cache_;
   WeakCallbackInfo<void>::Callback instance_finalizer_callback_;
-  GlobalHandleLifetimeManager globals_manager_;
 
   const std::shared_ptr<Counters>& async_counters() const {
     return async_counters_;
@@ -348,7 +347,6 @@ class AsyncCompileJob {
   Handle<JSPromise> module_promise_;
   std::unique_ptr<ModuleCompiler> compiler_;
   std::unique_ptr<compiler::ModuleEnv> module_env_;
-  GlobalHandleLifetimeManager globals_manager_;
 
   std::vector<DeferredHandles*> deferred_handles_;
   Handle<WasmModuleObject> module_object_;

src/wasm/wasm-module.cc

@@ -124,9 +124,6 @@ static void InstanceFinalizer(const v8::WeakCallbackInfo<void>& data) {
     WasmMemoryObject::RemoveInstance(isolate, memory, instance);
   }
 
-  // In all cases, release the global handles held to tables by this instance
-  WasmCompiledModule::DestroyGlobalHandles(isolate, compiled_module);
-
   // weak_wasm_module may have been cleared, meaning the module object
   // was GC-ed. In that case, there won't be any new instances created,
   // and we don't need to maintain the links between instances.

src/wasm/wasm-objects-inl.h

@@ -52,6 +52,10 @@ ACCESSORS(WasmInstanceObject, globals_buffer, JSArrayBuffer,
           kGlobalsBufferOffset)
 OPTIONAL_ACCESSORS(WasmInstanceObject, debug_info, WasmDebugInfo,
                    kDebugInfoOffset)
+OPTIONAL_ACCESSORS(WasmInstanceObject, function_tables, FixedArray,
+                   kFunctionTablesOffset)
+OPTIONAL_ACCESSORS(WasmInstanceObject, signature_tables, FixedArray,
+                   kSignatureTablesOffset)
 ACCESSORS(WasmInstanceObject, directly_called_instances, FixedArray,
           kDirectlyCalledInstancesOffset)
 

src/wasm/wasm-objects.cc

@@ -174,33 +174,9 @@ Handle<WasmModuleObject> WasmModuleObject::New(
   Handle<WeakCell> link_to_module =
       isolate->factory()->NewWeakCell(module_object);
   compiled_module->set_weak_wasm_module(link_to_module);
-  Handle<Object> global_handle =
-      isolate->global_handles()->Create(*module_object);
-  GlobalHandles::MakeWeak(global_handle.location(), global_handle.location(),
-                          &Finalizer, v8::WeakCallbackType::kFinalizer);
   return module_object;
 }
 
-void WasmModuleObject::Finalizer(const v8::WeakCallbackInfo<void>& data) {
-  DisallowHeapAllocation no_gc;
-  JSObject** p = reinterpret_cast<JSObject**>(data.GetParameter());
-  WasmModuleObject* module = reinterpret_cast<WasmModuleObject*>(*p);
-  WasmCompiledModule* compiled_module = module->compiled_module();
-  if (compiled_module->has_empty_function_tables()) {
-    DCHECK(compiled_module->has_empty_signature_tables());
-    for (int i = 0, e = compiled_module->empty_function_tables()->length();
-         i < e; ++i) {
-      GlobalHandles::Destroy(
-          reinterpret_cast<Object**>(WasmCompiledModule::GetTableValue(
-              compiled_module->ptr_to_empty_function_tables(), i)));
-      GlobalHandles::Destroy(
-          reinterpret_cast<Object**>(WasmCompiledModule::GetTableValue(
-              compiled_module->ptr_to_empty_signature_tables(), i)));
-    }
-  }
-  GlobalHandles::Destroy(reinterpret_cast<Object**>(p));
-}
-
 Handle<WasmTableObject> WasmTableObject::New(Isolate* isolate, uint32_t initial,
                                              int64_t maximum,
                                              Handle<FixedArray>* js_functions) {
@@ -300,12 +276,6 @@ void WasmTableObject::grow(Isolate* isolate, uint32_t count) {
       WasmCompiledModule::UpdateTableValue(
           compiled_module->ptr_to_signature_tables(), table_index,
           new_signature_table_addr);
-      // We need to destroy the global handles this instance held to the
-      // old tables now, otherwise we'd leak global handles.
-      GlobalHandles::Destroy(
-          reinterpret_cast<Object**>(old_function_table_addr));
-      GlobalHandles::Destroy(
-          reinterpret_cast<Object**>(old_signature_table_addr));
     }
   }
 }
@@ -942,29 +912,6 @@ Address WasmCompiledModule::GetTableValue(FixedArray* table, int index) {
   return reinterpret_cast<Address>(static_cast<size_t>(value));
 }
 
-void WasmCompiledModule::DestroyGlobalHandles(
-    Isolate* isolate, WasmCompiledModule* compiled_module) {
-  DisallowHeapAllocation no_gc;
-  if (compiled_module->has_function_tables()) {
-    FixedArray* function_tables = compiled_module->ptr_to_function_tables();
-    FixedArray* signature_tables = compiled_module->ptr_to_signature_tables();
-    FixedArray* empty_function_tables =
-        compiled_module->ptr_to_empty_function_tables();
-    // We don't release the placeholder ("empty") handles here, we do so
-    // in {WasmModuleObject::Finalizer}.
-    if (function_tables != empty_function_tables) {
-      for (int i = 0, e = function_tables->length(); i < e; ++i) {
-        GlobalHandleAddress func_addr =
-            WasmCompiledModule::GetTableValue(function_tables, i);
-        GlobalHandleAddress sig_addr =
-            WasmCompiledModule::GetTableValue(signature_tables, i);
-        GlobalHandles::Destroy(reinterpret_cast<Object**>(func_addr));
-        GlobalHandles::Destroy(reinterpret_cast<Object**>(sig_addr));
-      }
-    }
-  }
-}
-
 void WasmCompiledModule::Reset(Isolate* isolate,
                                WasmCompiledModule* compiled_module) {
   DisallowHeapAllocation no_gc;
@@ -994,10 +941,7 @@ void WasmCompiledModule::Reset(Isolate* isolate,
       compiled_module->set_globals_start(0);
     }
 
-    // Reset function tables. The global handles have been freed (see
-    // {DestroyGlobalHandles})
-    // The pointers are still embedded in code - so we want to reset
-    // them with the "empty" ones.
+    // Reset function tables.
     if (compiled_module->has_function_tables()) {
       FixedArray* function_tables = compiled_module->ptr_to_function_tables();
       FixedArray* signature_tables = compiled_module->ptr_to_signature_tables();
@@ -1212,6 +1156,8 @@ void WasmCompiledModule::ReinitializeAfterDeserialization(
     }
   }
 
+  // Reset, but don't delete any global handles, because their owning instance
+  // may still be active.
   WasmCompiledModule::Reset(isolate, *compiled_module);
   DCHECK(WasmSharedModuleData::IsWasmSharedModuleData(*shared));
 }

src/wasm/wasm-objects.h

@@ -23,33 +23,7 @@ namespace internal {
 namespace wasm {
 class InterpretedFrame;
 class WasmInterpreter;
-
-// When we compile or instantiate, we need to create global handles
-// for function tables. Normally, these handles get destroyed when the
-// respective objects get GCed. If we fail to construct those objects,
-// we can leak global handles. The exit path in these cases isn't unique,
-// and may grow.
-//
-// This type addresses that.
-
 typedef Address GlobalHandleAddress;
-
-class GlobalHandleLifetimeManager final {
- public:
-  void Add(GlobalHandleAddress addr) { handles_.push_back(addr); }
-  // Call this when compilation or instantiation has succeeded, and we've
-  // passed the control to a JS object with a finalizer that'll destroy
-  // the handles.
-  void ReleaseWithoutDestroying() { handles_.clear(); }
-  ~GlobalHandleLifetimeManager() {
-    for (auto& addr : handles_) {
-      GlobalHandles::Destroy(reinterpret_cast<Object**>(addr));
-    }
-  }
-
- private:
-  std::vector<Address> handles_;
-};
 }  // namespace wasm
 
 class WasmCompiledModule;
@@ -91,9 +65,6 @@ class WasmModuleObject : public JSObject {
 
   static Handle<WasmModuleObject> New(
       Isolate* isolate, Handle<WasmCompiledModule> compiled_module);
-
- private:
-  static void Finalizer(const v8::WeakCallbackInfo<void>& data);
 };
 
 // Representation of a WebAssembly.Table JavaScript-level object.
@@ -179,6 +150,9 @@ class WasmInstanceObject : public JSObject {
   DECL_OPTIONAL_ACCESSORS(memory_buffer, JSArrayBuffer)
   DECL_OPTIONAL_ACCESSORS(globals_buffer, JSArrayBuffer)
   DECL_OPTIONAL_ACCESSORS(debug_info, WasmDebugInfo)
+  DECL_OPTIONAL_ACCESSORS(function_tables, FixedArray)
+  DECL_OPTIONAL_ACCESSORS(signature_tables, FixedArray)
+
   // FixedArray of all instances whose code was imported
   DECL_OPTIONAL_ACCESSORS(directly_called_instances, FixedArray)
 
@@ -189,6 +163,8 @@ class WasmInstanceObject : public JSObject {
     kMemoryBufferIndex,
     kGlobalsBufferIndex,
     kDebugInfoIndex,
+    kFunctionTablesIndex,
+    kSignatureTablesIndex,
     kDirectlyCalledInstancesIndex,
     kFieldCount
   };
@@ -200,6 +176,8 @@ class WasmInstanceObject : public JSObject {
   DEF_OFFSET(MemoryBuffer)
   DEF_OFFSET(GlobalsBuffer)
   DEF_OFFSET(DebugInfo)
+  DEF_OFFSET(FunctionTables)
+  DEF_OFFSET(SignatureTables)
   DEF_OFFSET(DirectlyCalledInstances)
 
   WasmModuleObject* module_object();
@@ -422,8 +400,6 @@ class WasmCompiledModule : public FixedArray {
   static Handle<WasmCompiledModule> Clone(Isolate* isolate,
                                           Handle<WasmCompiledModule> module);
   static void Reset(Isolate* isolate, WasmCompiledModule* module);
-  static void DestroyGlobalHandles(Isolate* isolate,
-                                   WasmCompiledModule* compiled_module);
 
   inline Address GetEmbeddedMemStartOrNull() const;
   inline Address GetGlobalsStartOrNull() const;