Run the Port cleanup code inside BindToGC in a setTimeout call to avoid

chrome/renderer/extensions/miscellaneous_bindings.cc

Index: chrome/renderer/extensions/miscellaneous_bindings.cc
diff --git a/chrome/renderer/extensions/miscellaneous_bindings.cc b/chrome/renderer/extensions/miscellaneous_bindings.cc
index 8921d77ea680f16736049a7b2083a1ccf6447230..5521b2566c69783a8c02b7f1791f4bd2fadd36e9 100644
--- a/chrome/renderer/extensions/miscellaneous_bindings.cc
+++ b/chrome/renderer/extensions/miscellaneous_bindings.cc
@@ -9,7 +9,9 @@
 
 #include "base/basictypes.h"
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/lazy_instance.h"
+#include "base/message_loop/message_loop.h"
 #include "base/values.h"
 #include "chrome/common/extensions/extension_messages.h"
 #include "chrome/common/extensions/message_bundle.h"
@@ -160,39 +162,55 @@ class ExtensionImpl : public extensions::ChromeV8Extension {
     }
   }
 
-  struct GCCallbackArgs {
-    GCCallbackArgs(v8::Handle<v8::Object> object,
-                   v8::Handle<v8::Function> callback)
-        : object(object), callback(callback) {}
-
-    extensions::ScopedPersistent<v8::Object> object;
-    extensions::ScopedPersistent<v8::Function> callback;
+  // Holds a |callback| to run sometime after |object| is GC'ed. |callback| will
+  // not be executed re-entrantly to avoid running JS in an unexpected state.
+  class GCCallback {
+   public:
+    static void Bind(v8::Handle<v8::Object> object,
+                     v8::Handle<v8::Function> callback) {
+      GCCallback* cb = new GCCallback(object, callback);
+      cb->object_.MakeWeak(cb, NearDeathCallback);
+    }
 
    private:
-    DISALLOW_COPY_AND_ASSIGN(GCCallbackArgs);
-  };
+    static void NearDeathCallback(v8::Isolate* isolate,
+                                  v8::Persistent<v8::Object>* object,
+                                  GCCallback* self) {
+      // v8 says we need to explicitly reset weak handles from their callbacks.
+      // It's not implicit as one might expect.
+      self->object_.reset();
+      // Delete this to execute |callback| in destructor to be safe across

[Jeffrey Yasskin, 2013/07/24 00:20:17]

This comment is out of date. Probably you don't need it anymore.

+      // message loop shutdown and avoid leaking any v8 state.
+      base::MessageLoop::current()->PostTask(FROM_HERE,
+          base::Bind(&GCCallback::RunCallback, base::Owned(self)));
+    }
 
-  static void GCCallback(v8::Isolate* isolate,
-                         v8::Persistent<v8::Object>* object,
-                         GCCallbackArgs* args) {
-    v8::HandleScope handle_scope;
-    v8::Handle<v8::Context> context = args->callback->CreationContext();
-    v8::Context::Scope context_scope(context);
-    WebKit::WebScopedMicrotaskSuppression suppression;
-    // Wrap in try/catch here so that we don't call into any message/exception
-    // handlers during GC. That is a recipe for pain.
-    v8::TryCatch trycatch;
-    args->callback->Call(context->Global(), 0, NULL);
-    delete args;
-  }
+    GCCallback(v8::Handle<v8::Object> object, v8::Handle<v8::Function> callback)
+        : object_(object), callback_(callback) {
+    }
+
+    void RunCallback() {
+      v8::HandleScope handle_scope;
+      v8::Handle<v8::Context> context = callback_->CreationContext();

[adamk, 2013/07/24 00:18:05]

I would recommend checking for non-emptyness on the context and just bailing out
if it's empty.

[not at google - send to devlin, 2013/07/24 00:21:09]

CreationContext of an object can be empty?

[adamk, 2013/07/24 00:24:47]

It can return an empty handle if the V8 heap is in a bad state. See the
ON_BAILOUT macro in v8's api.cc.

But in non-dead-isolate cases, no, the CreationContext cannot be empty (and in
fact the function will hold a strong reference to it, keeping alive the entire
Global object until we release our eference.

+      v8::Context::Scope context_scope(context);
+      WebKit::WebScopedMicrotaskSuppression suppression;
+      callback_->Call(context->Global(), 0, NULL);
+    }
+
+    extensions::ScopedPersistent<v8::Object> object_;
+    extensions::ScopedPersistent<v8::Function> callback_;
+
+    DISALLOW_COPY_AND_ASSIGN(GCCallback);
+  };
 
-  // Binds a callback to be invoked when the given object is garbage collected.
+  // void BindToGC(object, callback)
+  //
+  // Binds |callback| to be invoked *sometime after* |object| is garbage
+  // collected. We don't call the method re-entrantly so as to avoid executing
+  // JS in some bizarro undefined mid-GC state.
   void BindToGC(const v8::FunctionCallbackInfo<v8::Value>& args) {
     CHECK(args.Length() == 2 && args[0]->IsObject() && args[1]->IsFunction());
-    GCCallbackArgs* context = new GCCallbackArgs(
-        v8::Handle<v8::Object>::Cast(args[0]),
-        v8::Handle<v8::Function>::Cast(args[1]));
-    context->object.MakeWeak(context, GCCallback);
+    GCCallback::Bind(args[0].As<v8::Object>(), args[1].As<v8::Function>());
   }
 };