Track down crash during RenderFrameImpl::didCommitProvisionalLoad.

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 1479 matching lines @@
     render_view_->suppress_dialogs_until_swap_out_ = false;
 
   Send(new FrameHostMsg_SwapOut_ACK(routing_id_));
 
   RenderViewImpl* render_view = render_view_.get();
   bool is_main_frame = is_main_frame_;
 
   // Now that all of the cleanup is complete and the browser side is notified,
   // start using the RenderFrameProxy, if one is created.
   if (proxy && swapped_out_forbidden) {
+    // The swap call deletes this RenderFrame via frameDetached.  Do not access
+    // any members after this call.
+    // TODO(creis): WebFrame::swap() can return false.  Most of those cases
+    // should be due to the frame being detached during unload (in which case
+    // the necessary cleanup has happened anyway), but it might be possible for
+    // it to return false without detaching.  Catch those cases below to track
+    // down https://crbug.com/575245.
     frame_->swap(proxy->web_frame());

[Charlie Reis, 2016/01/13 23:30:49]

dcheng: I'm not sure what we should do differently here if swap returns false. 
In some of those cases, the frame will have been detached (via JS), and in
others it might not (e.g., m_provisionalDocumentLoader check in
FrameLoader::prepareForCommit).

Can swap have an invariant that the frame is always detached afterward?  (It
might be able to make the distinction between the cases mentioned above.)  

Do we need the return value, given that no one is listening to it?

[dcheng, 2016/01/14 00:15:25]

We need to figure out the behavior of sync navigations in unload events first.
From my really simple testing, it appears that Chrome is the only browser (I
haven't tested Edge) that allows a fragment navigation in an unload event to
take effect. japhet@ points out that we should probably test pushState /
replaceState / about:blank as well.

This is an edge case I'm perfectly happy not to support, but we should probably
get some language in the spec about this (I don't see anything about this right
now). It's also sort of a web-facing change, even if it's just Chrome.
Well, if we wanted to preserve current Chrome behavior, running code after the
failed swap could be incorrect: for example, mandoline updated some WebFrame
pointers after calling swap(). These updates are incorrect if the swap() is
cancelled.

 
+    // For main frames, the swap should have cleared the RenderView's pointer to
+    // this frame.
+    if (is_main_frame)
+      CHECK(!render_view->main_render_frame_);
+
     if (is_loading)
       proxy->OnDidStartLoading();
   }
 
   // In --site-per-process, initialize the WebRemoteFrame with the replication
   // state passed by the process that is now rendering the frame.
   // TODO(alexmos): We cannot yet do this for swapped-out main frames, because
   // in that case we leave the LocalFrame as the main frame visible to Blink
   // and don't call swap() above. Because swap() is what creates a RemoteFrame
   // in proxy->web_frame(), the RemoteFrame will not exist for main frames.
@@ skipping 4206 matching lines @@
       media::ConvertToSwitchOutputDeviceCB(web_callbacks);
   scoped_refptr<media::AudioOutputDevice> device =
       AudioDeviceFactory::NewOutputDevice(routing_id_, 0, sink_id.utf8(),
                                           security_origin);
   media::OutputDeviceStatus status = device->GetDeviceStatus();
   device->Stop();
   callback.Run(status);
 }
 
 }  // namespace content