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..7f3e1e63c69d170f80f092b01246e03c88adcba5 100644
--- a/extensions/renderer/dispatcher.cc
+++ b/extensions/renderer/dispatcher.cc
@@ -500,6 +500,30 @@ void Dispatcher::DidCreateDocumentElement(blink::WebLocalFrame* frame) {
   }
 }
 
+void Dispatcher::AfterDidCreateDocumentElement(
+    const base::WeakPtr<content::RenderFrame>& render_frame) {
+  DCHECK(render_frame.get());
+  ExtensionFrameHelper* frame_helper =
+      ExtensionFrameHelper::Get(render_frame.get());

[ncarter (slow), 2016/02/11 22:47:32]

Nasko pointed me at this CL to see if there might be an alternative to minting
the WeakPtr on the content side and passing it through here. 

One idea would be to add a WeakPtrFactory<> to the ExtensionFrameHelper (whose
lifetime, I think, is bound to the renderframe). This would let the whole
WeakPtr dance be contained entirely inside the //extensions layer.

[robwu, 2016/02/11 23:36:50]

Done. I don't see any issue with passing a weakptr down from content/ (except
that the creation of the weak ptr is unnecessary if extensions are disabled),
but it's your call.

[ncarter (slow), 2016/02/12 19:32:57]

I'm not opposed to it per se either, but I think it's cleaner if we handle the
deletion-detection at the same layer that the deletion-triggering happens.

My reasoning is that passing a weakptr as an in-param fairly uncommon idiom
outside of ctors and functions used as callbacks (where time might pass between
the bind and the invocation). Uncommon idioms result in code that's harder to
understand: in the case of the content client calls, when I first saw them, I
wondered "Why would content need to pass a WeakPtr here? Is it possible the
frame has already been deleted? If so why would content be reporting an event on
the deleted frame?"

If RenderFrameHost had already exposed a public GetWeakPtr<> method I would have
had no trouble with you using that. But it wasn't there already, and since there
is a way to get the behavior you need without adding a method to
RenderFrameHost, that's what I suggested.

[robwu, 2016/02/12 19:51:43]

That makes sense. Thanks for explaining your line of reasoning and review!

+  if (!frame_helper)
+    return;  // The frame is invisible to extensions.
+
+  script_injection_manager_->AfterDidCreateDocumentElement(render_frame.get());
+  if (!render_frame.get())
+    return;  // Frame was destroyed by injected user script.
+
+  frame_helper->AfterDidCreateDocumentElement(render_frame);
+}
+
+void Dispatcher::AfterDidFinishDocumentLoad(
+    const base::WeakPtr<content::RenderFrame>& render_frame) {
+  if (!ExtensionFrameHelper::Get(render_frame.get()))
+    return;  // The frame is invisible to extensions.
+
+  script_injection_manager_->AfterDidFinishDocumentLoad(render_frame.get());
+  // 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,