Fix incorrect creation of duplicate navigation entries for repeated page load failures.

content/browser/frame_host/navigation_controller_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/browser/frame_host/navigation_controller_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
@@ skipping 778 matching lines @@
     details->previous_entry_index = -1;
   }
 
   // If we have a pending entry at this point, it should have a SiteInstance.
   // Restored entries start out with a null SiteInstance, but we should have
   // assigned one in NavigateToPendingEntry.
   DCHECK(pending_entry_index_ == -1 || pending_entry_->site_instance());
 
   // If we are doing a cross-site reload, we need to replace the existing
   // navigation entry, not add another entry to the history. This has the side
-  // effect of removing forward browsing history, if such existed.
-  // Or if we are doing a cross-site redirect navigation,
-  // we will do a similar thing.
-  details->did_replace_entry =
-      pending_entry_ && pending_entry_->should_replace_entry();
+  // effect of removing forward browsing history, if such existed. Or if we are
+  // doing a cross-site redirect navigation, we will do a similar thing.
+  if (params.url_is_unreachable) {

[Charlie Reis, 2015/04/03 20:59:46]

Should this be params.url_is_unreachable && failed_pending_entry_.get()?  See my
comment in navigator_impl.cc for why there might be a bug here.

[Avi (use Gerrit), 2015/04/06 19:38:16]

I'm trying to come up with a scenario in which there's a difference. Your
comment in NavigatorImpl clearly explains that the whole setup here is bad, but
doesn't show a specific scenario.

I'm imagining two frames loading as part of the same navigation.

If frame 1 errors, then it sets the failure variables, and doesn't load an error
page. Frame 2 commits, and it isn't an error load so it hits the else here.
There won't be a pending entry, so we get false overall. Maybe that second frame
commit should get the should_replace value from the (now dead) pending entry,
but the if() expression would need to be an || not &&. However, we can't do that
because of the possibility, like below, of the error values floating, in which
case the || would cause intermixing with completely unrelated future loads.

If frame 1 succeeds but frame 2 fails, then it doesn't matter as there won't be
a subsequent error page load.

If both frames fail, then both are url_is_unreachable, and it works as-is.

Can you clarify the scenario in which this code makes things worse?

[Charlie Reis, 2015/04/06 20:48:10]

The multiple frames thing is a different issue, and one that we can't resolve in
this if statement.  For example:
1) Frame 1 fails and sets failed_pending_entry_should_replace_ to false.
2) Frame 2 fails and sets failed_pending_entry_should_replace_ to true.
3) Frame 1 commits its error page.  We get here and set did_replace_entry to
true (incorrectly).

The bug I was talking about is that the pending entry might not have been
discarded when failing.  We need a case where should_preserve_entry is false but
should_replace_entry() is true for this bug to become visible.  I think that's
possible like this:
1) Start on the NTP.
2) Navigate to a test server page, then another page.
3) Go back to the NTP.
4) Clear your cache and shut down the test server.
5) Go forward.

The forward operation will fail and have should_replace_entry() == true, right? 
But should_preserve_entry will be false because
Browser::ShouldPreserveAbortedURLs sees the last committed page is the NTP.

Thus, when the commit comes in, we'll still have the pending entry and no
failed_pending_entry_*, meaning that we won't replace the desired entry with the
error message.  That means we'll probably clear the forward history when we
shouldn't.

(Note: it's just luck that I found what might be a test case here.  If this
doesn't work or there isn't a test case, we should still fix this bug.)

[Avi (use Gerrit), 2015/04/06 21:30:14]

Yeah, I got it. The pending entry isn't guaranteed to be cleared on error, so if
there is an error, we still might need to check the pending entry.

+    details->did_replace_entry = failed_pending_entry_.get() &&
+                                 failed_pending_entry_->should_replace_entry();
+  } else {
+    details->did_replace_entry = pending_entry_ &&
+                                 pending_entry_->should_replace_entry();
+  }
 
   // Do navigation-type specific actions. These will make and commit an entry.
   details->type = ClassifyNavigation(rfh, params);
 
   // is_in_page must be computed before the entry gets committed.
   details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(),
       params.url, params.was_within_same_page, rfh);
 
   switch (details->type) {
     case NAVIGATION_TYPE_NEW_PAGE:
@@ skipping 937 matching lines @@
                                              RestoreType type) {
   DCHECK(selected_index >= 0 && selected_index < GetEntryCount());
   ConfigureEntriesForRestore(&entries_, type);
 
   SetMaxRestoredPageID(static_cast<int32>(GetEntryCount()));
 
   last_committed_entry_index_ = selected_index;
 }
 
 void NavigationControllerImpl::DiscardNonCommittedEntriesInternal() {
-  DiscardPendingEntry();
+  DiscardPendingEntry(false);
   DiscardTransientEntry();
 }
 
-void NavigationControllerImpl::DiscardPendingEntry() {
+void NavigationControllerImpl::DiscardPendingEntry(bool was_failure) {
   // It is not safe to call DiscardPendingEntry while NavigateToEntry is in
   // progress, since this will cause a use-after-free.  (We only allow this
   // when the tab is being destroyed for shutdown, since it won't return to
   // NavigateToEntry in that case.)  http://crbug.com/347742.
   CHECK(!in_navigate_to_pending_entry_ || delegate_->IsBeingDestroyed());
 
-  if (pending_entry_index_ == -1)
-    delete pending_entry_;
+  if (pending_entry_index_ == -1) {
+    if (was_failure)
+      failed_pending_entry_.reset(pending_entry_);
+    else
+      delete pending_entry_;
+  }
+
   pending_entry_ = NULL;
   pending_entry_index_ = -1;
+  if (!was_failure)
+    failed_pending_entry_.reset();

[Charlie Reis, 2015/04/03 20:59:46]

You've confirmed this gets cleared after the error page commits, right?  (I
think we should get here via the DidCommit of the error page.)

[Avi (use Gerrit), 2015/04/06 19:38:16]

Yes, this gets cleared by the DiscardNonCommittedEntriesInternal in DidNavigate
for the error page commit. OTOH, you are right, we have no guarantees that there
will be an error page.

If you look at Camille's patch, she is deliberately loading an error page from
the browser process on failure, rather than relying on the renderer process
unilaterally doing that without telling the browser process. I see a variation
of that approach as being the sanest way forward here in a subsequent CL.

 }
 
 void NavigationControllerImpl::DiscardTransientEntry() {
   if (transient_entry_index_ == -1)
     return;
   entries_.erase(entries_.begin() + transient_entry_index_);
   if (last_committed_entry_index_ > transient_entry_index_)
     last_committed_entry_index_--;
   transient_entry_index_ = -1;
 }
@@ skipping 44 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content