Merge pull request #261 from axinging/xing_xwalk-4378-saverestorestate

[Backport]Don't discard navigations.

content/browser/frame_host/navigation_controller_impl.cc

@@ -994,44 +994,9 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
       rfh->GetSiteInstance(),
       params.page_id);
   if (existing_entry_index == -1) {
-    // The page was not found. It could have been pruned because of the limit on
-    // back/forward entries (not likely since we'll usually tell it to navigate
-    // to such entries). It could also mean that the renderer is smoking crack.
-    NOTREACHED();
-
-    // Because the unknown entry has committed, we risk showing the wrong URL in
-    // release builds. Instead, we'll kill the renderer process to be safe.
-    LOG(ERROR) << "terminating renderer for bad navigation: " << params.url;
-    RecordAction(base::UserMetricsAction("BadMessageTerminate_NC"));
-
-    // Temporary code so we can get more information.  Format:
-    //  http://url/foo.html#page1#max3#frame1#ids:2_Nx,1_1x,3_2
-    std::string temp = params.url.spec();
-    temp.append("#page");
-    temp.append(base::IntToString(params.page_id));
-    temp.append("#max");
-    temp.append(base::IntToString(delegate_->GetMaxPageID()));
-    temp.append("#frame");
-    temp.append(base::IntToString(rfh->GetRoutingID()));
-    temp.append("#ids");
-    for (int i = 0; i < static_cast<int>(entries_.size()); ++i) {
-      // Append entry metadata (e.g., 3_7x):
-      //  3: page_id
-      //  7: SiteInstance ID, or N for null
-      //  x: appended if not from the current SiteInstance
-      temp.append(base::IntToString(entries_[i]->GetPageID()));
-      temp.append("_");
-      if (entries_[i]->site_instance())
-        temp.append(base::IntToString(entries_[i]->site_instance()->GetId()));
-      else
-        temp.append("N");
-      if (entries_[i]->site_instance() != rfh->GetSiteInstance())
-        temp.append("x");
-      temp.append(",");
-    }
-    GURL url(temp);
-    rfh->render_view_host()->Send(new ViewMsg_TempCrashWithData(url));
-    return NAVIGATION_TYPE_NAV_IGNORE;
+    // The renderer has committed a navigation to an entry that no longer
+    // exists. Because the renderer is showing that page, resurrect that entry.
+    return NAVIGATION_TYPE_NEW_PAGE;
   }
   NavigationEntryImpl* existing_entry = entries_[existing_entry_index].get();
 
