Deal with frame removal by content scripts

content/renderer/render_frame_impl.cc

 // Copyright 2013 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 "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 3332 matching lines @@
     if (frame == main_frame) {
       // For now, don't remember plugin zoom values.  We don't want to mix them
       // with normal web content (i.e. a fixed layout plugin would usually want
       // them different).
       render_view_->Send(new ViewHostMsg_DocumentAvailableInMainFrame(
           render_view_->GetRoutingID(),
           main_frame->document().isPluginDocument()));
     }
   }
 
-  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));
 }
 
 void RenderFrameImpl::didReceiveTitle(blink::WebLocalFrame* frame,
                                       const blink::WebString& title,
                                       blink::WebTextDirection direction) {
   DCHECK(!frame_ || frame_ == frame);
   // Ignore all but top level navigations.
   if (!frame->parent()) {
@@ skipping 23 matching lines @@
                "id", routing_id_);
   DCHECK(!frame_ || frame_ == frame);
   WebDataSource* ds = frame->dataSource();
   DocumentState* document_state = DocumentState::FromDataSource(ds);
   document_state->set_finish_document_load_time(Time::Now());
 
   Send(new FrameHostMsg_DidFinishDocumentLoad(routing_id_));
 
   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());
 
   // If this is an empty document with an http status code indicating an error,
   // we may want to display our own error page, so the user doesn't end up
   // with an unexplained blank page.
   if (!document_is_empty)
     return;
 
@@ skipping 2699 matching lines @@
                                     int match_count,
                                     int ordinal,
                                     const WebRect& selection_rect,
                                     bool final_status_update) {
   Send(new FrameHostMsg_Find_Reply(routing_id_, request_id, match_count,
                                    selection_rect, ordinal,
                                    final_status_update));
 }
 
 }  // namespace content