Deal with frame removal by content scripts

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 1d26e5940b95c7e048e0c00402c46f46e2753403..f7257fa4e2906ba8290183a698570e25bf0d7e7c 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -3350,8 +3350,17 @@ void RenderFrameImpl::didCreateDocumentElement(blink::WebLocalFrame* frame) {
     }
   }
 
-  FOR_EACH_OBSERVER(RenderFrameObserver, observers_,
-                    DidCreateDocumentElement());
+  // Notify observers; The frame can be detached by an extension script.
+  if (observers_.might_have_observers()) {

[nasko, 2016/02/08 18:12:53]

Yuck :(. Why is this the only method that needs this functionality? Shouldn't
most observer interfaces that notify extensions be fixed in the same way?

[robwu, 2016/02/09 01:18:10]

This is needed because the observers expect the subject (RenderFrame) to be
valid, so that e.g. RenderFrameObserver::render_frame() returns a valid pointer.
But because content script execute synchronously, this assumption does not hold,
so we have to tread carefully, and check whether the RenderFrame is still valid.

I checked every file in extensions/renderer that includes
render_frame_observer.h, and found a few occurrences:
- extensions_render_frame_observer.h - safe (no untrusted calls)
- extension_frame_helper.h - safe (no untrusted calls in observer notifications)
- render_frame_observer_natives.cc - DidFailProvisionalLoad should be addressed
(that is for another CL, because it is unrelated to this specific issue and I
found another bug - crbug.com/585268).
- script_injection_manager.cc - all fixed by this CL.
- app_window_custom_bindings.cc - fixed by this CL.

+    base::WeakPtr<RenderFrameImpl> self = weak_factory_.GetWeakPtr();
+    base::ObserverListBase<RenderFrameObserver>::Iterator it(&observers_);
+    RenderFrameObserver* obs;
+    while ((obs = it.GetNext()) != nullptr) {
+      obs->DidCreateDocumentElement();
+      if (!self.get())
+        return;
+    }
+  }
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers(),
                     DidCreateDocumentElement(frame));
 }
@@ -3395,7 +3404,17 @@ void RenderFrameImpl::didFinishDocumentLoad(blink::WebLocalFrame* frame,
 
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers(),
                     DidFinishDocumentLoad(frame));
-  FOR_EACH_OBSERVER(RenderFrameObserver, observers_, DidFinishDocumentLoad());
+  // Notify observers; The frame can be detached by an extension script.
+  if (observers_.might_have_observers()) {
+    base::WeakPtr<RenderFrameImpl> self = weak_factory_.GetWeakPtr();
+    base::ObserverListBase<RenderFrameObserver>::Iterator it(&observers_);
+    RenderFrameObserver* obs;
+    while ((obs = it.GetNext()) != nullptr) {
+      obs->DidFinishDocumentLoad();
+      if (!self.get())
+        return;
+    }
+  }
 
   // Check whether we have new encoding name.
   UpdateEncoding(frame, frame->view()->pageEncoding().utf8());