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 208 matching lines @@
   // water marks.
   low_water_mark_ = high_water_mark_ = t;
   return t;
 }
 
 NavigationControllerImpl::NavigationControllerImpl(
     NavigationControllerDelegate* delegate,
     BrowserContext* browser_context)
     : browser_context_(browser_context),
       pending_entry_(NULL),
+      failed_pending_entry_id_(0),
+      failed_pending_entry_should_replace_(false),
       last_committed_entry_index_(-1),
       pending_entry_index_(-1),
       transient_entry_index_(-1),
       delegate_(delegate),
       max_restored_page_id_(-1),
       ssl_manager_(this),
       needs_reload_(false),
       is_initial_navigation_(true),
       in_navigate_to_pending_entry_(false),
       pending_reload_(NO_RELOAD),
@@ skipping 550 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.

[Charlie Reis, 2015/04/07 18:00:47]

Might also be worth mentioning the error page case, and that we need to remember
whether the error page should replace the current entry from DidFail to
DidCommit.

[Avi (use Gerrit), 2015/04/07 18:22:58]

Done.

+  if (params.url_is_unreachable && failed_pending_entry_id_ != 0) {
+    details->did_replace_entry = failed_pending_entry_should_replace_;
+  } 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 (was_failure && pending_entry_) {
+    failed_pending_entry_id_ = pending_entry_->GetUniqueID();
+    failed_pending_entry_should_replace_ =
+        pending_entry_->should_replace_entry();
+  } else {
+    failed_pending_entry_id_ = 0;
+  }
+
   if (pending_entry_index_ == -1)
     delete pending_entry_;
   pending_entry_ = NULL;
   pending_entry_index_ = -1;
 }
 
 void NavigationControllerImpl::DiscardTransientEntry() {
   if (transient_entry_index_ == -1)
     return;
   entries_.erase(entries_.begin() + transient_entry_index_);
@@ skipping 48 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content