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/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) {}
-
-    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.
+  //
+  // Bound to the lifetime of |object|.
+  class GCCallback {
+   public:
+    GCCallback(v8::Handle<v8::Object> object, v8::Handle<v8::Function> callback)
+        : object_(object), callback_(callback) {
+      object_.MakeWeak(this, NearDeathCallback);
+    }
 
    private:
-    DISALLOW_COPY_AND_ASSIGN(GCCallbackArgs);
+    friend class base::DeleteHelper<GCCallback>;
+
+    static void NearDeathCallback(v8::Isolate* isolate,
+                                  v8::Persistent<v8::Object>* object,
+                                  GCCallback* self) {

[adamk, 2013/07/23 22:47:38]

I think you want to call

self->object_.reset()

here to allow |object| to die.

[not at google - send to devlin, 2013/07/23 22:55:04]

ah good point.

[Jeffrey Yasskin, 2013/07/23 22:59:35]

And possibly link to
https://code.google.com/p/chromium/codesearch/#chromium/src/v8/include/v8.h&l...
since the obvious place,
https://code.google.com/p/chromium/codesearch/#chromium/src/v8/include/v8.h&l...
doesn't say anything about needing to do that.

[not at google - send to devlin, 2013/07/23 23:17:41]

Added comment (though not to the source itself, arbitrary line numbers will
change...)

+      // Delete this and execute function in destructor to be safe across
+      // message loop shutdown and avoid leaking any v8 state.
+      base::MessageLoop::current()->DeleteSoon(FROM_HERE, self);

[Jeffrey Yasskin, 2013/07/23 22:59:35]

I'd just PostTask(Bind(&GCCallback::DoStuff, Owned(self))). DeleteSoon doesn't
do anything special to actually delete objects if the MessageLoop has shut down,
but Bind(Owned()) would delete the object immediately.

[Jeffrey Yasskin, 2013/07/23 22:59:35]

After posting to the MessageLoop, how do we know that callback_ still has a
valid reference to V8?

[not at google - send to devlin, 2013/07/23 23:17:41]

Done.

[not at google - send to devlin, 2013/07/23 23:17:41]

callback_ is Persistent, so it will "by definition".

Though Adam, do we need to do something special when webkit decides to shut down
v8?

[adamk, 2013/07/23 23:43:21]

All the entry points check for a dead Isolate before trying to do anything, so I
don't believe there's anything you need to do here...

+    }
+
+    ~GCCallback() {
+      v8::HandleScope handle_scope;
+      v8::Handle<v8::Context> context = callback_->CreationContext();

[adamk, 2013/07/23 23:43:21]

...except that this Handle will be empty if the isolate's dead (e.g., due to
OOM), so the next line would crash. I guess you'd have to check for
context.IsEmpty() to be totally safe, but crashing is likely fine in that case
(in fact, the OOM should have already caused a crash).

Sad that the C++ version of this turns out to be more complicated, but I still
think it's better than forcing the callers to know about this stuff.

As long as this code is only ever used on the main thread, I think the code is
likely fine as is.

+      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;
+      callback_->Call(context->Global(), 0, NULL);
+    }
+
+    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);
+    new GCCallback(args[0].As<v8::Object>(), args[1].As<v8::Function>());

[Jeffrey Yasskin, 2013/07/23 22:59:35]

If possible, I'd rather not have code that looks like an obvious leak. Make it a
static method or call object_.MakeWeak() out here so readers can see where
ownership of the pointer probably went.

   }
 };
 
 }  // 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