@@ -1181,12 +1146,9 @@ NavigationType NavigationControllerImpl::ClassifyNavigationWithoutPageID(
   // Now we know that the notification is for an existing page. Find that entry.
   int existing_entry_index = GetEntryIndexWithUniqueID(params.nav_entry_id);
   if (existing_entry_index == -1) {
-    // The page was not found. It could have been pruned because of the limit on
-    // back/forward entries (not likely since we'll usually tell it to navigate
-    // to such entries). It could also mean that the renderer is smoking crack.
-    // TODO(avi): Crash the renderer like we do in the old ClassifyNavigation?
-    NOTREACHED() << "Could not find nav entry with id " << params.nav_entry_id;
-    return NAVIGATION_TYPE_NAV_IGNORE;
+    // The renderer has committed a navigation to an entry that no longer
+    // exists. Because the renderer is showing that page, resurrect that entry.
+    return NAVIGATION_TYPE_NEW_PAGE;
   }
 
   // Any top-level navigations with the same base (minus the reference fragment)
@@ -1213,8 +1175,11 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage(
   bool update_virtual_url;
   // Only make a copy of the pending entry if it is appropriate for the new page
   // that was just loaded.  We verify this at a coarse grain by checking that
-  // the SiteInstance hasn't been assigned to something else.
-  if (pending_entry_ &&
+  // the SiteInstance hasn't been assigned to something else, and by making sure
+  // that the pending entry was intended as a new entry (rather than being a
+  // history navigation that was interrupted by an unrelated, renderer-initiated
+  // navigation).
+  if (pending_entry_ && pending_entry_index_ == -1 &&
       (!pending_entry_->site_instance() ||
        pending_entry_->site_instance() == rfh->GetSiteInstance())) {
     new_entry = pending_entry_->Clone();
@@ -1755,13 +1720,13 @@ void NavigationControllerImpl::InsertOrReplaceEntry(NavigationEntryImpl* entry,
                                                     bool replace) {
   DCHECK(entry->GetTransitionType() != ui::PAGE_TRANSITION_AUTO_SUBFRAME);
 
-  // Copy the pending entry's unique ID to the committed entry.
-  // I don't know if pending_entry_index_ can be other than -1 here.
-  const NavigationEntryImpl* const pending_entry =
-      (pending_entry_index_ == -1) ?
-          pending_entry_ : entries_[pending_entry_index_].get();
-  if (pending_entry)
-    entry->set_unique_id(pending_entry->GetUniqueID());
+  // If the pending_entry_index_ is -1, the navigation was to a new page, and we
+  // need to keep continuity with the pending entry, so copy the pending entry's
+  // unique ID to the committed entry. If the pending_entry_index_ isn't -1,
+  // then the renderer navigated on its own, independent of the pending entry,
+  // so don't copy anything.
+  if (pending_entry_ && pending_entry_index_ == -1)
+    entry->set_unique_id(pending_entry_->GetUniqueID());
 
   DiscardNonCommittedEntriesInternal();
 

content/browser/frame_host/navigation_controller_impl_browsertest.cc

@@ -57,11 +57,31 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, LoadDataWithBaseURL) {
   EXPECT_EQ(controller.GetVisibleEntry()->GetOriginalRequestURL(), history_url);
 }
 
-// The renderer uses the position in the history list as a clue to whether a
-// navigation is stale. In the case where the entry limit is reached and the
-// history list is pruned, make sure that there is no mismatch that would cause
-// it to start incorrectly rejecting navigations as stale. See
-// http://crbug.com/89798.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, UniqueIDs) {
+  const NavigationControllerImpl& controller =
+      static_cast<const NavigationControllerImpl&>(
+          shell()->web_contents()->GetController());
+
+  GURL main_url(embedded_test_server()->GetURL(
+      "/navigation_controller/page_with_link_to_load_iframe.html"));
+  NavigateToURL(shell(), main_url);
+  ASSERT_EQ(1, controller.GetEntryCount());
+
+  // Use JavaScript to click the link and load the iframe.
+  std::string script = "document.getElementById('link').click()";
+  EXPECT_TRUE(content::ExecuteScript(shell()->web_contents(), script));
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  ASSERT_EQ(2, controller.GetEntryCount());
+
+  // Unique IDs should... um... be unique.
+  ASSERT_NE(controller.GetEntryAtIndex(0)->GetUniqueID(),
+            controller.GetEntryAtIndex(1)->GetUniqueID());
+}
+
+// This test used to make sure that a scheme used to prevent spoofs didn't ever
+// interfere with navigations. We switched to a different scheme, so now this is
+// just a test to make sure we can still navigate once we prune the history
+// list.
 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
                        DontIgnoreBackAfterNavEntryLimit) {
   NavigationController& controller =

content/browser/frame_host/navigation_controller_impl_unittest.cc

@@ -4894,4 +4894,66 @@ TEST_F(NavigationControllerTest, UnreachableURLGivesErrorPage) {
   }
 }
 
+// Tests that if a stale navigation comes back from the renderer, it is properly
+// resurrected.
+TEST_F(NavigationControllerTest, StaleNavigationsResurrected) {
+  NavigationControllerImpl& controller = controller_impl();
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, &controller);
+
+  // Start on page A.
+  const GURL url_a("http://foo.com/a");
+  main_test_rfh()->NavigateAndCommitRendererInitiated(0, true, url_a);
+  EXPECT_EQ(1U, navigation_entry_committed_counter_);
+  navigation_entry_committed_counter_ = 0;
+  EXPECT_EQ(1, controller.GetEntryCount());
+  EXPECT_EQ(0, controller.GetCurrentEntryIndex());
+
+  // Go to page B.
+  const GURL url_b("http://foo.com/b");
+  main_test_rfh()->NavigateAndCommitRendererInitiated(1, true, url_b);
+  EXPECT_EQ(1U, navigation_entry_committed_counter_);
+  navigation_entry_committed_counter_ = 0;
+  EXPECT_EQ(2, controller.GetEntryCount());
+  EXPECT_EQ(1, controller.GetCurrentEntryIndex());
+  int b_entry_id = controller.GetLastCommittedEntry()->GetUniqueID();
+  int b_page_id = controller.GetLastCommittedEntry()->GetPageID();
+
+  // Back to page A.
+  controller.GoBack();
+  contents()->CommitPendingNavigation();
+  EXPECT_EQ(1U, navigation_entry_committed_counter_);
+  navigation_entry_committed_counter_ = 0;
+  EXPECT_EQ(2, controller.GetEntryCount());
+  EXPECT_EQ(0, controller.GetCurrentEntryIndex());
+
+  // Start going forward to page B.
+  controller.GoForward();
+
+  // But the renderer unilaterally navigates to page C, pruning B.
+  const GURL url_c("http://foo.com/c");
+  main_test_rfh()->NavigateAndCommitRendererInitiated(2, true, url_c);
+  EXPECT_EQ(1U, navigation_entry_committed_counter_);
+  navigation_entry_committed_counter_ = 0;
+  EXPECT_EQ(2, controller.GetEntryCount());
+  EXPECT_EQ(1, controller.GetCurrentEntryIndex());
+  int c_entry_id = controller.GetLastCommittedEntry()->GetUniqueID();
+  EXPECT_NE(c_entry_id, b_entry_id);
+
+  // And then the navigation to B gets committed.
+  main_test_rfh()->SendNavigate(b_page_id, b_entry_id, false, url_b);
+  EXPECT_EQ(1U, navigation_entry_committed_counter_);
+  navigation_entry_committed_counter_ = 0;
+
+  // Even though we were doing a history navigation, because the entry was
+  // pruned it will end up as a *new* entry at the end of the entry list. This
+  // means that occasionally a navigation conflict will end up with one entry
+  // bubbling to the end of the entry list, but that's the least-bad option.
+  EXPECT_EQ(3, controller.GetEntryCount());
+  EXPECT_EQ(2, controller.GetCurrentEntryIndex());
+  EXPECT_EQ(url_a, controller.GetEntryAtIndex(0)->GetURL());
+  EXPECT_EQ(url_c, controller.GetEntryAtIndex(1)->GetURL());
+  EXPECT_EQ(url_b, controller.GetEntryAtIndex(2)->GetURL());
+}
+
 }  // namespace content

