[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;
+  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),
+        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) {
   g_frame_helpers.Get().insert(this);
 }
 
 ExtensionFrameHelper::~ExtensionFrameHelper() {
   g_frame_helpers.Get().erase(this);
@@ skipping 39 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(
+        new PendingContext(context, extension_group, world_id));
+  }
+}
+
+void ExtensionFrameHelper::DidClearWindowObject() {
+  did_clear_window_ = true;
+  for (PendingContext* context : pending_contexts_) {

[Devlin, 2015/11/12 01:12:03]

This would be a lot cleaner if we could have a
WebFrame::getCreatedScriptContexts method - but there isn't one.  Any feelings
on adding one?

[dcheng, 2015/11/13 00:26:33]

I don't think we should add one. At the end of the day, we need this patch to
work around bad callback ordering that no one really understands how it happens.
The right long-term fix is to understand what's going on and fix the callbacks.

Since you're investigating this, any chance you can capture some stack traces
for me? I'd be interested in comparing the stack trace for when
DidCreateScriptContext is called after DidClearWindowObject vs before.

[Devlin, 2015/11/13 19:50:05]

Stack traces: https://paste.googleplex.com/6093038275264512

Grabbed a few sample traces of DidCreateScriptContext before vs after
DidClearWindowObject, as well as the stack traces of two script contexts for the
frame that would otherwise be incorrectly categorized (listed here as "possible
culprit").  Note that I didn't do any de-duping, so multiple stack traces could
be the same.

+    v8::HandleScope scope(context->isolate);
+    v8::Context::Scope context_scope(
+        v8::Local<v8::Context>::New(context->isolate, context->context));
+    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();

[dcheng, 2015/11/13 00:26:33]

Maybe pending_context_ should be a map of v8::Global<v8::Context>'s to the other
info?

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

I don't know if that would work very well... using an object like a v8 handle as
a key seems dangerous.  And since this is the only place we look up by context,
I think it's probably best as a vector.

+       ++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