Avoid corrupting the pending_entry when an unexpected navigation commits.

content/browser/tab_contents/navigation_controller.cc

 // Copyright (c) 2011 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/tab_contents/navigation_controller.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/time.h"
@@ skipping 484 matching lines @@
 
   // Save the previous state before we clobber it.
   if (GetLastCommittedEntry()) {
     details->previous_url = GetLastCommittedEntry()->url();
     details->previous_entry_index = last_committed_entry_index();
   } else {
     details->previous_url = GURL();
     details->previous_entry_index = -1;
   }
 
-  // Assign the current site instance to any pending entry, so we can find it
-  // later by calling GetEntryIndexWithPageID. We only care about this if the
-  // pending entry is an existing navigation and not a new one (or else we
-  // wouldn't care about finding it with GetEntryIndexWithPageID).
-  //
-  // TODO(brettw) this seems slightly bogus as we don't really know if the
-  // pending entry is what this navigation is for. There is a similar TODO
-  // w.r.t. the pending entry in RendererDidNavigateToNewPage.
-  if (pending_entry_index_ >= 0) {
+  // The pending_entry has no SiteInstance when we are restoring an entry.  We
+  // must fill it in here so we can find the entry later by calling
+  // GetEntryIndexWithPageID.  In all other cases, the SiteInstance should be
+  // assigned already and we shouldn't change it.
+  if (pending_entry_index_ >= 0 && !pending_entry_->site_instance()) {

[brettw, 2011/03/22 05:39:18]

Is there a unit test covering this case?

[Charlie Reis, 2011/03/22 17:04:32]

Yes: NavigationControllerTest.RestoreNavigate checks that the site instance and
restore type are set properly.  It fails if this block is removed.

+    DCHECK(pending_entry_->restore_type() != NavigationEntry::RESTORE_NONE);
     pending_entry_->set_site_instance(tab_contents_->GetSiteInstance());
     pending_entry_->set_restore_type(NavigationEntry::RESTORE_NONE);
   }
 
   // is_in_page must be computed before the entry gets committed.
   details->is_in_page = IsURLInPageNavigation(params.url);
 
   // Do navigation-type specific actions. These will make and commit an entry.
   details->type = ClassifyNavigation(params);
 
@@ skipping 24 matching lines @@
     default:
       NOTREACHED();
   }
 
   // All committed entries should have nonempty content state so WebKit doesn't
   // get confused when we go back to them (see the function for details).
   DCHECK(!params.content_state.empty());
   NavigationEntry* active_entry = GetActiveEntry();
   active_entry->set_content_state(params.content_state);
 
+  // The active entry's SiteInstance should match our SiteInstance.
+  DCHECK(active_entry->site_instance() == tab_contents_->GetSiteInstance());
+
   // WebKit doesn't set the "auto" transition on meta refreshes properly (bug
   // 1051891) so we manually set it for redirects which we normally treat as
   // "non-user-gestures" where we want to update stuff after navigations.
   //
   // Note that the redirect check also checks for a pending entry to
   // differentiate real redirects from browser initiated navigations to a
   // redirected entry. This happens when you hit back to go to a page that was
   // the destination of a redirect, we don't want to treat it as a redirect
   // even though that's what its transition will be. See bug 1117048.
   //
@@ skipping 124 matching lines @@
     entries->push_back(entry);
   }
 }
 
 void NavigationController::RendererDidNavigateToNewPage(
     const ViewHostMsg_FrameNavigate_Params& params, bool* did_replace_entry) {
   NavigationEntry* new_entry;
   if (pending_entry_) {
     // TODO(brettw) this assumes that the pending entry is appropriate for the
     // new page that was just loaded. I don't think this is necessarily the
-    // case! We should have some more tracking to know for sure. This goes along
-    // with a similar TODO at the top of RendererDidNavigate where we blindly
-    // set the site instance on the pending entry.
+    // case! We should have some more tracking to know for sure.
     new_entry = new NavigationEntry(*pending_entry_);
 
     // Don't use the page type from the pending entry. Some interstitial page
     // may have set the type to interstitial. Once we commit, however, the page
     // type must always be normal.
     new_entry->set_page_type(NORMAL_PAGE);
   } else {
     new_entry = new NavigationEntry;
   }
 
@@ skipping 30 matching lines @@
   if (entry->update_virtual_url_with_url())
     UpdateVirtualURLToURL(entry, params.url);
   DCHECK(entry->site_instance() == NULL ||
          entry->site_instance() == tab_contents_->GetSiteInstance());
   entry->set_site_instance(tab_contents_->GetSiteInstance());
 
   entry->set_has_post_data(params.is_post);
 
   // The entry we found in the list might be pending if the user hit
   // back/forward/reload. This load should commit it (since it's already in the
-  // list, we can just discard the pending pointer).
+  // list, we can just discard the pending pointer).  We should also discard the
+  // pending entry if it corresponds to a different navigation, since that one
+  // is now likely canceled.

[brettw, 2011/03/22 05:39:18]

Can you add here that it may not be cancelled, but then when it commits it will
just look like a new navigation, which will be OK.

[Charlie Reis, 2011/03/22 17:04:32]

Done.

   //
   // Note that we need to use the "internal" version since we don't want to
   // actually change any other state, just kill the pointer.
-  if (entry == pending_entry_)
+  if (pending_entry_)
     DiscardNonCommittedEntriesInternal();
 
   // If a transient entry was removed, the indices might have changed, so we
   // have to query the entry index again.
   last_committed_entry_index_ =
       GetEntryIndexWithPageID(tab_contents_->GetSiteInstance(), params.page_id);
 }
 
 void NavigationController::RendererDidNavigateToSamePage(
     const ViewHostMsg_FrameNavigate_Params& params) {
@@ skipping 414 matching lines @@
   size_t insert_index = 0;
   for (int i = 0; i < max_index; i++) {
     // When cloning a tab, copy all entries except interstitial pages
     if (source.entries_[i].get()->page_type() != INTERSTITIAL_PAGE) {
       entries_.insert(entries_.begin() + insert_index++,
                       linked_ptr<NavigationEntry>(
                           new NavigationEntry(*source.entries_[i])));
     }
   }
 }