Fix re-entrancy and lifetime issue in RenderFrameObserverNatives::OnDocumentElementCreated

extensions/renderer/render_frame_observer_natives.cc

Index: extensions/renderer/render_frame_observer_natives.cc
diff --git a/extensions/renderer/render_frame_observer_natives.cc b/extensions/renderer/render_frame_observer_natives.cc
index c3a147ddd4b657ec3e31f4cb9807918185a54084..1c1f06e87f3217890d3519e426f51be50ebc4542 100644
--- a/extensions/renderer/render_frame_observer_natives.cc
+++ b/extensions/renderer/render_frame_observer_natives.cc
@@ -19,43 +19,29 @@ namespace {
 // Deletes itself when done.
 class LoadWatcher : public content::RenderFrameObserver {
  public:
-  LoadWatcher(ScriptContext* context,
-              content::RenderFrame* frame,
-              v8::Local<v8::Function> cb)
-      : content::RenderFrameObserver(frame),
-        context_(context),
-        callback_(context->isolate(), cb) {
-    if (ExtensionFrameHelper::Get(frame)->
-            did_create_current_document_element()) {
-      // If the document element is already created, then we can call the
-      // callback immediately (though post it to the message loop so as to not
-      // call it re-entrantly).
-      // The Unretained is safe because this class manages its own lifetime.
-      base::MessageLoop::current()->PostTask(
-          FROM_HERE,
-          base::Bind(&LoadWatcher::CallbackAndDie, base::Unretained(this),
-                     true));
-    }
+  LoadWatcher(content::RenderFrame* frame,
+              const base::Callback<void(bool)>& callback)
+      : content::RenderFrameObserver(frame), callback_(callback) {}
+
+  void DidCreateDocumentElement() override {
+    // The callback must be run as soon as the root element is available.
+    // Running the callback may trigger DidCreateDocumentElement or
+    // DidFailProvisionalLoad, so delete this before running the callback.
+    base::Callback<void(bool)> callback = callback_;
+    delete this;
+    callback.Run(true);
   }
 
-  void DidCreateDocumentElement() override { CallbackAndDie(true); }
-
   void DidFailProvisionalLoad(const blink::WebURLError& error) override {
-    CallbackAndDie(false);
-  }
-
- private:
-  void CallbackAndDie(bool succeeded) {
-    v8::Isolate* isolate = context_->isolate();
-    v8::HandleScope handle_scope(isolate);
-    v8::Local<v8::Value> args[] = {v8::Boolean::New(isolate, succeeded)};
-    context_->CallFunction(v8::Local<v8::Function>::New(isolate, callback_),
-                           arraysize(args), args);
+    // Use PostTask to avoid running user scripts while handling this
+    // DidFailProvisionalLoad notification.
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+                                           base::Bind(callback_, false));
     delete this;
   }
 
-  ScriptContext* context_;
-  v8::Global<v8::Function> callback_;
+ private:
+  base::Callback<void(bool)> callback_;
 
   DISALLOW_COPY_AND_ASSIGN(LoadWatcher);
 };
@@ -63,13 +49,20 @@ class LoadWatcher : public content::RenderFrameObserver {
 }  // namespace
 
 RenderFrameObserverNatives::RenderFrameObserverNatives(ScriptContext* context)
-    : ObjectBackedNativeHandler(context) {
+    : ObjectBackedNativeHandler(context), weak_ptr_factory_(this) {
   RouteFunction(
       "OnDocumentElementCreated",
       base::Bind(&RenderFrameObserverNatives::OnDocumentElementCreated,
                  base::Unretained(this)));
 }
 
+RenderFrameObserverNatives::~RenderFrameObserverNatives() {}
+
+void RenderFrameObserverNatives::Invalidate() {
+  weak_ptr_factory_.InvalidateWeakPtrs();
+  ObjectBackedNativeHandler::Invalidate();
+}
+
 void RenderFrameObserverNatives::OnDocumentElementCreated(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   CHECK(args.Length() == 2);
@@ -84,9 +77,32 @@ void RenderFrameObserverNatives::OnDocumentElementCreated(
     return;
   }
 
-  new LoadWatcher(context(), frame, args[1].As<v8::Function>());
+  v8::Global<v8::Function> v8_callback(context()->isolate(),

[Devlin, 2016/02/10 20:30:47]

Something else I just thought of:
v8::Globals are refcounted - does the fact that this is passed to a callback
that can outlive the RenderFrameObserverNatives mean that we could be
artificially prolonging the life of a context or isolate, since those are passed
to the function?

[robwu, 2016/02/10 20:45:49]

I don't know, I'm a complete noob with V8 (I just learned about its API
yesterday, to fix this bug).

The lifespan of a RenderFrame and its RenderFrameObserverNative (owned by the
ModuleSystem) are quite similar, so it shouldn't matter in practice.

But if you come across a V8 guru who knows the answer, please let me know. I'm
curious as well :)

[Devlin, 2016/02/10 20:59:00]

Let's go with this for now - it's certainly better than crashing, and I'm sure
it's not the only place with a possible-maybe-leak. But cc'd jochen@ (resident
v8 guru) to educate us when's he's back in.

In the meantime,
https://developers.google.com/v8/embed#handles-and-garbage-collection offers
some good information, but doesn't really go into much detail in many areas.

+                                       args[1].As<v8::Function>());
+  base::Callback<void(bool)> callback(
+      base::Bind(&RenderFrameObserverNatives::InvokeCallback,
+                 weak_ptr_factory_.GetWeakPtr(), base::Passed(&v8_callback)));
+  if (ExtensionFrameHelper::Get(frame)->did_create_current_document_element()) {
+    // If the document element is already created, then we can call the callback
+    // immediately (though use PostTask to ensure that the callback is called
+    // asynchronously).
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+                                           base::Bind(callback, true));
+  } else {
+    new LoadWatcher(frame, callback);
+  }
 
   args.GetReturnValue().Set(true);
 }
 
+void RenderFrameObserverNatives::InvokeCallback(
+    v8::Global<v8::Function> callback,
+    bool succeeded) {
+  v8::Isolate* isolate = context()->isolate();
+  v8::HandleScope handle_scope(isolate);
+  v8::Local<v8::Value> args[] = {v8::Boolean::New(isolate, succeeded)};
+  context()->CallFunction(v8::Local<v8::Function>::New(isolate, callback),
+                          arraysize(args), args);
+}
+
 }  // namespace extensions