Deal with frame removal by content scripts

extensions/renderer/dispatcher.cc

 // Copyright 2014 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/dispatcher.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 482 matching lines @@
   }
 
   // In testing, the document lifetime events can happen after the render
   // process shutdown event.
   // See: http://crbug.com/21508 and http://crbug.com/500851
   if (content_watcher_) {
     content_watcher_->DidCreateDocumentElement(frame);
   }
 }
 
+void Dispatcher::AfterDidCreateDocumentElement(
+    content::RenderFrame* render_frame) {
+  ExtensionFrameHelper* frame_helper = ExtensionFrameHelper::Get(render_frame);
+  if (!frame_helper)
+    return;  // The frame is invisible to extensions.
+
+  base::WeakPtr<ExtensionFrameHelper> frame_helper_weak =
+      frame_helper->AsWeakPtr();
+
+  script_injection_manager_->AfterDidCreateDocumentElement(render_frame);

[Devlin, 2016/02/12 22:21:56]

Would it make sense to have the script injection manager register a callback
with the frame helper?  Then, there's one canonical way to do work here (and we
don't have *quite* as many lifetime checks)?

Also, would that allow us to contain ExtensionFrameHelper's weakness entirely
within itself?  (We generally prefer to just have an internal weak ptr over
SupportsWeakPtr - there are a few threads on chromium-dev that explain it pretty
well.)

[robwu, 2016/02/12 22:37:13]

I avoided that, because ScriptInjectionManager needs to be notified of every
frame, so I optimized for minimal overhead.
The callbacks in ExtensionFrameHelper are only used to support
chrome.app.window.create, which rarely happens.
If we follow your suggestion, then yes. An alternative to what we currently
have,
ContentRendererClient -> Dispatcher -> ExtensionFrameHost
is
ContentRendererClient -> ExtensionFrameHost -> Dispatcher

but then the following pattern would be repeated 4x (for the two AfterDidXXX
notifications, for shell and chrome clients).

ExtensionFrameHelper* frame_helper = ExtensionFrameHelper::Get(render_frame);
if (frame_helper)
  frame_helper->AfterDidXXX(render_frame);
  // AfterDidXXX might have invalidated |render_frame| and |this|.

WDYT? Should we trade self-contained WeakPtrFactory for a bit of duplication?

[Devlin, 2016/02/12 22:51:01]

What's the optimization there?  Just avoiding creating a callback and running
it?  If that's all, I'd say let's do it anyway - base::Callback is designed to
be fast and lightweight.
Nah, going into dispatcher is good.  It's what everything else does.  I'd prefer
to not expose too much more to the content client.

[robwu, 2016/02/12 23:24:49]

This happens (which seems not that expensive):
1. Map RenderFrame to ExtensionFrameHelper
2. Create callback
3. Insert the callback in a vector.
4. Later: Run callback.

But if I go down this route, then for consistency I'd also have to change
DidFinishDocumentLoad to be callback-based. Otherwise there are two things to do
comparable things in the ScriptInjectionManager.

[Devlin, 2016/02/12 23:42:24]

That sounds reasonable to me.  If we're going to start using
ExtensionFrameHelper as an indication of frame liveness, let's commit to it! :)

(As a result, we *might* be able to clean up a few of the other places in
ScriptInjectionManager, but let's hold off on those changes.)

+  if (!frame_helper_weak.get())
+    return;  // Frame was destroyed by injected user script.
+
+  frame_helper->AfterDidCreateDocumentElement();
+}
+
+void Dispatcher::AfterDidFinishDocumentLoad(
+    content::RenderFrame* render_frame) {
+  if (!ExtensionFrameHelper::Get(render_frame))
+    return;  // The frame is invisible to extensions.
+
+  script_injection_manager_->AfterDidFinishDocumentLoad(render_frame);
+  // The frame might be destroyed by the injected user script at this point.
+}
+
 void Dispatcher::OnExtensionResponse(int request_id,
                                      bool success,
                                      const base::ListValue& response,
                                      const std::string& error) {
   request_sender_->HandleResponse(request_id, success, response, error);
 }
 
 void Dispatcher::DispatchEvent(const std::string& extension_id,
                                const std::string& event_name) const {
   base::ListValue args;
@@ skipping 1099 matching lines @@
   // The "guestViewDeny" module must always be loaded last. It registers
   // error-providing custom elements for the GuestView types that are not
   // available, and thus all of those types must have been checked and loaded
   // (or not loaded) beforehand.
   if (context_type == Feature::BLESSED_EXTENSION_CONTEXT) {
     module_system->Require("guestViewDeny");
   }
 }
 
 }  // namespace extensions