Deal with frame removal by content scripts

extensions/renderer/extension_frame_helper.cc

Index: extensions/renderer/extension_frame_helper.cc
diff --git a/extensions/renderer/extension_frame_helper.cc b/extensions/renderer/extension_frame_helper.cc
index c063f84a169ba2a5d6915b6cdc121647f9b93e2c..d1bfa25e9f2f804e75ea41782a7c0b8a3d538862 100644
--- a/extensions/renderer/extension_frame_helper.cc
+++ b/extensions/renderer/extension_frame_helper.cc
@@ -58,6 +58,22 @@ bool RenderFrameMatches(const ExtensionFrameHelper* frame_helper,
   return true;
 }
 
+// Runs every callback in |callbacks_to_be_run_and_cleared| while |frame_helper|
+// is valid, and clears |callbacks_to_be_run_and_cleared|.
+void RunCallbacksWhileFrameIsValid(
+    base::WeakPtr<ExtensionFrameHelper> frame_helper,
+    std::vector<base::Closure>* callbacks_to_be_run_and_cleared) {
+  // The JavaScript code can cause re-entrancy. To avoid a deadlock, don't run
+  // callbacks that are added during the iteration.
+  std::vector<base::Closure> callbacks;
+  callbacks_to_be_run_and_cleared->swap(callbacks);
+  for (auto& callback : callbacks) {
+    callback.Run();
+    if (!frame_helper.get())
+      return;  // Frame and ExtensionFrameHelper invalidated by callback.
+  }
+}
+
 }  // namespace
 
 ExtensionFrameHelper::ExtensionFrameHelper(content::RenderFrame* render_frame,
@@ -68,7 +84,8 @@ ExtensionFrameHelper::ExtensionFrameHelper(content::RenderFrame* render_frame,
       tab_id_(-1),
       browser_window_id_(-1),
       extension_dispatcher_(extension_dispatcher),
-      did_create_current_document_element_(false) {
+      did_create_current_document_element_(false),
+      weak_ptr_factory_(this) {
   g_frame_helpers.Get().insert(this);
 }
 
@@ -123,6 +140,29 @@ void ExtensionFrameHelper::DidCreateNewDocument() {
   did_create_current_document_element_ = false;
 }
 
+void ExtensionFrameHelper::AfterDidCreateDocumentElement() {
+  DCHECK(did_create_current_document_element_);
+  RunCallbacksWhileFrameIsValid(weak_ptr_factory_.GetWeakPtr(),
+                                &document_element_created_callbacks_);
+  // |this| might be dead by now.
+}
+
+void ExtensionFrameHelper::AfterDidFinishDocumentLoad() {
+  RunCallbacksWhileFrameIsValid(weak_ptr_factory_.GetWeakPtr(),
+                                &document_load_finished_callbacks_);
+  // |this| might be dead by now.
+}
+
+void ExtensionFrameHelper::ScheduleAfterDidCreateDocumentElement(
+    const base::Closure& callback) {
+  document_element_created_callbacks_.push_back(callback);

[Devlin, 2016/02/16 18:37:37]

Can we add a pair of bools (did finish doc load, did create doc element) or an
enum to represent the state this frame is in so that we can DCHECK here that
people aren't adding callbacks that will never be called (or will be called on a
new load)?

[robwu, 2016/02/22 00:15:02]

No. The re-entrancy via JavaScript and Blink makes it not possible to assert
things like DCHECK(!did_create_current_document_element_). It might be possible
for the load event, but that is not guaranteed by any contract in Blink.

Because running the scripts now happens in a separate notification, we could
change the implementation to only run the script once per document, but that is
not required for fixing this bug (and a huge undertaking in itself), so I'm not
going to block this CL on that.
Until there exist a "frame is unloaded" notification from Blink, I cannot really
determine whether this holds. ScriptInjectionManager::RFOHelper attempts to
answer this, but that is in very specific to content scripts and not suitable
for general use.

ScheduleRunAtDocumentStart should be called in a DidCreateDocumentElement
notification (this is clearly documented in the header), and I think that it's
sufficient for now.

[dcheng, 2016/02/22 00:28:34]

FWIW this already exists in the form of WebFrameClient::frame detached

[dcheng, 2016/02/22 00:28:34]

FWIW this already exists in the form of WebFrameClient::frameDetached

[robwu, 2016/02/22 00:52:31]

Is that also called upon navigation? I thought that it's only for frame removal
and process swaps. By "frame unload", I'm referring to the event where the
document and script context of a frame become invalid.

+}
+
+void ExtensionFrameHelper::ScheduleAfterDidFinishDocumentLoad(
+    const base::Closure& callback) {
+  document_load_finished_callbacks_.push_back(callback);
+}
+
 void ExtensionFrameHelper::DidMatchCSS(
     const blink::WebVector<blink::WebString>& newly_matching_selectors,
     const blink::WebVector<blink::WebString>& stopped_matching_selectors) {