Deal with frame removal by content scripts

extensions/renderer/dispatcher.cc

Index: extensions/renderer/dispatcher.cc
diff --git a/extensions/renderer/dispatcher.cc b/extensions/renderer/dispatcher.cc
index a8a2b3ed3076b3dc1d3405b6025a9c2025ce59cc..8a7cc96a0a2c642162db8a72ec8a856e532669d8 100644
--- a/extensions/renderer/dispatcher.cc
+++ b/extensions/renderer/dispatcher.cc
@@ -500,6 +500,31 @@ void Dispatcher::DidCreateDocumentElement(blink::WebLocalFrame* 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,