[Code Health] Clean up gin function templates on isolate disposal
gin previously relied exclusively on garbage collection in order to
trigger cleanup of function templates. This is unreliable as garbage
collection is never guaranteed to run, especially for short-lived
isolates.
This change adds observers for isolate disposal in order to clean up
function templates once an isolate is disposed.
A few (possibly non-exhaustive) leaked dangling pointers are fixed
with this change.
Bug: 1462087
AX-Relnotes: n/a
Change-Id: I2462de9cfb9622c03bc047b4cd727c6bf3cd3b11
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5104930
Commit-Queue: Ashley Newson <[email protected]>
Reviewed-by: Katie Dektar <[email protected]>
Reviewed-by: Michael Lippautz <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1244593}
diff --git a/gin/BUILD.gn b/gin/BUILD.gn
index 56bf66a..a2af2a1 100644
--- a/gin/BUILD.gn
+++ b/gin/BUILD.gn
@@ -200,6 +200,7 @@
"converter_unittest.cc",
"data_object_builder_unittest.cc",
"interceptor_unittest.cc",
+ "isolate_holder_unittest.cc",
"per_context_data_unittest.cc",
"shell_runner_unittest.cc",
"test/run_all_unittests.cc",
diff --git a/gin/function_template.cc b/gin/function_template.cc
index bb4b0d5..d4c615c 100644
--- a/gin/function_template.cc
+++ b/gin/function_template.cc
@@ -4,14 +4,34 @@
#include "gin/function_template.h"
+#include "base/observer_list.h"
#include "base/strings/strcat.h"
namespace gin {
namespace internal {
+CallbackHolderBase::DisposeObserver::DisposeObserver(
+ gin::PerIsolateData* per_isolate_data,
+ CallbackHolderBase* holder)
+ : per_isolate_data_(*per_isolate_data), holder_(*holder) {
+ per_isolate_data_->AddDisposeObserver(this);
+}
+CallbackHolderBase::DisposeObserver::~DisposeObserver() {
+ per_isolate_data_->RemoveDisposeObserver(this);
+}
+void CallbackHolderBase::DisposeObserver::OnBeforeDispose(
+ v8::Isolate* isolate) {
+ holder_->v8_ref_.Reset();
+}
+void CallbackHolderBase::DisposeObserver::OnDisposed() {
+ // The holder contains the observer, so the observer is destroyed here also.
+ delete &holder_.get();
+}
+
CallbackHolderBase::CallbackHolderBase(v8::Isolate* isolate)
- : v8_ref_(isolate, v8::External::New(isolate, this)) {
+ : v8_ref_(isolate, v8::External::New(isolate, this)),
+ dispose_observer_(PerIsolateData::From(isolate), this) {
v8_ref_.SetWeak(this, &CallbackHolderBase::FirstWeakCallback,
v8::WeakCallbackType::kParameter);
}
diff --git a/gin/function_template.h b/gin/function_template.h
index f095a261..cc6b966 100644
--- a/gin/function_template.h
+++ b/gin/function_template.h
@@ -12,10 +12,12 @@
#include "base/check.h"
#include "base/functional/callback.h"
#include "base/memory/raw_ptr.h"
+#include "base/observer_list.h"
#include "base/strings/strcat.h"
#include "gin/arguments.h"
#include "gin/converter.h"
#include "gin/gin_export.h"
+#include "gin/per_isolate_data.h"
#include "v8/include/v8-external.h"
#include "v8/include/v8-forward.h"
#include "v8/include/v8-persistent-handle.h"
@@ -47,6 +49,14 @@
// base::RepeatingCallback from CreateFunctionTemplate through v8 (via
// v8::FunctionTemplate) to DispatchToCallback, where it is invoked.
+// CallbackHolder will clean up the callback in two different scenarios:
+// - If the garbage collector finds that it's garbage and collects it. (But note
+// that even _if_ we become garbage, we might never get collected!)
+// - If the isolate gets disposed.
+//
+// TODO(crbug.com/1285119): When gin::Wrappable gets migrated over to using
+// cppgc, this class should also be considered for migration.
+
// This simple base class is used so that we can share a single object template
// among every CallbackHolder instance.
class GIN_EXPORT CallbackHolderBase {
@@ -61,12 +71,26 @@
virtual ~CallbackHolderBase();
private:
+ class DisposeObserver : gin::PerIsolateData::DisposeObserver {
+ public:
+ DisposeObserver(gin::PerIsolateData* per_isolate_data,
+ CallbackHolderBase* holder);
+ ~DisposeObserver() override;
+ void OnBeforeDispose(v8::Isolate* isolate) override;
+ void OnDisposed() override;
+
+ private:
+ const raw_ref<gin::PerIsolateData> per_isolate_data_;
+ const raw_ref<CallbackHolderBase> holder_;
+ };
+
static void FirstWeakCallback(
const v8::WeakCallbackInfo<CallbackHolderBase>& data);
static void SecondWeakCallback(
const v8::WeakCallbackInfo<CallbackHolderBase>& data);
v8::Global<v8::External> v8_ref_;
+ DisposeObserver dispose_observer_;
};
template<typename Sig>
@@ -269,6 +293,12 @@
// internal reasons, thus it is generally a good idea to cache the template
// returned by this function. Otherwise, repeated method invocations from JS
// will create substantial memory leaks. See http://crbug.com/463487.
+//
+// The callback will be destroyed if either the function template gets garbage
+// collected or _after_ the isolate is disposed. Garbage collection can never be
+// relied upon. As such, any destructors for objects bound to the callback must
+// not depend on the isolate being alive at the point they are called. The order
+// in which callbacks are destroyed is not guaranteed.
template <typename Sig>
v8::Local<v8::FunctionTemplate> CreateFunctionTemplate(
v8::Isolate* isolate,
diff --git a/gin/isolate_holder.cc b/gin/isolate_holder.cc
index 4cb88241..cdaa098 100644
--- a/gin/isolate_holder.cc
+++ b/gin/isolate_holder.cc
@@ -9,6 +9,7 @@
#include <string.h>
#include <memory>
+#include <optional>
#include <utility>
#include "base/check_op.h"
@@ -23,6 +24,7 @@
#include "gin/v8_isolate_memory_dump_provider.h"
#include "gin/v8_shared_memory_dump_provider.h"
#include "v8/include/v8-isolate.h"
+#include "v8/include/v8-locker.h"
#include "v8/include/v8-snapshot.h"
namespace gin {
@@ -127,9 +129,18 @@
IsolateHolder::~IsolateHolder() {
isolate_memory_dump_provider_.reset();
+ {
+ std::optional<v8::Locker> locker;
+ if (access_mode_ == AccessMode::kUseLocker) {
+ locker.emplace(isolate_);
+ }
+ v8::Isolate::Scope isolate_scope(isolate_);
+ isolate_data_->NotifyBeforeDispose();
+ }
// Calling Isolate::Dispose makes sure all threads which might access
// PerIsolateData are finished.
isolate_->Dispose();
+ isolate_data_->NotifyDisposed();
isolate_data_.reset();
isolate_ = nullptr;
}
diff --git a/gin/isolate_holder_unittest.cc b/gin/isolate_holder_unittest.cc
new file mode 100644
index 0000000..f7db686
--- /dev/null
+++ b/gin/isolate_holder_unittest.cc
@@ -0,0 +1,46 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "gin/public/isolate_holder.h"
+
+#include "base/task/single_thread_task_runner.h"
+#include "gin/converter.h"
+#include "gin/function_template.h"
+#include "gin/test/v8_test.h"
+#include "v8/include/v8-context.h"
+#include "v8/include/v8-isolate.h"
+#include "v8/include/v8-local-handle.h"
+#include "v8/include/v8-template.h"
+
+namespace gin {
+
+// Verifies IsolateHolder can be constructed and destructed in kUseLocker access
+// mode. These tests doesn't specifically/deliberately exercise anything
+// multi-threaded.
+class UseLockerIsolateHolderTest : public V8Test {
+ gin::IsolateHolder::AccessMode AccessMode() const override {
+ return gin::IsolateHolder::AccessMode::kUseLocker;
+ }
+};
+
+// This test exercises teardown of function templates with isolate holders.
+TEST_F(UseLockerIsolateHolderTest, FunctionTemplateLifetime) {
+ v8::Isolate* isolate = instance_->isolate();
+ v8::HandleScope handle_scope(isolate);
+ v8::Local<v8::Context> context = context_.Get(isolate);
+ v8::Context::Scope context_scope(context);
+ v8::Local<v8::ObjectTemplate> object_template =
+ v8::ObjectTemplate::New(isolate);
+ object_template->Set(
+ isolate, "someFunction",
+ gin::CreateFunctionTemplate(isolate, base::BindRepeating([]() {})));
+ context->Global()
+ ->Set(context, StringToV8(isolate, "someObjectTemplate"),
+ object_template->NewInstance(context).ToLocalChecked())
+ .Check();
+ // There's nothing specific to assert - we just want to make sure that it
+ // doesn't crash on teardown.
+}
+
+} // namespace gin
diff --git a/gin/per_isolate_data.cc b/gin/per_isolate_data.cc
index 5dfd0a5..a187526 100644
--- a/gin/per_isolate_data.cc
+++ b/gin/per_isolate_data.cc
@@ -139,6 +139,26 @@
return NULL;
}
+void PerIsolateData::AddDisposeObserver(DisposeObserver* observer) {
+ dispose_observers_.AddObserver(observer);
+}
+
+void PerIsolateData::RemoveDisposeObserver(DisposeObserver* observer) {
+ dispose_observers_.RemoveObserver(observer);
+}
+
+void PerIsolateData::NotifyBeforeDispose() {
+ for (auto& observer : dispose_observers_) {
+ observer.OnBeforeDispose(isolate_.get());
+ }
+}
+
+void PerIsolateData::NotifyDisposed() {
+ for (auto& observer : dispose_observers_) {
+ observer.OnDisposed();
+ }
+}
+
void PerIsolateData::EnableIdleTasks(
std::unique_ptr<V8IdleTaskRunner> idle_task_runner) {
task_runner_->EnableIdleTasks(std::move(idle_task_runner));
diff --git a/gin/per_isolate_data.h b/gin/per_isolate_data.h
index 6353521..3d3f07c 100644
--- a/gin/per_isolate_data.h
+++ b/gin/per_isolate_data.h
@@ -8,8 +8,10 @@
#include <map>
#include <memory>
+#include "base/check.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/ref_counted.h"
+#include "base/observer_list.h"
#include "base/task/single_thread_task_runner.h"
#include "gin/gin_export.h"
#include "gin/public/isolate_holder.h"
@@ -29,6 +31,16 @@
// class stores all the Gin-related data that varies per isolate.
class GIN_EXPORT PerIsolateData {
public:
+ class DisposeObserver : public base::CheckedObserver {
+ public:
+ // Called just before the isolate is about to be disposed. The isolate will
+ // be entered before the observer is notified, but there will not be a
+ // handle scope by default.
+ virtual void OnBeforeDispose(v8::Isolate* isolate) = 0;
+ // Called just after the isolate has been disposed.
+ virtual void OnDisposed() = 0;
+ };
+
PerIsolateData(
v8::Isolate* isolate,
v8::ArrayBuffer::Allocator* allocator,
@@ -72,6 +84,11 @@
WrappableBase* base);
NamedPropertyInterceptor* GetNamedPropertyInterceptor(WrappableBase* base);
+ void AddDisposeObserver(DisposeObserver* observer);
+ void RemoveDisposeObserver(DisposeObserver* observer);
+ void NotifyBeforeDispose();
+ void NotifyDisposed();
+
void EnableIdleTasks(std::unique_ptr<V8IdleTaskRunner> idle_task_runner);
v8::Isolate* isolate() { return isolate_; }
@@ -99,6 +116,7 @@
FunctionTemplateMap function_templates_;
IndexedPropertyInterceptorMap indexed_interceptors_;
NamedPropertyInterceptorMap named_interceptors_;
+ base::ObserverList<DisposeObserver> dispose_observers_;
std::shared_ptr<V8ForegroundTaskRunnerBase> task_runner_;
std::shared_ptr<V8ForegroundTaskRunnerBase> low_priority_task_runner_;
};
diff --git a/gin/test/v8_test.cc b/gin/test/v8_test.cc
index a4106b6..3e58bc78 100644
--- a/gin/test/v8_test.cc
+++ b/gin/test/v8_test.cc
@@ -35,6 +35,9 @@
gin::ArrayBufferAllocator::SharedInstance());
instance_ = CreateIsolateHolder();
+ if (AccessMode() == gin::IsolateHolder::AccessMode::kUseLocker) {
+ locker_.emplace(instance_->isolate());
+ }
instance_->isolate()->Enter();
HandleScope handle_scope(instance_->isolate());
context_.Reset(instance_->isolate(), Context::New(instance_->isolate()));
@@ -48,13 +51,18 @@
context_.Reset();
}
instance_->isolate()->Exit();
+ locker_.reset();
instance_.reset();
}
std::unique_ptr<gin::IsolateHolder> V8Test::CreateIsolateHolder() const {
return std::make_unique<gin::IsolateHolder>(
- base::SingleThreadTaskRunner::GetCurrentDefault(),
+ base::SingleThreadTaskRunner::GetCurrentDefault(), AccessMode(),
gin::IsolateHolder::IsolateType::kBlinkMainThread);
}
+gin::IsolateHolder::AccessMode V8Test::AccessMode() const {
+ return gin::IsolateHolder::AccessMode::kSingleThread;
+}
+
} // namespace gin
diff --git a/gin/test/v8_test.h b/gin/test/v8_test.h
index 9c9d202..428736f 100644
--- a/gin/test/v8_test.h
+++ b/gin/test/v8_test.h
@@ -6,17 +6,18 @@
#define GIN_TEST_V8_TEST_H_
#include <memory>
+#include <optional>
#include "base/compiler_specific.h"
#include "base/test/task_environment.h"
+#include "gin/public/isolate_holder.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "v8/include/v8-forward.h"
+#include "v8/include/v8-locker.h"
#include "v8/include/v8-persistent-handle.h"
namespace gin {
-class IsolateHolder;
-
// V8Test is a simple harness for testing interactions with V8. V8Test doesn't
// have any dependencies on Gin's module system.
class V8Test : public testing::Test {
@@ -32,8 +33,10 @@
protected:
// This is used during SetUp() to initialize instance_.
virtual std::unique_ptr<IsolateHolder> CreateIsolateHolder() const;
+ virtual gin::IsolateHolder::AccessMode AccessMode() const;
base::test::TaskEnvironment task_environment_;
std::unique_ptr<IsolateHolder> instance_;
+ std::optional<v8::Locker> locker_;
v8::Persistent<v8::Context> context_;
};
diff --git a/gin/v8_foreground_task_runner_with_locker.h b/gin/v8_foreground_task_runner_with_locker.h
index d3242de..0ef509b 100644
--- a/gin/v8_foreground_task_runner_with_locker.h
+++ b/gin/v8_foreground_task_runner_with_locker.h
@@ -36,7 +36,15 @@
bool NonNestableTasksEnabled() const override;
private:
- raw_ptr<v8::Isolate> isolate_;
+ // This dangles because the isolate must be disposed before the task runner
+ // can safely be destroyed. V8-managed tasks in other threads might try to
+ // post more tasks whilst the isolate is being disposed (before V8 cancels
+ // them as part of disposal).
+ //
+ // Once the isolate is disposed, V8 has made sure that no more tasks should be
+ // running or get posted, and this task runner will quickly get destroyed
+ // afterwards.
+ raw_ptr<v8::Isolate, DanglingUntriaged> isolate_;
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
};
