Do not reset navigation state when BeforeUnload is cancelled by a commit

content/browser/web_contents/web_contents_impl.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/web_contents/web_contents_impl.h"
 
 #include <stddef.h>
 
 #include <cmath>
 #include <utility>
@@ skipping 4672 matching lines @@
 void WebContentsImpl::OnDialogClosed(int render_process_id,
                                      int render_frame_id,
                                      IPC::Message* reply_msg,
                                      bool dialog_was_suppressed,
                                      bool success,
                                      const base::string16& user_input) {
   RenderFrameHostImpl* rfh = RenderFrameHostImpl::FromID(render_process_id,
                                                          render_frame_id);
   last_dialog_suppressed_ = dialog_was_suppressed;
 
-  if (is_showing_before_unload_dialog_ && !success) {
+  // It is possible for the RenderFrameHost to have been swapped out in the
+  // meantime. Do not reset the navigation state in that case.
+  if (is_showing_before_unload_dialog_ && !success && rfh == GetMainFrame()) {

[Charlie Reis, 2016/03/24 21:50:41]

This won't work for subframes.  (I know we have some bugs about showing dialogs
from subframes, but this will need to work when we fix them.)

Can we compare the NavigationHandle's nav_entry_id to the pending entry ID
instead?

[clamy, 2016/03/25 13:43:48]

I don't quite see which NavigationHandle I should be comparing to the
NavigationEntry and what I would get from that. In the problematic case (ie the
commit of the pending rfh), rfh would have no NavigationHandle but the pending
RFH (which is now the current one) would have it. Even if I check that the IDs
match, this gives me no information on whether I need to cancel the navigation
or not. In other cases, the user could have canceled the dialog and the IDs
would still match. For example, this exact same case but where the user closed
the dialog instead of letting the pending navigation commit.

I cannot check that the state of the RFH is the default one, since the crash I'm
fixing happens in the middle of the swap out, the RFH has changed but its state
has not been modified yet.

I agree that this is problematic for subframes, but it seems to me that this
entire code does not handle the subframe case. It can probably be changed to
address them later.

[Charlie Reis, 2016/03/25 23:59:43]

Ok.  I hadn't looked closely enough to see which RFHs would have the
NavigationHandle in this case, and I was looking for a way to tell if the frame
was associated with the navigation or not.  Perhaps that's not the way.
It should be a simple fix to make it work for subframes, and I'd rather not dig
a deeper hole there.  Couldn't we check if rfh ==
rfh->frame_tree_node()->current_frame_host(), rather than assuming it's the main
frame?

[clamy, 2016/03/29 13:01:50]

Done.

     if (rfh)
       rfh->frame_tree_node()->BeforeUnloadCanceled();
     controller_.DiscardNonCommittedEntries();
 
     FOR_EACH_OBSERVER(WebContentsObserver, observers_,
                       BeforeUnloadDialogCancelled());

[Charlie Reis, 2016/03/25 23:59:43]

Is it safe to skip this?  Looks like it might confuse CoreTabHelper.

[clamy, 2016/03/29 13:01:50]

Since this could be problematic, I now only skip the reset of teh loading state
and the pending entry.

   }
 
   is_showing_before_unload_dialog_ = false;
   if (rfh) {
     rfh->JavaScriptDialogClosed(reply_msg, success, user_input,
                                 dialog_was_suppressed);
   } else {
     // Don't leak the sync IPC reply if the RFH or process is gone.
     delete reply_msg;
   }
@@ skipping 129 matching lines @@
   else
     WasHidden();
 }
 
 void WebContentsImpl::SetJavaScriptDialogManagerForTesting(
     JavaScriptDialogManager* dialog_manager) {
   dialog_manager_ = dialog_manager;
 }
 
 }  // namespace content