Bypass the v8 GC second pass callback set up by GCCallback for extension messaging.
This fixes a bug where an extension script context was being invalidated in
between calls for the first and second pass v8::PersistentBase::SetWeak
callbacks, which would invalidate a base::WeakPtr and crash. This bug can be
trivially bypassed by removing the second pass entirely and relying on the post
back to the current message loop.
This also adds 2 tests, one where the script context is invalidated before GC,
the other where GC happens before the script context is invalidated.
This patch is a combination of jochen's https://crrev.com/1247093005 and
kalman's https://crrev.com/1242273007, see those patches for more comments.
BUG=512080
[email protected],[email protected],[email protected]
Review URL: https://codereview.chromium.org/1250093008
Cr-Commit-Position: refs/heads/master@{#340224}
diff --git a/extensions/renderer/gc_callback.cc b/extensions/renderer/gc_callback.cc
new file mode 100644
index 0000000..9f4a1d5
--- /dev/null
+++ b/extensions/renderer/gc_callback.cc
@@ -0,0 +1,55 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/renderer/gc_callback.h"
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "extensions/renderer/script_context.h"
+
+namespace extensions {
+
+GCCallback::GCCallback(ScriptContext* context,
+ const v8::Local<v8::Object>& object,
+ const v8::Local<v8::Function>& callback,
+ const base::Closure& fallback)
+ : context_(context),
+ object_(context->isolate(), object),
+ callback_(context->isolate(), callback),
+ fallback_(fallback),
+ weak_ptr_factory_(this) {
+ object_.SetWeak(this, OnObjectGC, v8::WeakCallbackType::kParameter);
+ context->AddInvalidationObserver(base::Bind(&GCCallback::OnContextInvalidated,
+ weak_ptr_factory_.GetWeakPtr()));
+}
+
+GCCallback::~GCCallback() {}
+
+// static
+void GCCallback::OnObjectGC(const v8::WeakCallbackInfo<GCCallback>& data) {
+ // Usually FirstWeakCallback should do nothing other than reset |object_|
+ // and then set a second weak callback to run later. We can sidestep that,
+ // because posting a task to the current message loop is all but free - but
+ // DO NOT add any more work to this method. The only acceptable place to add
+ // code is RunCallback.
+ GCCallback* self = data.GetParameter();
+ self->object_.Reset();
+ base::MessageLoop::current()->PostTask(
+ FROM_HERE, base::Bind(&GCCallback::RunCallback,
+ self->weak_ptr_factory_.GetWeakPtr()));
+}
+
+void GCCallback::RunCallback() {
+ v8::Isolate* isolate = context_->isolate();
+ v8::HandleScope handle_scope(isolate);
+ context_->CallFunction(v8::Local<v8::Function>::New(isolate, callback_));
+ delete this;
+}
+
+void GCCallback::OnContextInvalidated() {
+ fallback_.Run();
+ delete this;
+}
+
+} // namespace extensions
diff --git a/extensions/renderer/gc_callback.h b/extensions/renderer/gc_callback.h
new file mode 100644
index 0000000..072c191c
--- /dev/null
+++ b/extensions/renderer/gc_callback.h
@@ -0,0 +1,56 @@
+// Copyright 2015 The Chromium 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 EXTENSIONS_RENDERER_GC_CALLBACK_H_
+#define EXTENSIONS_RENDERER_GC_CALLBACK_H_
+
+#include <map>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "v8/include/v8.h"
+
+namespace extensions {
+
+class ScriptContext;
+
+// Runs |callback| when v8 garbage collects |object|, or |fallback| if
+// |context| is invalidated first. Exactly one of |callback| or |fallback| will
+// be called, after which it deletes itself.
+class GCCallback {
+ public:
+ GCCallback(ScriptContext* context,
+ const v8::Local<v8::Object>& object,
+ const v8::Local<v8::Function>& callback,
+ const base::Closure& fallback);
+ ~GCCallback();
+
+ private:
+ static void OnObjectGC(const v8::WeakCallbackInfo<GCCallback>& data);
+ void RunCallback();
+ void OnContextInvalidated();
+
+ // The context which owns |object_|.
+ ScriptContext* context_;
+
+ // The object this GCCallback is bound to.
+ v8::Global<v8::Object> object_;
+
+ // The function to run when |object_| is garbage collected.
+ v8::Global<v8::Function> callback_;
+
+ // The function to run if |context_| is invalidated before we have a chance
+ // to execute |callback_|.
+ base::Closure fallback_;
+
+ base::WeakPtrFactory<GCCallback> weak_ptr_factory_;
+
+ DISALLOW_COPY_AND_ASSIGN(GCCallback);
+};
+
+} // namespace extensions
+
+#endif // EXTENSIONS_RENDERER_GC_CALLBACK_H_
diff --git a/extensions/renderer/gc_callback_unittest.cc b/extensions/renderer/gc_callback_unittest.cc
new file mode 100644
index 0000000..1686884
--- /dev/null
+++ b/extensions/renderer/gc_callback_unittest.cc
@@ -0,0 +1,161 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/bind.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_set.h"
+#include "extensions/common/features/feature.h"
+#include "extensions/renderer/gc_callback.h"
+#include "extensions/renderer/scoped_web_frame.h"
+#include "extensions/renderer/script_context.h"
+#include "extensions/renderer/script_context_set.h"
+#include "gin/function_template.h"
+#include "gin/public/context_holder.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/WebKit/public/web/WebFrame.h"
+#include "v8/include/v8.h"
+
+namespace extensions {
+namespace {
+
+void SetToTrue(bool* value) {
+ if (*value)
+ ADD_FAILURE() << "Value is already true";
+ *value = true;
+}
+
+class GCCallbackTest : public testing::Test {
+ public:
+ GCCallbackTest() : script_context_set_(&extensions_, &active_extensions_) {}
+
+ protected:
+ base::MessageLoop& message_loop() { return message_loop_; }
+
+ ScriptContextSet& script_context_set() { return script_context_set_; }
+
+ v8::Local<v8::Context> v8_context() {
+ return v8::Local<v8::Context>::New(v8::Isolate::GetCurrent(), v8_context_);
+ }
+
+ ScriptContext* RegisterScriptContext() {
+ // No extension group or world ID.
+ return script_context_set_.Register(
+ web_frame_.frame(),
+ v8::Local<v8::Context>::New(v8::Isolate::GetCurrent(), v8_context_), 0,
+ 0);
+ }
+
+ void RequestGarbageCollection() {
+ v8::Isolate::GetCurrent()->RequestGarbageCollectionForTesting(
+ v8::Isolate::kFullGarbageCollection);
+ }
+
+ private:
+ void SetUp() override {
+ v8::Isolate* isolate = v8::Isolate::GetCurrent();
+ v8::HandleScope handle_scope(isolate);
+ v8::Local<v8::Context> local_v8_context = v8::Context::New(isolate);
+ v8_context_.Reset(isolate, local_v8_context);
+ // ScriptContexts rely on gin.
+ gin_context_holder_.reset(new gin::ContextHolder(isolate));
+ gin_context_holder_->SetContext(local_v8_context);
+ }
+
+ void TearDown() override {
+ gin_context_holder_.reset();
+ v8_context_.Reset();
+ RequestGarbageCollection();
+ }
+
+ base::MessageLoop message_loop_;
+ ScopedWebFrame web_frame_; // (this will construct the v8::Isolate)
+ ExtensionSet extensions_;
+ ExtensionIdSet active_extensions_;
+ ScriptContextSet script_context_set_;
+ v8::Global<v8::Context> v8_context_;
+ scoped_ptr<gin::ContextHolder> gin_context_holder_;
+
+ DISALLOW_COPY_AND_ASSIGN(GCCallbackTest);
+};
+
+TEST_F(GCCallbackTest, GCBeforeContextInvalidated) {
+ v8::Isolate* isolate = v8::Isolate::GetCurrent();
+ v8::HandleScope handle_scope(isolate);
+ v8::Context::Scope context_scope(v8_context());
+
+ ScriptContext* script_context = RegisterScriptContext();
+
+ bool callback_invoked = false;
+ bool fallback_invoked = false;
+
+ {
+ // Nest another HandleScope so that |object| and |unreachable_function|'s
+ // handles will be garbage collected.
+ v8::HandleScope handle_scope(isolate);
+ v8::Local<v8::Object> object = v8::Object::New(isolate);
+ v8::Local<v8::FunctionTemplate> unreachable_function =
+ gin::CreateFunctionTemplate(isolate,
+ base::Bind(SetToTrue, &callback_invoked));
+ // The GCCallback will delete itself, or memory tests will complain.
+ new GCCallback(script_context, object, unreachable_function->GetFunction(),
+ base::Bind(SetToTrue, &fallback_invoked));
+ }
+
+ // Trigger a GC. Only the callback should be invoked.
+ RequestGarbageCollection();
+ message_loop().RunUntilIdle();
+
+ EXPECT_TRUE(callback_invoked);
+ EXPECT_FALSE(fallback_invoked);
+
+ // Invalidate the context. The fallback should not be invoked because the
+ // callback was already invoked.
+ script_context_set().Remove(script_context);
+ message_loop().RunUntilIdle();
+
+ EXPECT_FALSE(fallback_invoked);
+}
+
+TEST_F(GCCallbackTest, ContextInvalidatedBeforeGC) {
+ v8::Isolate* isolate = v8::Isolate::GetCurrent();
+ v8::HandleScope handle_scope(isolate);
+ v8::Context::Scope context_scope(v8_context());
+
+ ScriptContext* script_context = RegisterScriptContext();
+
+ bool callback_invoked = false;
+ bool fallback_invoked = false;
+
+ {
+ // Nest another HandleScope so that |object| and |unreachable_function|'s
+ // handles will be garbage collected.
+ v8::HandleScope handle_scope(isolate);
+ v8::Local<v8::Object> object = v8::Object::New(isolate);
+ v8::Local<v8::FunctionTemplate> unreachable_function =
+ gin::CreateFunctionTemplate(isolate,
+ base::Bind(SetToTrue, &callback_invoked));
+ // The GCCallback will delete itself, or memory tests will complain.
+ new GCCallback(script_context, object, unreachable_function->GetFunction(),
+ base::Bind(SetToTrue, &fallback_invoked));
+ }
+
+ // Invalidate the context. Only the fallback should be invoked.
+ script_context_set().Remove(script_context);
+ message_loop().RunUntilIdle();
+
+ EXPECT_FALSE(callback_invoked);
+ EXPECT_TRUE(fallback_invoked);
+
+ // Trigger a GC. The callback should not be invoked because the fallback was
+ // already invoked.
+ RequestGarbageCollection();
+ message_loop().RunUntilIdle();
+
+ EXPECT_FALSE(callback_invoked);
+}
+
+} // namespace
+} // namespace extensions
diff --git a/extensions/renderer/messaging_bindings.cc b/extensions/renderer/messaging_bindings.cc
index 4017309..1e0f6e2 100644
--- a/extensions/renderer/messaging_bindings.cc
+++ b/extensions/renderer/messaging_bindings.cc
@@ -25,6 +25,7 @@
#include "extensions/common/manifest_handlers/externally_connectable.h"
#include "extensions/renderer/dispatcher.h"
#include "extensions/renderer/event_bindings.h"
+#include "extensions/renderer/gc_callback.h"
#include "extensions/renderer/object_backed_native_handler.h"
#include "extensions/renderer/script_context.h"
#include "extensions/renderer/script_context_set.h"
@@ -58,72 +59,6 @@
namespace {
-// Binds |callback| to run when |object| is garbage collected. So as to not
-// re-entrantly call into v8, we execute this function asynchronously, at
-// which point |context| may have been invalidated. If so, |callback| is not
-// run, and |fallback| will be called instead.
-//
-// Deletes itself when the object args[0] is garbage collected or when the
-// context is invalidated.
-class GCCallback : public base::SupportsWeakPtr<GCCallback> {
- public:
- GCCallback(ScriptContext* context,
- const v8::Local<v8::Object>& object,
- const v8::Local<v8::Function>& callback,
- const base::Closure& fallback)
- : context_(context),
- object_(context->isolate(), object),
- callback_(context->isolate(), callback),
- fallback_(fallback) {
- object_.SetWeak(this, FirstWeakCallback, v8::WeakCallbackType::kParameter);
- context->AddInvalidationObserver(
- base::Bind(&GCCallback::OnContextInvalidated, AsWeakPtr()));
- }
-
- private:
- static void FirstWeakCallback(const v8::WeakCallbackInfo<GCCallback>& data) {
- // v8 says we need to explicitly reset weak handles from their callbacks.
- // It's not implicit as one might expect.
- data.GetParameter()->object_.Reset();
- data.SetSecondPassCallback(SecondWeakCallback);
- }
-
- static void SecondWeakCallback(const v8::WeakCallbackInfo<GCCallback>& data) {
- base::MessageLoop::current()->PostTask(
- FROM_HERE,
- base::Bind(&GCCallback::RunCallback, data.GetParameter()->AsWeakPtr()));
- }
-
- void RunCallback() {
- CHECK(context_);
- v8::Isolate* isolate = context_->isolate();
- v8::HandleScope handle_scope(isolate);
- context_->CallFunction(v8::Local<v8::Function>::New(isolate, callback_));
- delete this;
- }
-
- void OnContextInvalidated() {
- fallback_.Run();
- context_ = NULL;
- delete this;
- }
-
- // ScriptContext which owns this GCCallback.
- ScriptContext* context_;
-
- // Holds a global handle to the object this GCCallback is bound to.
- v8::Global<v8::Object> object_;
-
- // Function to run when |object_| bound to this GCCallback is GC'd.
- v8::Global<v8::Function> callback_;
-
- // Function to run if context is invalidated before we have a chance
- // to execute |callback_|.
- base::Closure fallback_;
-
- DISALLOW_COPY_AND_ASSIGN(GCCallback);
-};
-
// Tracks every reference between ScriptContexts and Ports, by ID.
class PortTracker {
public:
