[Extensions] Wait until the Window object is cleared before classifying contexts

extensions/renderer/extension_frame_helper.cc

Index: extensions/renderer/extension_frame_helper.cc
diff --git a/extensions/renderer/extension_frame_helper.cc b/extensions/renderer/extension_frame_helper.cc
index fe581c331fd845868386be85175a7fe91f97244b..fe9af69b3221ade6e3cbb356ce03d005a7c283c5 100644
--- a/extensions/renderer/extension_frame_helper.cc
+++ b/extensions/renderer/extension_frame_helper.cc
@@ -47,15 +47,30 @@ bool RenderFrameMatches(const ExtensionFrameHelper* frame_helper,
 
 }  // namespace
 
+struct ExtensionFrameHelper::PendingContext {
+  v8::Global<v8::Context> context;

[Devlin, 2015/11/16 22:46:45]

(responding to the question of global vs local vs foo)

From the guide Jochen provided (thanks, Jochen! That actually cleared a lot up
:)) [1], it seems like a Global is the proper thing to use here.  In the guide,
this is called a UniquePersistent, and handles lifetime without needing the
HandleScope object.  And, in v8.h, UniquePersistent is an alias for Global - so
I *think* Global is the more up-to-date terminology (please correct me if I'm
wrong, Jochen).

[1] https://developers.google.com/v8/embed#handles-and-garbage-collection

+  int extension_group;
+  int world_id;
+  v8::Isolate* isolate;
+
+  PendingContext(v8::Local<v8::Context> context,
+                 int extension_group,
+                 int world_id)
+      : context(context->GetIsolate(), context),

[dcheng, 2015/11/18 00:57:22]

I'd suggest just capturing context here (you can always get the isolate from it
again later)

[Devlin, 2015/11/18 18:02:11]

Except that we store the context as a global - so in order to access it, we need
an isolate.  (Globals aren't like Locals where you can just use -> and have it
work.)  So we'd need an isolate in order to get a context that we could use to
call GetIsolate().

Unless I'm missing something - which, with v8 apis, is entirely possible.

[dcheng, 2015/11/18 18:19:53]

Right, makes sense. v8 is magic.

+        extension_group(extension_group),
+        world_id(world_id),
+        isolate(context->GetIsolate()) {}
+};
+
 ExtensionFrameHelper::ExtensionFrameHelper(content::RenderFrame* render_frame,
                                            Dispatcher* extension_dispatcher)
     : content::RenderFrameObserver(render_frame),
       content::RenderFrameObserverTracker<ExtensionFrameHelper>(render_frame),
+      did_clear_window_(false),
       view_type_(VIEW_TYPE_INVALID),
       tab_id_(-1),
       browser_window_id_(-1),
-      extension_dispatcher_(extension_dispatcher),
-      did_create_current_document_element_(false) {
+      extension_dispatcher_(extension_dispatcher) {
   g_frame_helpers.Get().insert(this);
 }
 
@@ -110,6 +125,10 @@ void ExtensionFrameHelper::DidCreateNewDocument() {
   did_create_current_document_element_ = false;
 }
 
+void ExtensionFrameHelper::DidStartProvisionalLoad() {
+  did_clear_window_ = false;
+}
+
 void ExtensionFrameHelper::DidMatchCSS(
     const blink::WebVector<blink::WebString>& newly_matching_selectors,
     const blink::WebVector<blink::WebString>& stopped_matching_selectors) {
@@ -122,15 +141,41 @@ void ExtensionFrameHelper::DidCreateScriptContext(
     v8::Local<v8::Context> context,
     int extension_group,
     int world_id) {
-  extension_dispatcher_->DidCreateScriptContext(
-      render_frame()->GetWebFrame(), context, extension_group, world_id);
+  if (did_clear_window_) {
+    extension_dispatcher_->DidCreateScriptContext(
+        render_frame()->GetWebFrame(), context, extension_group, world_id);
+  } else {
+    pending_contexts_.push_back(make_scoped_ptr(
+        new PendingContext(context, extension_group, world_id)));
+  }
+}
+
+void ExtensionFrameHelper::DidClearWindowObject() {
+  did_clear_window_ = true;
+  for (const auto& context : pending_contexts_) {

[dcheng, 2015/11/18 18:19:53]

Nit: s/context/pending_context/? To avoid the name collision between with the v8
context below (or I guess you could just call it v8_context).

[Devlin, 2015/11/18 18:36:28]

Done.

+    v8::HandleScope scope(context->isolate);

[dcheng, 2015/11/18 18:19:53]

Nit: v8::Local<v8::Context> context(context->context.Get(context->isolate));

and then just use context on line 158 and line 161.

[Devlin, 2015/11/18 18:36:28]

heh - just what we needed - another context variable! ;)

Done.

+    v8::Context::Scope context_scope(
+        v8::Local<v8::Context>::New(context->isolate, context->context));

[dcheng, 2015/11/18 00:57:22]

I'm assuming this is required because DidCreateScriptContext() ends up executing
script?

[Devlin, 2015/11/18 18:02:11]

Right - we set up the extension bindings, which involves at least instantiating
new objects, and I think also running a bit of (trusted) JS.

+    extension_dispatcher_->DidCreateScriptContext(
+        render_frame()->GetWebFrame(),
+        v8::Local<v8::Context>::New(context->isolate, context->context),
+        context->extension_group, context->world_id);
+  }
+  pending_contexts_.clear();
 }
 
 void ExtensionFrameHelper::WillReleaseScriptContext(
-    v8::Local<v8::Context> context,
+    v8::Local<v8::Context> v8_context,
     int world_id) {
-  extension_dispatcher_->WillReleaseScriptContext(
-      render_frame()->GetWebFrame(), context, world_id);
+  for (auto iter = pending_contexts_.begin(); iter != pending_contexts_.end();
+       ++iter) {
+    if ((*iter)->context == v8_context) {
+      pending_contexts_.erase(iter);
+      return;
+    }
+  }
+  extension_dispatcher_->WillReleaseScriptContext(render_frame()->GetWebFrame(),
+                                                  v8_context, world_id);
 }
 
 bool ExtensionFrameHelper::OnMessageReceived(const IPC::Message& message) {