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

extensions/renderer/extension_frame_helper.cc

 // Copyright 2013 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 "extensions/renderer/extension_frame_helper.h"
 
 #include "content/public/renderer/render_frame.h"
 #include "extensions/common/api/messaging/message.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension_messages.h"
@@ skipping 29 matching lines @@
   if (url.host() != match_extension_id)
     return false;
   if (match_window_id != extension_misc::kUnknownWindowId &&
       frame_helper->browser_window_id() != match_window_id)
     return false;
   return true;
 }
 
 }  // 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);
 }
 
 ExtensionFrameHelper::~ExtensionFrameHelper() {
   g_frame_helpers.Get().erase(this);
 }
 
 // static
 std::vector<content::RenderFrame*> ExtensionFrameHelper::GetExtensionFrames(
     const std::string& extension_id,
@@ skipping 34 matching lines @@
 void ExtensionFrameHelper::DidCreateDocumentElement() {
   did_create_current_document_element_ = true;
   extension_dispatcher_->DidCreateDocumentElement(
       render_frame()->GetWebFrame());
 }
 
 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) {
   extension_dispatcher_->content_watcher()->DidMatchCSS(
       render_frame()->GetWebFrame(), newly_matching_selectors,
       stopped_matching_selectors);
 }
 
 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) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ExtensionFrameHelper, message)
     IPC_MESSAGE_HANDLER(ExtensionMsg_DispatchOnConnect,
                         OnExtensionDispatchOnConnect)
     IPC_MESSAGE_HANDLER(ExtensionMsg_DeliverMessage, OnExtensionDeliverMessage)
     IPC_MESSAGE_HANDLER(ExtensionMsg_DispatchOnDisconnect,
                         OnExtensionDispatchOnDisconnect)
@@ skipping 71 matching lines @@
     const std::string& module_name,
     const std::string& function_name,
     const base::ListValue& args,
     bool user_gesture) {
   extension_dispatcher_->InvokeModuleSystemMethod(render_frame(), extension_id,
                                                   module_name, function_name,
                                                   args, user_gesture);
 }
 
 }  // namespace extensions