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

chrome/renderer/extensions/miscellaneous_bindings.cc

 // Copyright (c) 2012 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 "chrome/renderer/extensions/miscellaneous_bindings.h"
 
 #include <map>
 #include <string>
 
 #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"
 #include "chrome/common/url_constants.h"
 #include "chrome/renderer/extensions/chrome_v8_context.h"
 #include "chrome/renderer/extensions/chrome_v8_context_set.h"
 #include "chrome/renderer/extensions/chrome_v8_extension.h"
 #include "chrome/renderer/extensions/dispatcher.h"
 #include "chrome/renderer/extensions/event_bindings.h"
 #include "chrome/renderer/extensions/scoped_persistent.h"
@@ skipping 130 matching lines @@
 
     int port_id = args[0]->Int32Value();
     if (HasPortData(port_id) && --GetPortData(port_id).ref_count == 0) {
       // Send via the RenderThread because the RenderView might be closing.
       content::RenderThread::Get()->Send(
           new ExtensionHostMsg_CloseChannel(port_id, std::string()));
       ClearPortData(port_id);
     }
   }
 
-  struct GCCallbackArgs {
-    GCCallbackArgs(v8::Handle<v8::Object> object,
-                   v8::Handle<v8::Function> callback)
-        : object(object), callback(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) {
+      new GCCallback(object, callback);

[Jeffrey Yasskin, 2013/07/23 23:52:47]

Now this looks like an obvious leak. Just move the object_.MakeWeak into this
function.

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

Not obvious to me, but ok done.

+    }
 
-    extensions::ScopedPersistent<v8::Object> object;
-    extensions::ScopedPersistent<v8::Function> callback;
+    // Public for base::Owned.
+    ~GCCallback() {
+      v8::HandleScope handle_scope;
+      v8::Handle<v8::Context> context = callback_->CreationContext();
+      v8::Context::Scope context_scope(context);
+      WebKit::WebScopedMicrotaskSuppression suppression;
+      callback_->Call(context->Global(), 0, NULL);
+    }
 
    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
+      // message loop shutdown and avoid leaking any v8 state.
+      base::MessageLoop::current()->PostTask(FROM_HERE,
+          base::Bind(&GCCallback::Nothing, base::Owned(self)));
+    }
+
+    GCCallback(v8::Handle<v8::Object> object, v8::Handle<v8::Function> callback)
+        : object_(object), callback_(callback) {
+      object_.MakeWeak(this, NearDeathCallback);
+    }
+
+    void Nothing() {
+      // Just something to bind to so that this can be deleted via base::Owned.

[Jeffrey Yasskin, 2013/07/23 23:52:47]

Do the work in here, not in the destructor. Destructors should generally not
have visible side-effects, and in this case, if the MessageLoop is gone, you
almost certainly want to just delete the GCCallback, and not call back into
Javascript in the middle of the gc.

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

Done.

+    }
+
+    extensions::ScopedPersistent<v8::Object> object_;
+    extensions::ScopedPersistent<v8::Function> callback_;
+
+    DISALLOW_COPY_AND_ASSIGN(GCCallback);
   };
 
-  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;
-  }
-
-  // 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>());
   }
 };
 
 }  // namespace
 
 namespace extensions {
 
 ChromeV8Extension* MiscellaneousBindings::Get(
     Dispatcher* dispatcher,
     ChromeV8Context* context) {
@@ skipping 133 matching lines @@
     } else {
       arguments.push_back(v8::Null());
     }
     (*it)->module_system()->CallModuleMethod("miscellaneous_bindings",
                                              "dispatchOnDisconnect",
                                              &arguments);
   }
 }
 
 }  // namespace extensions