[DevTools] Avoid current_ and pending_ being the same host in RenderFrameDevToolsAgentHost.

content/browser/devtools/render_frame_devtools_agent_host.cc

 // Copyright (c) 2012 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/browser/devtools/render_frame_devtools_agent_host.h"
 
 #include <tuple>
 #include <utility>
 
 #include "base/guid.h"
@@ skipping 599 matching lines @@
 
 void RenderFrameDevToolsAgentHost::AboutToNavigateRenderFrame(
     RenderFrameHost* old_host,
     RenderFrameHost* new_host) {
   if (IsBrowserSideNavigationEnabled())
     return;
 
   DCHECK(!pending_ || pending_->host() != old_host);
   if (!current_ || current_->host() != old_host)
     return;
   if (old_host == new_host && !current_frame_crashed_)

[Charlie Reis, 2016/09/23 18:26:16]

Interesting.  I guess current_frame_crashed_ isn't true in this case, despite
the sad tab?  At first glance, I would have expected us to return early here.

[dgozman, 2016/09/24 01:44:04]

The logic here is: if we didn't crash there is no need to reattach and
reinitialize the RenderFrameImpl. Since we crashed in this case, we skip early
return.

     return;
   DCHECK(!pending_);
   SetPending(static_cast<RenderFrameHostImpl*>(new_host));
+  // Commit when navigating the same frame after crash, avoiding the same
+  // host in current_ and pending_.
+  if (old_host == new_host)
+    CommitPending();
 }
 
 void RenderFrameDevToolsAgentHost::AboutToNavigate(
     NavigationHandle* navigation_handle) {
   if (!IsBrowserSideNavigationEnabled())
     return;
   DCHECK(current_);
   navigating_handles_.insert(navigation_handle);
 }
 
 void RenderFrameDevToolsAgentHost::RenderFrameHostChanged(
     RenderFrameHost* old_host,
     RenderFrameHost* new_host) {
   if (IsBrowserSideNavigationEnabled())
     return;
 
   DCHECK(!pending_ || pending_->host() != old_host);

[Charlie Reis, 2016/09/23 18:26:16]

Maybe we can also upgrade this from a DCHECK to a CHECK?  That would have
prevented the use-after-free and made this a simple crash rather than a security
bug.

[dgozman, 2016/09/24 01:44:04]

I can land a separate patch which upgrades to CHECK, so that we separate a
bug-fix from something which may potentially break in the wild.

[Charlie Reis, 2016/09/26 16:49:02]

Sounds good, thanks.

   if (!current_ || current_->host() != old_host)
     return;
 
   // AboutToNavigateRenderFrame was not called for renderer-initiated
   // navigation.
   if (!pending_)
     SetPending(static_cast<RenderFrameHostImpl*>(new_host));
 
   CommitPending();
 }
@@ skipping 375 matching lines @@
     RenderFrameHost* host) {
   return (current_ && current_->host() == host) ||
       (pending_ && pending_->host() == host);
 }
 
 bool RenderFrameDevToolsAgentHost::IsChildFrame() {
   return current_ && current_->host()->GetParent();
 }
 
 }  // namespace content