content/common/view_messages.h

@@ -895,10 +895,6 @@ IPC_MESSAGE_ROUTED2(ViewMsg_SavePageAsMHTML,
                     int /* job_id */,
                     IPC::PlatformFileForTransit /* file handle */)
 
-// Temporary message to diagnose an unexpected condition in WebContentsImpl.
-IPC_MESSAGE_CONTROL1(ViewMsg_TempCrashWithData,
-                     GURL /* data */)
-
 // An acknowledge to ViewHostMsg_MultipleTargetsTouched to notify the renderer
 // process to release the magnified image.
 IPC_MESSAGE_ROUTED1(ViewMsg_ReleaseDisambiguationPopupBitmap,

content/renderer/render_frame_impl.cc

@@ -4139,12 +4139,8 @@ void RenderFrameImpl::OnFailedNavigation(
   bool is_history_navigation = request_params.page_state.IsValid();
   WebURLRequest::CachePolicy cache_policy =
       WebURLRequest::UseProtocolCachePolicy;
-  if (!RenderFrameImpl::PrepareRenderViewForNavigation(
-          common_params.url, is_history_navigation, request_params, &is_reload,
-          &cache_policy)) {
-    Send(new FrameHostMsg_DidDropNavigation(routing_id_));
-    return;
-  }
+  RenderFrameImpl::PrepareRenderViewForNavigation(
+      common_params.url, request_params, &is_reload, &cache_policy);
 
   GetContentClient()->SetActiveURL(common_params.url);
 
@@ -4166,6 +4162,8 @@ void RenderFrameImpl::OnFailedNavigation(
   SendFailedProvisionalLoad(failed_request, error, frame_);
 
   if (!ShouldDisplayErrorPageForFailedLoad(error_code, common_params.url)) {
+    // TODO(avi): Remove this; we shouldn't ever be dropping navigations.
+    // http://crbug.com/501960
     Send(new FrameHostMsg_DidDropNavigation(routing_id_));
     return;
   }
@@ -4470,12 +4468,8 @@ void RenderFrameImpl::NavigateInternal(
   bool is_history_navigation = request_params.page_state.IsValid();
   WebURLRequest::CachePolicy cache_policy =
       WebURLRequest::UseProtocolCachePolicy;
-  if (!RenderFrameImpl::PrepareRenderViewForNavigation(
-          common_params.url, is_history_navigation, request_params, &is_reload,
-          &cache_policy)) {
-    Send(new FrameHostMsg_DidDropNavigation(routing_id_));
-    return;
-  }
+  RenderFrameImpl::PrepareRenderViewForNavigation(
+      common_params.url, request_params, &is_reload, &cache_policy);
 
   GetContentClient()->SetActiveURL(common_params.url);
 
@@ -4694,28 +4688,18 @@ RenderFrameImpl::CreateRendererFactory() {
 #endif
 }
 
-bool RenderFrameImpl::PrepareRenderViewForNavigation(
+void RenderFrameImpl::PrepareRenderViewForNavigation(
     const GURL& url,
-    bool is_history_navigation,
     const RequestNavigationParams& request_params,
     bool* is_reload,
     WebURLRequest::CachePolicy* cache_policy) {
+  DCHECK(render_view_->webview());
+
   MaybeHandleDebugURL(url);
-  if (!render_view_->webview())
-    return false;
 
   FOR_EACH_OBSERVER(
       RenderViewObserver, render_view_->observers_, Navigate(url));
 
-  // If this is a stale back/forward (due to a recent navigation the browser
-  // didn't know about), ignore it. Only check if swapped in because if the
-  // frame is swapped out, it won't commit before asking the browser.
-  if (!render_view_->is_swapped_out() && is_history_navigation &&
-      render_view_->history_list_offset_ !=
-          request_params.current_history_list_offset) {
-    return false;
-  }
-
   render_view_->history_list_offset_ =
       request_params.current_history_list_offset;
   render_view_->history_list_length_ =
@@ -4726,7 +4710,7 @@ bool RenderFrameImpl::PrepareRenderViewForNavigation(
   }
 
   if (!is_swapped_out_ || frame_->parent())
-    return true;
+    return;
 
   // This is a swapped out main frame, so swap the renderer back in.
   // We marked the view as hidden when swapping the view out, so be sure to
@@ -4748,7 +4732,7 @@ bool RenderFrameImpl::PrepareRenderViewForNavigation(
 
   render_view_->SetSwappedOut(false);
   is_swapped_out_ = false;
-  return true;
+  return;
 }
 
 void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request) {

content/renderer/render_frame_impl.h

@@ -770,11 +770,9 @@ class CONTENT_EXPORT RenderFrameImpl
   // The method is virtual so that layouttests can override it.
   virtual scoped_ptr<MediaStreamRendererFactory> CreateRendererFactory();
 
-  // Checks that the RenderView is ready to display the navigation to |url|. If
-  // the return value is false, the navigation should be abandoned.
-  bool PrepareRenderViewForNavigation(
+  // Does preparation for the navigation to |url|.
+  void PrepareRenderViewForNavigation(
       const GURL& url,
-      bool is_history_navigation,
       const RequestNavigationParams& request_params,
       bool* is_reload,
       blink::WebURLRequest::CachePolicy* cache_policy);

content/renderer/render_thread_impl.cc

@@ -1524,7 +1524,6 @@ bool RenderThreadImpl::OnControlMessageReceived(const IPC::Message& msg) {
     // is there a new non-windows message I should add here?
     IPC_MESSAGE_HANDLER(ViewMsg_New, OnCreateNewView)
     IPC_MESSAGE_HANDLER(ViewMsg_NetworkTypeChanged, OnNetworkTypeChanged)
-    IPC_MESSAGE_HANDLER(ViewMsg_TempCrashWithData, OnTempCrashWithData)
     IPC_MESSAGE_HANDLER(WorkerProcessMsg_CreateWorker, OnCreateNewSharedWorker)
     IPC_MESSAGE_HANDLER(ViewMsg_TimezoneChange, OnUpdateTimezone)
 #if defined(OS_ANDROID)
@@ -1684,11 +1683,6 @@ void RenderThreadImpl::OnNetworkTypeChanged(
       NetConnectionTypeToWebConnectionType(type));
 }
 
-void RenderThreadImpl::OnTempCrashWithData(const GURL& data) {
-  GetContentClient()->SetActiveURL(data);
-  CHECK(false);
-}
-
 void RenderThreadImpl::OnUpdateTimezone(const std::string& zone_id) {
   if (!blink_platform_impl_)
     return;

content/renderer/render_thread_impl.h

@@ -451,7 +451,6 @@ class CONTENT_EXPORT RenderThreadImpl
 #endif
   void OnNetworkTypeChanged(net::NetworkChangeNotifier::ConnectionType type);
   void OnGetAccessibilityTree();
-  void OnTempCrashWithData(const GURL& data);
   void OnUpdateTimezone(const std::string& zoneId);
   void OnMemoryPressure(
       base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level);