diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc index 8339b73051bb2..6c6e294dbbc16 100644 --- a/content/browser/frame_host/navigation_controller_impl.cc +++ b/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(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(); diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc index 6cc541ef01a1e..0b13e3ac86995 100644 --- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc +++ b/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( + 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 = diff --git a/content/browser/frame_host/navigation_controller_impl_unittest.cc b/content/browser/frame_host/navigation_controller_impl_unittest.cc index cd10a01267ce5..9a821578ef474 100644 --- a/content/browser/frame_host/navigation_controller_impl_unittest.cc +++ b/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(¬ifications, &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 diff --git a/content/common/view_messages.h b/content/common/view_messages.h index 448a65fd0bce0..b0934b0bf8c64 100644 --- a/content/common/view_messages.h +++ b/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, diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc index 3bae908ba1c90..5f6ea90a3f376 100644 --- a/content/renderer/render_frame_impl.cc +++ b/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) { diff --git a/content/renderer/render_frame_impl.h b/content/renderer/render_frame_impl.h index e7df9615d1705..40418cafa19fc 100644 --- a/content/renderer/render_frame_impl.h +++ b/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 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); diff --git a/content/renderer/render_thread_impl.cc b/content/renderer/render_thread_impl.cc index 0d3aa9a0757b3..972002a17b47d 100644 --- a/content/renderer/render_thread_impl.cc +++ b/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; diff --git a/content/renderer/render_thread_impl.h b/content/renderer/render_thread_impl.h index 5b79e723018ad..ef73307719e05 100644 --- a/content/renderer/render_thread_impl.h +++ b/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); diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc index bf2f15de09cf9..739b0be6784f2 100644 --- a/content/renderer/render_view_browsertest.cc +++ b/content/renderer/render_view_browsertest.cc @@ -907,86 +907,6 @@ TEST_F(RenderViewImplTest, DISABLED_LastCommittedUpdateState) { EXPECT_EQ(state_C, state); } -// Test that stale back/forward navigations arriving from the browser are -// ignored. See http://crbug.com/86758. -TEST_F(RenderViewImplTest, StaleNavigationsIgnored) { - // Load page A. - LoadHTML("
Page A
"); - EXPECT_EQ(1, view()->history_list_length_); - EXPECT_EQ(0, view()->history_list_offset_); - - // Load page B, which will trigger an UpdateState message for page A. - LoadHTML("
Page B
"); - EXPECT_EQ(2, view()->history_list_length_); - EXPECT_EQ(1, view()->history_list_offset_); - - // Check for a valid UpdateState message for page A. - ProcessPendingMessages(); - const IPC::Message* msg_A = render_thread_->sink().GetUniqueMessageMatching( - ViewHostMsg_UpdateState::ID); - ASSERT_TRUE(msg_A); - ViewHostMsg_UpdateState::Param param; - ViewHostMsg_UpdateState::Read(msg_A, ¶m); - int page_id_A = get<0>(param); - PageState state_A = get<1>(param); - EXPECT_EQ(1, page_id_A); - render_thread_->sink().ClearMessages(); - - // Back to page A (nav_entry_id 1) and commit. - CommonNavigationParams common_params_A; - RequestNavigationParams request_params_A; - common_params_A.navigation_type = FrameMsg_Navigate_Type::NORMAL; - common_params_A.transition = ui::PAGE_TRANSITION_FORWARD_BACK; - request_params_A.current_history_list_length = 2; - request_params_A.current_history_list_offset = 1; - request_params_A.pending_history_list_offset = 0; - request_params_A.page_id = 1; - request_params_A.nav_entry_id = 1; - request_params_A.page_state = state_A; - NavigateMainFrame(common_params_A, StartNavigationParams(), request_params_A); - ProcessPendingMessages(); - - // A new navigation commits, clearing the forward history. - LoadHTML("
Page C
"); - EXPECT_EQ(2, view()->history_list_length_); - EXPECT_EQ(1, view()->history_list_offset_); - EXPECT_EQ(3, view()->page_id_); // page C is now page id 3 - int was_page_c = -1; - base::string16 check_page_c = base::ASCIIToUTF16( - "Number(document.getElementById('pagename').innerHTML == 'Page C')"); - EXPECT_TRUE(ExecuteJavaScriptAndReturnIntValue(check_page_c, &was_page_c)); - EXPECT_EQ(1, was_page_c); - - // The browser then sends a stale navigation to B, which should be ignored. - CommonNavigationParams common_params_B; - RequestNavigationParams request_params_B; - common_params_B.navigation_type = FrameMsg_Navigate_Type::NORMAL; - common_params_B.transition = ui::PAGE_TRANSITION_FORWARD_BACK; - request_params_B.current_history_list_length = 2; - request_params_B.current_history_list_offset = 0; - request_params_B.pending_history_list_offset = 1; - request_params_B.page_id = 2; - request_params_B.nav_entry_id = 2; - request_params_B.page_state = - state_A; // Doesn't matter, just has to be present. - NavigateMainFrame(common_params_B, StartNavigationParams(), request_params_B); - - // State should be unchanged. - EXPECT_EQ(2, view()->history_list_length_); - EXPECT_EQ(1, view()->history_list_offset_); - EXPECT_EQ(3, view()->page_id_); // page C, not page B - was_page_c = -1; - EXPECT_TRUE(ExecuteJavaScriptAndReturnIntValue(check_page_c, &was_page_c)); - EXPECT_EQ(1, was_page_c); - - // Check for a valid DidDropNavigation message. - ProcessPendingMessages(); - const IPC::Message* msg = render_thread_->sink().GetUniqueMessageMatching( - FrameHostMsg_DidDropNavigation::ID); - ASSERT_TRUE(msg); - render_thread_->sink().ClearMessages(); -} - // Test that our IME backend sends a notification message when the input focus // changes. TEST_F(RenderViewImplTest, OnImeTypeChanged) {