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
R=rdevlin.cronin@chromium.org,jochen@chromium.org,adamk@chromium.org

Review URL: https://codereview.chromium.org/1250093008

Cr-Commit-Position: refs/heads/master@{#340224}

6 files changed, 276 insertions, 66 deletions

diff --git a/extensions/extensions.gypi b/extensions/extensions.gypi
index 9b26f23..f6ee60d 100644
--- a/extensions/extensions.gypi
+++ b/extensions/extensions.gypi
@@ -894,6 +894,8 @@
       'renderer/extensions_renderer_client.h',
       'renderer/file_system_natives.cc',
       'renderer/file_system_natives.h',
+      'renderer/gc_callback.cc',
+      'renderer/gc_callback.h',
       'renderer/guest_view/extensions_guest_view_container.cc',
       'renderer/guest_view/extensions_guest_view_container.h',
       'renderer/guest_view/extensions_guest_view_container_dispatcher.cc',
diff --git a/extensions/extensions_tests.gypi b/extensions/extensions_tests.gypi
index c8b422b..eed12fb 100644
--- a/extensions/extensions_tests.gypi
+++ b/extensions/extensions_tests.gypi
@@ -145,6 +145,7 @@
       'renderer/api_test_base.h',
       'renderer/api_test_base_unittest.cc',
       'renderer/event_unittest.cc',
+      'renderer/gc_callback_unittest.cc',
       'renderer/json_schema_unittest.cc',
       'renderer/messaging_utils_unittest.cc',
       'renderer/module_system_test.cc',
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..072c191
--- /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 @@ using v8_helpers::IsEmptyOrUndefied;
 
 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: