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

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 0a748f1078548df692d61e70b33952ee0b2ec6dc..948721bf3adfebeac585ea6203fac806b7cc62da 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -4690,7 +4690,9 @@ void WebContentsImpl::OnDialogClosed(int 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();