Avoid stale navigation requests without excessive page id knowledge in the renderer process.
This reverts the page id portions of r92748 and r96724, but keeps some unrelated cleanup as well as some tests.
BUG=431853, 178491
TEST=covered by tests, as well as it shouldn't regress the original bug 86758
Review URL: https://codereview.chromium.org/743803002
Cr-Commit-Position: refs/heads/master@{#307614}
diff --git a/content/browser/frame_host/navigation_controller_delegate.h b/content/browser/frame_host/navigation_controller_delegate.h
index 52567ef..90fd4d3 100644
--- a/content/browser/frame_host/navigation_controller_delegate.h
+++ b/content/browser/frame_host/navigation_controller_delegate.h
@@ -51,10 +51,8 @@
const LoadCommittedDetails& load_details) = 0;
virtual bool NavigateToPendingEntry(
NavigationController::ReloadType reload_type) = 0;
- virtual void SetHistoryLengthAndPrune(
- const SiteInstance* site_instance,
- int merge_history_length,
- int32 minimum_page_id) = 0;
+ virtual void SetHistoryOffsetAndLength(int history_offset,
+ int history_length) = 0;
virtual void CopyMaxPageIDsFrom(WebContents* web_contents) = 0;
virtual void UpdateMaxPageID(int32 page_id) = 0;
virtual void UpdateMaxPageIDForSiteInstance(SiteInstance* site_instance,
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 4be5a3ce..ecd60437 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -1317,19 +1317,8 @@
NavigationControllerImpl* source =
static_cast<NavigationControllerImpl*>(temp);
- // The SiteInstance and page_id of the last committed entry needs to be
- // remembered at this point, in case there is only one committed entry
- // and it is pruned. We use a scoped_refptr to ensure the SiteInstance
- // can't be freed during this time period.
- NavigationEntryImpl* last_committed =
- NavigationEntryImpl::FromNavigationEntry(GetLastCommittedEntry());
- scoped_refptr<SiteInstance> site_instance(
- last_committed->site_instance());
- int32 minimum_page_id = last_committed->GetPageID();
- int32 max_page_id =
- delegate_->GetMaxPageIDForSiteInstance(site_instance.get());
- // Remove all the entries leaving the active entry.
+ // Remove all the entries leaving the last committed entry.
PruneAllButLastCommittedInternal();
// We now have one entry, possibly with a new pending entry. Ensure that
@@ -1339,7 +1328,7 @@
source->PruneOldestEntryIfFull();
// Insert the entries from source. Don't use source->GetCurrentEntryIndex as
- // we don't want to copy over the transient entry. Ignore any pending entry,
+ // we don't want to copy over the transient entry. Ignore any pending entry,
// since it has not committed in source.
int max_source_index = source->last_committed_entry_index_;
if (max_source_index == -1)
@@ -1358,22 +1347,20 @@
// Adjust indices such that the last entry and pending are at the end now.
last_committed_entry_index_ = GetEntryCount() - 1;
- delegate_->SetHistoryLengthAndPrune(site_instance.get(),
- max_source_index,
- minimum_page_id);
+ delegate_->SetHistoryOffsetAndLength(last_committed_entry_index_,
+ GetEntryCount());
- // Copy the max page id map from the old tab to the new tab. This ensures
- // that new and existing navigations in the tab's current SiteInstances
- // are identified properly.
+ // Copy the max page id map from the old tab to the new tab. This ensures that
+ // new and existing navigations in the tab's current SiteInstances are
+ // identified properly.
+ NavigationEntryImpl* last_committed =
+ NavigationEntryImpl::FromNavigationEntry(GetLastCommittedEntry());
+ int32 site_max_page_id =
+ delegate_->GetMaxPageIDForSiteInstance(last_committed->site_instance());
delegate_->CopyMaxPageIDsFrom(source->delegate()->GetWebContents());
+ delegate_->UpdateMaxPageIDForSiteInstance(last_committed->site_instance(),
+ site_max_page_id);
max_restored_page_id_ = source->max_restored_page_id_;
-
- // If there is a last committed entry, be sure to include it in the new
- // max page ID map.
- if (max_page_id > -1) {
- delegate_->UpdateMaxPageIDForSiteInstance(site_instance.get(),
- max_page_id);
- }
}
bool NavigationControllerImpl::CanPruneAllButLastCommitted() {
@@ -1400,18 +1387,11 @@
void NavigationControllerImpl::PruneAllButLastCommitted() {
PruneAllButLastCommittedInternal();
- // We should still have a last committed entry.
- DCHECK_NE(-1, last_committed_entry_index_);
+ DCHECK_EQ(0, last_committed_entry_index_);
+ DCHECK_EQ(1, GetEntryCount());
- // We pass 0 instead of GetEntryCount() for the history_length parameter of
- // SetHistoryLengthAndPrune, because it will create history_length additional
- // history entries.
- // TODO(jochen): This API is confusing and we should clean it up.
- // http://crbug.com/178491
- NavigationEntryImpl* entry =
- NavigationEntryImpl::FromNavigationEntry(GetVisibleEntry());
- delegate_->SetHistoryLengthAndPrune(
- entry->site_instance(), 0, entry->GetPageID());
+ delegate_->SetHistoryOffsetAndLength(last_committed_entry_index_,
+ GetEntryCount());
}
void NavigationControllerImpl::PruneAllButLastCommittedInternal() {
diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index 20c00ee..3eca788 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -3,9 +3,11 @@
// found in the LICENSE file.
#include "base/bind.h"
+#include "base/strings/stringprintf.h"
#include "content/browser/frame_host/navigation_controller_impl.h"
#include "content/browser/frame_host/navigation_entry_impl.h"
#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
#include "content/public/test/content_browser_test.h"
#include "content/public/test/content_browser_test_utils.h"
#include "content/shell/browser/shell.h"
@@ -22,7 +24,7 @@
const NavigationController& controller =
shell()->web_contents()->GetController();
- // load data. Blocks until it is done.
+ // Load data. Blocks until it is done.
content::LoadDataWithBaseURL(shell(), history_url, data, base_url);
// We should use history_url instead of the base_url as the original url of
@@ -30,5 +32,49 @@
// paths in the data, or enforcing same origin policy.
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,
+ DontIgnoreBackAfterNavEntryLimit) {
+ NavigationController& controller =
+ shell()->web_contents()->GetController();
+
+ const int kMaxEntryCount =
+ static_cast<int>(NavigationControllerImpl::max_entry_count());
+
+ // Load up to the max count, all entries should be there.
+ for (int url_index = 0; url_index < kMaxEntryCount; ++url_index) {
+ GURL url(base::StringPrintf("data:text/html,page%d", url_index));
+ EXPECT_TRUE(NavigateToURL(shell(), url));
+ }
+
+ EXPECT_EQ(controller.GetEntryCount(), kMaxEntryCount);
+
+ // Navigate twice more more.
+ for (int url_index = kMaxEntryCount;
+ url_index < kMaxEntryCount + 2; ++url_index) {
+ GURL url(base::StringPrintf("data:text/html,page%d", url_index));
+ EXPECT_TRUE(NavigateToURL(shell(), url));
+ }
+
+ // We expect http://www.a.com/0 and /1 to be gone.
+ EXPECT_EQ(kMaxEntryCount, controller.GetEntryCount());
+ EXPECT_EQ(GURL("data:text/html,page2"),
+ controller.GetEntryAtIndex(0)->GetURL());
+
+ // Now try to go back. This should not hang.
+ ASSERT_TRUE(controller.CanGoBack());
+ controller.GoBack();
+ EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+
+ // This should have successfully gone back.
+ EXPECT_EQ(GURL(base::StringPrintf("data:text/html,page%d", kMaxEntryCount)),
+ controller.GetLastCommittedEntry()->GetURL());
+}
+
} // namespace content
diff --git a/content/browser/frame_host/navigation_controller_impl_unittest.cc b/content/browser/frame_host/navigation_controller_impl_unittest.cc
index 0ca6ae59..3b0796a 100644
--- a/content/browser/frame_host/navigation_controller_impl_unittest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_unittest.cc
@@ -3415,9 +3415,7 @@
static_cast<TestWebContents*>(CreateTestWebContents()));
NavigationControllerImpl& other_controller = other_contents->GetController();
other_contents->NavigateAndCommit(url3);
- other_contents->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 2,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(2, 3);
other_controller.CopyStateFromAndPrune(&controller, false);
// other_controller should now contain the 3 urls: url1, url2 and url3.
@@ -3463,9 +3461,7 @@
static_cast<TestWebContents*>(CreateTestWebContents()));
NavigationControllerImpl& other_controller = other_contents->GetController();
other_contents->NavigateAndCommit(url3);
- other_contents->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 1,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(1, 2);
other_controller.CopyStateFromAndPrune(&controller, false);
// other_controller should now contain: url1, url3
@@ -3501,9 +3497,7 @@
NavigationControllerImpl& other_controller = other_contents->GetController();
other_contents->NavigateAndCommit(url3);
other_contents->NavigateAndCommit(url4);
- other_contents->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(1)), 2,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(2, 3);
other_controller.CopyStateFromAndPrune(&controller, false);
// other_controller should now contain: url1, url2, url4
@@ -3541,9 +3535,7 @@
other_contents->NavigateAndCommit(url4);
other_controller.GoBack();
other_contents->CommitPendingNavigation();
- other_contents->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 2,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(2, 3);
other_controller.CopyStateFromAndPrune(&controller, false);
// other_controller should now contain: url1, url2, url3
@@ -3582,9 +3574,7 @@
other_contents->NavigateAndCommit(url3);
other_controller.LoadURL(
url4, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
- other_contents->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 1,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(1, 2);
other_controller.CopyStateFromAndPrune(&controller, false);
// other_controller should now contain url1, url3, and a pending entry
@@ -3629,9 +3619,7 @@
other_controller.GetPendingEntry()->SetPageID(
other_controller.GetLastCommittedEntry()->GetPageID());
- other_contents->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 1,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(1, 2);
other_controller.CopyStateFromAndPrune(&controller, false);
// other_controller should now contain url1, url2a, and a pending entry
@@ -3673,9 +3661,7 @@
static_cast<TestWebContents*>(CreateTestWebContents()));
NavigationControllerImpl& other_controller = other_contents->GetController();
other_contents->NavigateAndCommit(url3);
- other_contents->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 2,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(2, 3);
other_controller.CopyStateFromAndPrune(&controller, false);
// other_controller should now contain: url1, url2, url3
@@ -3720,9 +3706,7 @@
static_cast<TestWebContents*>(CreateTestWebContents()));
NavigationControllerImpl& other_controller = other_contents->GetController();
other_contents->NavigateAndCommit(url4);
- other_contents->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 2,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(2, 3);
other_controller.CopyStateFromAndPrune(&controller, false);
// We should have received a pruned notification.
@@ -3771,9 +3755,7 @@
static_cast<TestWebContents*>(CreateTestWebContents()));
NavigationControllerImpl& other_controller = other_contents->GetController();
other_contents->NavigateAndCommit(url3);
- other_contents->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 1,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(1, 2);
other_controller.CopyStateFromAndPrune(&controller, true);
// other_controller should now contain the 2 urls: url1 and url3.
@@ -3825,9 +3807,7 @@
static_cast<TestWebContents*>(CreateTestWebContents()));
NavigationControllerImpl& other_controller = other_contents->GetController();
other_contents->NavigateAndCommit(url4);
- other_contents->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 2,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(2, 3);
other_controller.CopyStateFromAndPrune(&controller, true);
// We should have received no pruned notification.
@@ -3882,9 +3862,7 @@
// Load a page, then copy state from |source_contents|.
NavigateAndCommit(kInitialUrl);
- contents()->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(controller_impl().GetEntryAtIndex(0)), 2,
- controller_impl().GetEntryAtIndex(0)->GetPageID());
+ contents()->ExpectSetHistoryOffsetAndLength(2, 3);
controller_impl().CopyStateFromAndPrune(&source_controller, false);
ASSERT_EQ(3, controller_impl().GetEntryCount());
@@ -3960,9 +3938,7 @@
const GURL url1("http://foo1");
NavigateAndCommit(url1);
- contents()->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(controller.GetEntryAtIndex(0)), 0,
- controller.GetEntryAtIndex(0)->GetPageID());
+ contents()->ExpectSetHistoryOffsetAndLength(0, 1);
controller.PruneAllButLastCommitted();
@@ -3984,9 +3960,7 @@
controller.GoBack();
contents()->CommitPendingNavigation();
- contents()->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(controller.GetEntryAtIndex(0)), 0,
- controller.GetEntryAtIndex(0)->GetPageID());
+ contents()->ExpectSetHistoryOffsetAndLength(0, 1);
controller.PruneAllButLastCommitted();
@@ -4007,9 +3981,7 @@
controller.GoBack();
contents()->CommitPendingNavigation();
- contents()->ExpectSetHistoryLengthAndPrune(
- GetSiteInstanceFromEntry(controller.GetEntryAtIndex(1)), 0,
- controller.GetEntryAtIndex(1)->GetPageID());
+ contents()->ExpectSetHistoryOffsetAndLength(0, 1);
controller.PruneAllButLastCommitted();
@@ -4034,8 +4006,7 @@
EXPECT_TRUE(controller.GetPendingEntry());
EXPECT_EQ(2, controller.GetEntryCount());
- contents()->ExpectSetHistoryLengthAndPrune(
- NULL, 0, controller.GetPendingEntry()->GetPageID());
+ contents()->ExpectSetHistoryOffsetAndLength(0, 1);
controller.PruneAllButLastCommitted();
// We should only have the last committed and pending entries at this point,
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index 13bb3160..8807340a 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -1257,7 +1257,7 @@
// loading" message will be received asynchronously from the UI of the
// browser. But we want to keep the throbber in sync with what's happening
// in the UI. For example, we want to start throbbing immediately when the
- // user naivgates even if the renderer is delayed. There is also an issue
+ // user navigates even if the renderer is delayed. There is also an issue
// with the throbber starting because the WebUI (which controls whether the
// favicon is displayed) happens synchronously. If the start loading
// messages was asynchronous, then the default favicon would flash in.
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 9614814..ca9112a 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -1943,28 +1943,18 @@
DidDetachInterstitialPage());
}
-void WebContentsImpl::SetHistoryLengthAndPrune(
- const SiteInstance* site_instance,
- int history_length,
- int32 minimum_page_id) {
- // SetHistoryLengthAndPrune doesn't work when there are pending cross-site
- // navigations. Callers should ensure that this is the case.
- if (GetRenderManager()->pending_render_view_host()) {
- NOTREACHED();
- return;
- }
- RenderViewHostImpl* rvh = GetRenderViewHostImpl();
- if (!rvh) {
- NOTREACHED();
- return;
- }
- if (site_instance && rvh->GetSiteInstance() != site_instance) {
- NOTREACHED();
- return;
- }
- Send(new ViewMsg_SetHistoryLengthAndPrune(GetRoutingID(),
- history_length,
- minimum_page_id));
+void WebContentsImpl::SetHistoryOffsetAndLength(int history_offset,
+ int history_length) {
+ SetHistoryOffsetAndLengthForView(
+ GetRenderViewHost(), history_offset, history_length);
+}
+
+void WebContentsImpl::SetHistoryOffsetAndLengthForView(
+ RenderViewHost* render_view_host,
+ int history_offset,
+ int history_length) {
+ render_view_host->Send(new ViewMsg_SetHistoryOffsetAndLength(
+ render_view_host->GetRoutingID(), history_offset, history_length));
}
void WebContentsImpl::ReloadFocusedFrame(bool ignore_cache) {
@@ -4148,6 +4138,10 @@
return false;
}
+ SetHistoryOffsetAndLengthForView(render_view_host,
+ controller_.GetLastCommittedEntryIndex(),
+ controller_.GetEntryCount());
+
#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
// Force a ViewMsg_Resize to be sent, needed to make plugins show up on
// linux. See crbug.com/83941.
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index a747fe8..339865a6 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -611,13 +611,10 @@
bool NavigateToPendingEntry(
NavigationController::ReloadType reload_type) override;
- // Sets the history for this WebContentsImpl to |history_length| entries, and
- // moves the current page_id to the last entry in the list if it's valid.
- // This is mainly used when a prerendered page is swapped into the current
- // tab. The method is virtual for testing.
- void SetHistoryLengthAndPrune(const SiteInstance* site_instance,
- int merge_history_length,
- int32 minimum_page_id) override;
+ // Sets the history for this WebContentsImpl to |history_length| entries, with
+ // an offset of |history_offset|.
+ void SetHistoryOffsetAndLength(int history_offset,
+ int history_length) override;
// Called by InterstitialPageImpl when it creates a RenderFrameHost.
void RenderFrameForInterstitialPageCreated(
@@ -886,6 +883,12 @@
// Misc non-view stuff -------------------------------------------------------
+ // Sets the history for a specified RenderViewHost to |history_length|
+ // entries, with an offset of |history_offset|.
+ void SetHistoryOffsetAndLengthForView(RenderViewHost* render_view_host,
+ int history_offset,
+ int history_length);
+
// Helper functions for sending notifications.
void NotifyViewSwapped(RenderViewHost* old_host, RenderViewHost* new_host);
void NotifyFrameSwapped(RenderFrameHost* old_host, RenderFrameHost* new_host);
diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc
index a361553d..ba1a8f00 100644
--- a/content/browser/web_contents/web_contents_impl_unittest.cc
+++ b/content/browser/web_contents/web_contents_impl_unittest.cc
@@ -2282,10 +2282,7 @@
static_cast<TestWebContents*>(CreateTestWebContents()));
NavigationControllerImpl& other_controller = other_contents->GetController();
other_contents->NavigateAndCommit(url3);
- other_contents->ExpectSetHistoryLengthAndPrune(
- NavigationEntryImpl::FromNavigationEntry(
- other_controller.GetEntryAtIndex(0))->site_instance(), 1,
- other_controller.GetEntryAtIndex(0)->GetPageID());
+ other_contents->ExpectSetHistoryOffsetAndLength(1, 2);
other_controller.CopyStateFromAndPrune(&controller(), false);
// The merged controller should only have two entries: url1 and url2.
diff --git a/content/common/frame_messages.h b/content/common/frame_messages.h
index af8289f..e83664e 100644
--- a/content/common/frame_messages.h
+++ b/content/common/frame_messages.h
@@ -234,9 +234,8 @@
// FrameHostMsg_DidCommitProvisionalLoad message.
IPC_STRUCT_MEMBER(int32, page_id)
- // If page_id is -1, then pending_history_list_offset will also be -1.
- // Otherwise, it contains the offset into the history list corresponding to
- // the current navigation.
+ // For history navigations, this is the offset in the history list of the
+ // pending load. For non-history navigations, this will be ignored.
IPC_STRUCT_MEMBER(int, pending_history_list_offset)
// Informs the RenderView of where its current page contents reside in
diff --git a/content/common/view_messages.h b/content/common/view_messages.h
index 03fc70bd..10a1c1a 100644
--- a/content/common/view_messages.h
+++ b/content/common/view_messages.h
@@ -562,18 +562,12 @@
base::TimeTicks /* timebase */,
base::TimeDelta /* interval */)
-// Sent to the RenderView when a new tab is swapped into an existing
-// tab and the histories need to be merged. The existing tab has a history of
-// |merged_history_length| which precedes the history of the new tab. All
-// page_ids >= |minimum_page_id| in the new tab are appended to the history.
-//
-// For example, suppose the history of page_ids in the new tab's RenderView
-// is [4 7 8]. This is merged into an existing tab with 3 history items, and
-// all pages in the new tab with page_id >= 7 are to be preserved.
-// The resulting page history is [-1 -1 -1 7 8].
-IPC_MESSAGE_ROUTED2(ViewMsg_SetHistoryLengthAndPrune,
- int, /* merge_history_length */
- int32 /* minimum_page_id */)
+// Sent when the history is altered outside of navigation. The history list was
+// reset to |history_length| length, and the offset was reset to be
+// |history_offset|.
+IPC_MESSAGE_ROUTED2(ViewMsg_SetHistoryOffsetAndLength,
+ int /* history_offset */,
+ int /* history_length */)
// Tells the renderer to create a new view.
// This message is slightly different, the view it takes (via
diff --git a/content/public/test/render_view_test.cc b/content/public/test/render_view_test.cc
index b8358010..cc7d883e 100644
--- a/content/public/test/render_view_test.cc
+++ b/content/public/test/render_view_test.cc
@@ -337,14 +337,6 @@
impl->focusedNodeChanged(node);
}
-void RenderViewTest::ClearHistory() {
- RenderViewImpl* impl = static_cast<RenderViewImpl*>(view_);
- impl->page_id_ = -1;
- impl->history_list_offset_ = -1;
- impl->history_list_length_ = 0;
- impl->history_page_ids_.clear();
-}
-
void RenderViewTest::Reload(const GURL& url) {
FrameMsg_Navigate_Params params;
params.common_params.url = url;
@@ -422,14 +414,14 @@
int history_list_length = impl->historyBackListCount() +
impl->historyForwardListCount() + 1;
- int pending_offset = offset + impl->history_list_offset();
+ int pending_offset = offset + impl->history_list_offset_;
FrameMsg_Navigate_Params navigate_params;
navigate_params.common_params.navigation_type =
FrameMsg_Navigate_Type::NORMAL;
navigate_params.common_params.transition = ui::PAGE_TRANSITION_FORWARD_BACK;
navigate_params.current_history_list_length = history_list_length;
- navigate_params.current_history_list_offset = impl->history_list_offset();
+ navigate_params.current_history_list_offset = impl->history_list_offset_;
navigate_params.pending_history_list_offset = pending_offset;
navigate_params.page_id = impl->page_id_ + offset;
navigate_params.commit_params.page_state = state;
diff --git a/content/public/test/render_view_test.h b/content/public/test/render_view_test.h
index 46dc35ce1..67442d0 100644
--- a/content/public/test/render_view_test.h
+++ b/content/public/test/render_view_test.h
@@ -109,9 +109,6 @@
// Simulates |node| being focused.
void SetFocused(const blink::WebNode& node);
- // Clears anything associated with the browsing history.
- void ClearHistory();
-
// Simulates a navigation with a type of reload to the given url.
void Reload(const GURL& url);
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index f16b132..6a185c0 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -948,36 +948,26 @@
TRACE_EVENT2("navigation", "RenderFrameImpl::OnNavigate",
"id", routing_id_,
"url", params.common_params.url.possibly_invalid_spec());
+
bool is_reload =
RenderViewImpl::IsReload(params.common_params.navigation_type);
+ bool is_history_navigation = params.commit_params.page_state.IsValid();
WebURLRequest::CachePolicy cache_policy =
WebURLRequest::UseProtocolCachePolicy;
if (!RenderFrameImpl::PrepareRenderViewForNavigation(
params.common_params.url, params.common_params.navigation_type,
- params.commit_params.page_state, true, params.pending_history_list_offset,
- params.page_id, &is_reload, &cache_policy)) {
+ params.commit_params.page_state, true, is_history_navigation,
+ params.current_history_list_offset, params.page_id, &is_reload,
+ &cache_policy)) {
Send(new FrameHostMsg_DidDropNavigation(routing_id_));
return;
}
- int pending_history_list_offset = params.pending_history_list_offset;
- int current_history_list_offset = params.current_history_list_offset;
- int current_history_list_length = params.current_history_list_length;
+ render_view_->history_list_offset_ = params.current_history_list_offset;
+ render_view_->history_list_length_ = params.current_history_list_length;
if (params.should_clear_history_list) {
- CHECK_EQ(pending_history_list_offset, -1);
- CHECK_EQ(current_history_list_offset, -1);
- CHECK_EQ(current_history_list_length, 0);
- }
- render_view_->history_list_offset_ = current_history_list_offset;
- render_view_->history_list_length_ = current_history_list_length;
- if (render_view_->history_list_length_ >= 0) {
- render_view_->history_page_ids_.resize(
- render_view_->history_list_length_, -1);
- }
- if (pending_history_list_offset >= 0 &&
- pending_history_list_offset < render_view_->history_list_length_) {
- render_view_->history_page_ids_[pending_history_list_offset] =
- params.page_id;
+ CHECK_EQ(-1, render_view_->history_list_offset_);
+ CHECK_EQ(0, render_view_->history_list_length_);
}
GetContentClient()->SetActiveURL(params.common_params.url);
@@ -1015,7 +1005,7 @@
frame->reloadWithOverrideURL(params.common_params.url, true);
else
frame->reload(ignore_cache);
- } else if (params.commit_params.page_state.IsValid()) {
+ } else if (is_history_navigation) {
// We must know the page ID of the page we are navigating back to.
DCHECK_NE(params.page_id, -1);
scoped_ptr<HistoryEntry> entry =
@@ -2370,7 +2360,7 @@
// We bump our Page ID to correspond with the new session history entry.
render_view_->page_id_ = render_view_->next_page_id_++;
- // Don't update history_page_ids_ (etc) for kSwappedOutURL, since
+ // Don't update history list values for kSwappedOutURL, since
// we don't want to forget the entry that was there, and since we will
// never come back to kSwappedOutURL. Note that we have to call
// UpdateSessionHistory and update page_id_ even in this case, so that
@@ -2384,10 +2374,6 @@
render_view_->history_list_offset_ = kMaxSessionHistoryEntries - 1;
render_view_->history_list_length_ =
render_view_->history_list_offset_ + 1;
- render_view_->history_page_ids_.resize(
- render_view_->history_list_length_, -1);
- render_view_->history_page_ids_[render_view_->history_list_offset_] =
- render_view_->page_id_;
}
} else {
// Inspect the navigation_state on this frame to see if the navigation
@@ -2408,14 +2394,6 @@
render_view_->history_list_offset_ =
navigation_state->pending_history_list_offset();
-
- // If the history list is valid, our list of page IDs should be correct.
- DCHECK(render_view_->history_list_length_ <= 0 ||
- render_view_->history_list_offset_ < 0 ||
- render_view_->history_list_offset_ >=
- render_view_->history_list_length_ ||
- render_view_->history_page_ids_[render_view_->history_list_offset_]
- == render_view_->page_id_);
}
}
@@ -3712,11 +3690,14 @@
CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableBrowserSideNavigation));
bool is_reload = false;
+ bool is_history_navigation = commit_params.page_state.IsValid();
WebURLRequest::CachePolicy cache_policy =
WebURLRequest::UseProtocolCachePolicy;
if (!RenderFrameImpl::PrepareRenderViewForNavigation(
common_params.url, common_params.navigation_type,
- commit_params.page_state, false, -1, -1, &is_reload, &cache_policy)) {
+ commit_params.page_state, false /* check_for_stale_navigation */,
+ is_history_navigation, -1 /* current_history_list_offset; TODO(clamy)*/,
+ -1, &is_reload, &cache_policy)) {
return;
}
@@ -4138,8 +4119,9 @@
const GURL& url,
FrameMsg_Navigate_Type::Value navigate_type,
const PageState& state,
- bool check_history,
- int pending_history_list_offset,
+ bool check_for_stale_navigation,
+ bool is_history_navigation,
+ int current_history_list_offset,
int32 page_id,
bool* is_reload,
WebURLRequest::CachePolicy* cache_policy) {
@@ -4151,10 +4133,14 @@
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.
- if (check_history && render_view_->IsBackForwardToStaleEntry(
- state, pending_history_list_offset, page_id, *is_reload))
+ // 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.
+ // TODO(clamy): remove check_for_stale_navigation
+ if (check_for_stale_navigation &&
+ !render_view_->is_swapped_out() && is_history_navigation &&
+ render_view_->history_list_offset_ != current_history_list_offset) {
return false;
+ }
if (!is_swapped_out_ || frame_->parent())
return true;
diff --git a/content/renderer/render_frame_impl.h b/content/renderer/render_frame_impl.h
index 7be406d..ab9645d 100644
--- a/content/renderer/render_frame_impl.h
+++ b/content/renderer/render_frame_impl.h
@@ -662,8 +662,9 @@
const GURL& url,
FrameMsg_Navigate_Type::Value navigate_type,
const PageState& state,
- bool check_history,
- int pending_history_list_offset,
+ bool check_for_stale_navigation,
+ bool is_history_navigation,
+ int current_history_list_offset,
int32 page_id,
bool* is_reload,
blink::WebURLRequest::CachePolicy* cache_policy);
diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc
index c4665ff..386b4d8bd 100644
--- a/content/renderer/render_view_browsertest.cc
+++ b/content/renderer/render_view_browsertest.cc
@@ -798,21 +798,18 @@
EXPECT_EQ(state_C, state);
}
-// Test that the history_page_ids_ list can reveal when a stale back/forward
-// navigation arrives from the browser and can be ignored. See
-// http://crbug.com/86758.
+// 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("<div>Page A</div>");
EXPECT_EQ(1, view()->history_list_length_);
EXPECT_EQ(0, view()->history_list_offset_);
- EXPECT_EQ(1, view()->history_page_ids_[0]);
// Load page B, which will trigger an UpdateState message for page A.
LoadHTML("<div>Page B</div>");
EXPECT_EQ(2, view()->history_list_length_);
EXPECT_EQ(1, view()->history_list_offset_);
- EXPECT_EQ(2, view()->history_page_ids_[1]);
// Check for a valid UpdateState message for page A.
ProcessPendingMessages();
@@ -844,7 +841,7 @@
LoadHTML("<div>Page C</div>");
EXPECT_EQ(2, view()->history_list_length_);
EXPECT_EQ(1, view()->history_list_offset_);
- EXPECT_EQ(3, view()->history_page_ids_[1]);
+ EXPECT_EQ(3, view()->page_id_); // page C is now page id 3
// The browser then sends a stale navigation to B, which should be ignored.
FrameMsg_Navigate_Params params_B;
@@ -863,7 +860,7 @@
// State should be unchanged.
EXPECT_EQ(2, view()->history_list_length_);
EXPECT_EQ(1, view()->history_list_offset_);
- EXPECT_EQ(3, view()->history_page_ids_[1]);
+ EXPECT_EQ(3, view()->page_id_); // page C, not page B
// Check for a valid DidDropNavigation message.
ProcessPendingMessages();
@@ -873,75 +870,6 @@
render_thread_->sink().ClearMessages();
}
-// Test that we do not ignore navigations after the entry limit is reached,
-// in which case the browser starts dropping entries from the front. In this
-// case, we'll see a page_id mismatch but the RenderView's id will be older,
-// not newer, than params.page_id. Use this as a cue that we should update the
-// state and not treat it like a navigation to a cropped forward history item.
-// See http://crbug.com/89798.
-TEST_F(RenderViewImplTest, DontIgnoreBackAfterNavEntryLimit) {
- // Load page A.
- LoadHTML("<div>Page A</div>");
- EXPECT_EQ(1, view()->history_list_length_);
- EXPECT_EQ(0, view()->history_list_offset_);
- EXPECT_EQ(1, view()->history_page_ids_[0]);
-
- // Load page B, which will trigger an UpdateState message for page A.
- LoadHTML("<div>Page B</div>");
- EXPECT_EQ(2, view()->history_list_length_);
- EXPECT_EQ(1, view()->history_list_offset_);
- EXPECT_EQ(2, view()->history_page_ids_[1]);
-
- // 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 = param.a;
- PageState state_A = param.b;
- EXPECT_EQ(1, page_id_A);
- render_thread_->sink().ClearMessages();
-
- // Load page C, which will trigger an UpdateState message for page B.
- LoadHTML("<div>Page C</div>");
- EXPECT_EQ(3, view()->history_list_length_);
- EXPECT_EQ(2, view()->history_list_offset_);
- EXPECT_EQ(3, view()->history_page_ids_[2]);
-
- // Check for a valid UpdateState message for page B.
- ProcessPendingMessages();
- const IPC::Message* msg_B = render_thread_->sink().GetUniqueMessageMatching(
- ViewHostMsg_UpdateState::ID);
- ASSERT_TRUE(msg_B);
- ViewHostMsg_UpdateState::Read(msg_B, ¶m);
- int page_id_B = param.a;
- PageState state_B = param.b;
- EXPECT_EQ(2, page_id_B);
- render_thread_->sink().ClearMessages();
-
- // Suppose the browser has limited the number of NavigationEntries to 2.
- // It has now dropped the first entry, but the renderer isn't notified.
- // Ensure that going back to page B (page_id 2) at offset 0 is successful.
- FrameMsg_Navigate_Params params_B;
- params_B.common_params.navigation_type = FrameMsg_Navigate_Type::NORMAL;
- params_B.common_params.transition = ui::PAGE_TRANSITION_FORWARD_BACK;
- params_B.current_history_list_length = 2;
- params_B.current_history_list_offset = 1;
- params_B.pending_history_list_offset = 0;
- params_B.page_id = 2;
- params_B.commit_params.page_state = state_B;
- params_B.commit_params.browser_navigation_start =
- base::TimeTicks::FromInternalValue(1);
- frame()->OnNavigate(params_B);
- ProcessPendingMessages();
-
- EXPECT_EQ(2, view()->history_list_length_);
- EXPECT_EQ(0, view()->history_list_offset_);
- EXPECT_EQ(2, view()->history_page_ids_[0]);
-}
-
// Test that our IME backend sends a notification message when the input focus
// changes.
TEST_F(RenderViewImplTest, OnImeTypeChanged) {
@@ -1684,203 +1612,16 @@
EXPECT_EQ(invalid_gurl, view()->target_url_);
}
-TEST_F(RenderViewImplTest, SetHistoryLengthAndPrune) {
- int expected_page_id = -1;
-
- // No history to merge and no committed pages.
- view()->OnSetHistoryLengthAndPrune(0, -1);
- EXPECT_EQ(0, view()->history_list_length_);
- EXPECT_EQ(-1, view()->history_list_offset_);
-
- // History to merge and no committed pages.
- view()->OnSetHistoryLengthAndPrune(2, -1);
- EXPECT_EQ(2, view()->history_list_length_);
- EXPECT_EQ(1, view()->history_list_offset_);
- EXPECT_EQ(-1, view()->history_page_ids_[0]);
- EXPECT_EQ(-1, view()->history_page_ids_[1]);
- ClearHistory();
-
- blink::WebHistoryItem item;
- item.initialize();
-
- // No history to merge and a committed page to be kept.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- view()->OnSetHistoryLengthAndPrune(0, expected_page_id);
+TEST_F(RenderViewImplTest, SetHistoryLengthAndOffset) {
+ // No history to merge; one committed page.
+ view()->OnSetHistoryOffsetAndLength(0, 1);
EXPECT_EQ(1, view()->history_list_length_);
EXPECT_EQ(0, view()->history_list_offset_);
- EXPECT_EQ(expected_page_id, view()->history_page_ids_[0]);
- ClearHistory();
- // No history to merge and a committed page to be pruned.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- view()->OnSetHistoryLengthAndPrune(0, expected_page_id + 1);
- EXPECT_EQ(0, view()->history_list_length_);
- EXPECT_EQ(-1, view()->history_list_offset_);
- ClearHistory();
-
- // No history to merge and a committed page that the browser was unaware of.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- view()->OnSetHistoryLengthAndPrune(0, -1);
- EXPECT_EQ(1, view()->history_list_length_);
- EXPECT_EQ(0, view()->history_list_offset_);
- EXPECT_EQ(expected_page_id, view()->history_page_ids_[0]);
- ClearHistory();
-
- // History to merge and a committed page to be kept.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- view()->OnSetHistoryLengthAndPrune(2, expected_page_id);
- EXPECT_EQ(3, view()->history_list_length_);
- EXPECT_EQ(2, view()->history_list_offset_);
- EXPECT_EQ(-1, view()->history_page_ids_[0]);
- EXPECT_EQ(-1, view()->history_page_ids_[1]);
- EXPECT_EQ(expected_page_id, view()->history_page_ids_[2]);
- ClearHistory();
-
- // History to merge and a committed page to be pruned.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- view()->OnSetHistoryLengthAndPrune(2, expected_page_id + 1);
+ // History of length 1 to merge; one committed page.
+ view()->OnSetHistoryOffsetAndLength(1, 2);
EXPECT_EQ(2, view()->history_list_length_);
EXPECT_EQ(1, view()->history_list_offset_);
- EXPECT_EQ(-1, view()->history_page_ids_[0]);
- EXPECT_EQ(-1, view()->history_page_ids_[1]);
- ClearHistory();
-
- // History to merge and a committed page that the browser was unaware of.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- view()->OnSetHistoryLengthAndPrune(2, -1);
- EXPECT_EQ(3, view()->history_list_length_);
- EXPECT_EQ(2, view()->history_list_offset_);
- EXPECT_EQ(-1, view()->history_page_ids_[0]);
- EXPECT_EQ(-1, view()->history_page_ids_[1]);
- EXPECT_EQ(expected_page_id, view()->history_page_ids_[2]);
- ClearHistory();
-
- int expected_page_id_2 = -1;
-
- // No history to merge and two committed pages, both to be kept.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id_2 = view()->page_id_;
- EXPECT_GT(expected_page_id_2, expected_page_id);
- view()->OnSetHistoryLengthAndPrune(0, expected_page_id);
- EXPECT_EQ(2, view()->history_list_length_);
- EXPECT_EQ(1, view()->history_list_offset_);
- EXPECT_EQ(expected_page_id, view()->history_page_ids_[0]);
- EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[1]);
- ClearHistory();
-
- // No history to merge and two committed pages, and only the second is kept.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id_2 = view()->page_id_;
- EXPECT_GT(expected_page_id_2, expected_page_id);
- view()->OnSetHistoryLengthAndPrune(0, expected_page_id_2);
- EXPECT_EQ(1, view()->history_list_length_);
- EXPECT_EQ(0, view()->history_list_offset_);
- EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[0]);
- ClearHistory();
-
- // No history to merge and two committed pages, both of which the browser was
- // unaware of.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id_2 = view()->page_id_;
- EXPECT_GT(expected_page_id_2, expected_page_id);
- view()->OnSetHistoryLengthAndPrune(0, -1);
- EXPECT_EQ(2, view()->history_list_length_);
- EXPECT_EQ(1, view()->history_list_offset_);
- EXPECT_EQ(expected_page_id, view()->history_page_ids_[0]);
- EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[1]);
- ClearHistory();
-
- // History to merge and two committed pages, both to be kept.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id_2 = view()->page_id_;
- EXPECT_GT(expected_page_id_2, expected_page_id);
- view()->OnSetHistoryLengthAndPrune(2, expected_page_id);
- EXPECT_EQ(4, view()->history_list_length_);
- EXPECT_EQ(3, view()->history_list_offset_);
- EXPECT_EQ(-1, view()->history_page_ids_[0]);
- EXPECT_EQ(-1, view()->history_page_ids_[1]);
- EXPECT_EQ(expected_page_id, view()->history_page_ids_[2]);
- EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[3]);
- ClearHistory();
-
- // History to merge and two committed pages, and only the second is kept.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id_2 = view()->page_id_;
- EXPECT_GT(expected_page_id_2, expected_page_id);
- view()->OnSetHistoryLengthAndPrune(2, expected_page_id_2);
- EXPECT_EQ(3, view()->history_list_length_);
- EXPECT_EQ(2, view()->history_list_offset_);
- EXPECT_EQ(-1, view()->history_page_ids_[0]);
- EXPECT_EQ(-1, view()->history_page_ids_[1]);
- EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[2]);
- ClearHistory();
-
- // History to merge and two committed pages, both of which the browser was
- // unaware of.
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id = view()->page_id_;
- frame()->didCommitProvisionalLoad(GetMainFrame(),
- item,
- blink::WebStandardCommit);
- expected_page_id_2 = view()->page_id_;
- EXPECT_GT(expected_page_id_2, expected_page_id);
- view()->OnSetHistoryLengthAndPrune(2, -1);
- EXPECT_EQ(4, view()->history_list_length_);
- EXPECT_EQ(3, view()->history_list_offset_);
- EXPECT_EQ(-1, view()->history_page_ids_[0]);
- EXPECT_EQ(-1, view()->history_page_ids_[1]);
- EXPECT_EQ(expected_page_id, view()->history_page_ids_[2]);
- EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[3]);
}
TEST_F(RenderViewImplTest, ContextMenu) {
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index 6291019f..a4292e94 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -848,8 +848,6 @@
it != disambiguation_bitmaps_.end();
++it)
delete it->second;
- history_page_ids_.clear();
-
base::debug::TraceLog::GetInstance()->RemoveProcessLabel(routing_id_);
// If file chooser is still waiting for answer, dispatch empty answer.
@@ -1355,8 +1353,8 @@
OnGetSerializedHtmlDataForCurrentPageWithLocalLinks)
IPC_MESSAGE_HANDLER(ViewMsg_ShowContextMenu, OnShowContextMenu)
// TODO(viettrungluu): Move to a separate message filter.
- IPC_MESSAGE_HANDLER(ViewMsg_SetHistoryLengthAndPrune,
- OnSetHistoryLengthAndPrune)
+ IPC_MESSAGE_HANDLER(ViewMsg_SetHistoryOffsetAndLength,
+ OnSetHistoryOffsetAndLength)
IPC_MESSAGE_HANDLER(ViewMsg_EnableViewSourceMode, OnEnableViewSourceMode)
IPC_MESSAGE_HANDLER(ViewMsg_ReleaseDisambiguationPopupBitmap,
OnReleaseDisambiguationPopupBitmap)
@@ -1399,44 +1397,6 @@
handling_input_event_ = false;
}
-bool RenderViewImpl::IsBackForwardToStaleEntry(const PageState& state,
- int pending_history_list_offset,
- int32 page_id,
- bool is_reload) {
- // Make sure this isn't a back/forward to an entry we have already cropped
- // or replaced from our history, before the browser knew about it. If so,
- // a new navigation has committed in the mean time, and we can ignore this.
- bool is_back_forward = !is_reload && state.IsValid();
-
- // Note: if the history_list_length_ is 0 for a back/forward, we must be
- // restoring from a previous session. We'll update our state in OnNavigate.
- if (!is_back_forward || history_list_length_ <= 0)
- return false;
-
- DCHECK_EQ(static_cast<int>(history_page_ids_.size()), history_list_length_);
-
- // Check for whether the forward history has been cropped due to a recent
- // navigation the browser didn't know about.
- if (pending_history_list_offset >= history_list_length_)
- return true;
-
- // Check for whether this entry has been replaced with a new one.
- int expected_page_id =
- history_page_ids_[pending_history_list_offset];
- if (expected_page_id > 0 && page_id != expected_page_id) {
- if (page_id < expected_page_id)
- return true;
-
- // Otherwise we've removed an earlier entry and should have shifted all
- // entries left. For now, it's ok to lazily update the list.
- // TODO(creis): Notify all live renderers when we remove entries from
- // the front of the list, so that we don't hit this case.
- history_page_ids_[pending_history_list_offset] = page_id;
- }
-
- return false;
-}
-
void RenderViewImpl::OnCopyImageAt(int x, int y) {
webview()->copyImageAt(WebPoint(x, y));
}
@@ -1491,27 +1451,15 @@
edit_commands_ = edit_commands;
}
-void RenderViewImpl::OnSetHistoryLengthAndPrune(int history_length,
- int32 minimum_page_id) {
+void RenderViewImpl::OnSetHistoryOffsetAndLength(int history_offset,
+ int history_length) {
+ DCHECK_GE(history_offset, -1);
DCHECK_GE(history_length, 0);
- DCHECK(history_list_offset_ == history_list_length_ - 1);
- DCHECK_GE(minimum_page_id, -1);
- // Generate the new list.
- std::vector<int32> new_history_page_ids(history_length, -1);
- for (size_t i = 0; i < history_page_ids_.size(); ++i) {
- if (minimum_page_id >= 0 && history_page_ids_[i] < minimum_page_id)
- continue;
- new_history_page_ids.push_back(history_page_ids_[i]);
- }
- new_history_page_ids.swap(history_page_ids_);
-
- // Update indexes.
- history_list_length_ = history_page_ids_.size();
- history_list_offset_ = history_list_length_ - 1;
+ history_list_offset_ = history_offset;
+ history_list_length_ = history_length;
}
-
void RenderViewImpl::OnSetInitialFocus(bool reverse) {
if (!webview())
return;
diff --git a/content/renderer/render_view_impl.h b/content/renderer/render_view_impl.h
index 7b9ca36..23b1a0a 100644
--- a/content/renderer/render_view_impl.h
+++ b/content/renderer/render_view_impl.h
@@ -184,8 +184,6 @@
// May return NULL when the view is closing.
blink::WebView* webview() const;
- int history_list_offset() const { return history_list_offset_; }
-
const WebPreferences& webkit_preferences() const {
return webkit_preferences_;
}
@@ -549,8 +547,6 @@
DidFailProvisionalLoadWithErrorForError);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest,
DidFailProvisionalLoadWithErrorForCancellation);
- FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest,
- DontIgnoreBackAfterNavEntryLimit);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, ImeComposition);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, InsertCharacters);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, JSBlockSentAfterPageLoad);
@@ -563,6 +559,8 @@
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest,
SetEditableSelectionAndComposition);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, StaleNavigationsIgnored);
+ FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest,
+ DontIgnoreBackAfterNavEntryLimit);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, UpdateTargetURLWithInvalidURL);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest,
GetCompositionCharacterBoundsTest);
@@ -572,7 +570,7 @@
#if defined(OS_MACOSX)
FRIEND_TEST_ALL_PREFIXES(RenderViewTest, MacTestCmdUp);
#endif
- FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, SetHistoryLengthAndPrune);
+ FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, SetHistoryLengthAndOffset);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, ZoomLimit);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, NavigateFrame);
FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, BasicRenderFrame);
@@ -683,7 +681,7 @@
void OnSetActive(bool active);
void OnSetBackgroundOpaque(bool opaque);
void OnExitFullscreen();
- void OnSetHistoryLengthAndPrune(int history_length, int32 minimum_page_id);
+ void OnSetHistoryOffsetAndLength(int history_offset, int history_length);
void OnSetInitialFocus(bool reverse);
void OnSetPageEncoding(const std::string& encoding_name);
void OnSetRendererPrefs(const RendererPreferences& renderer_prefs);
@@ -730,12 +728,6 @@
// Returns NULL if there is no such WebPlugin.
blink::WebPlugin* GetWebPluginForFind();
- // Returns true if the |params| navigation is to an entry that has been
- // cropped due to a recent navigation the browser did not know about.
- bool IsBackForwardToStaleEntry(const PageState& state,
- int pending_history_list_offset,
- int32 page_id,
- bool is_reload);
// If we initiated a navigation, this function will populate |document_state|
// with the navigation information saved in OnNavigate().
@@ -902,12 +894,6 @@
// are gone.
int frames_in_progress_;
- // The list of page IDs for each history item this RenderView knows about.
- // Some entries may be -1 if they were rendered by other processes or were
- // restored from a previous session. This lets us detect attempts to
- // navigate to stale entries that have been cropped from our history.
- std::vector<int32> history_page_ids_;
-
// UI state ------------------------------------------------------------------
// The state of our target_url transmissions. When we receive a request to
diff --git a/content/test/test_web_contents.cc b/content/test/test_web_contents.cc
index 5d46f61a..e4f4846f 100644
--- a/content/test/test_web_contents.cc
+++ b/content/test/test_web_contents.cc
@@ -33,10 +33,8 @@
TestWebContents::TestWebContents(BrowserContext* browser_context)
: WebContentsImpl(browser_context, NULL),
delegate_view_override_(NULL),
- expect_set_history_length_and_prune_(false),
- expect_set_history_length_and_prune_site_instance_(NULL),
- expect_set_history_length_and_prune_history_length_(0),
- expect_set_history_length_and_prune_min_page_id_(-1) {
+ expect_set_history_offset_and_length_(false),
+ expect_set_history_offset_and_length_history_length_(0) {
}
TestWebContents* TestWebContents::Create(BrowserContext* browser_context,
@@ -47,7 +45,7 @@
}
TestWebContents::~TestWebContents() {
- EXPECT_FALSE(expect_set_history_length_and_prune_);
+ EXPECT_FALSE(expect_set_history_offset_and_length_);
}
TestRenderFrameHost* TestWebContents::GetMainFrame() {
@@ -217,27 +215,21 @@
AddDestructionObserver(contents);
}
-void TestWebContents::ExpectSetHistoryLengthAndPrune(
- const SiteInstance* site_instance,
- int history_length,
- int32 min_page_id) {
- expect_set_history_length_and_prune_ = true;
- expect_set_history_length_and_prune_site_instance_ =
- static_cast<const SiteInstanceImpl*>(site_instance);
- expect_set_history_length_and_prune_history_length_ = history_length;
- expect_set_history_length_and_prune_min_page_id_ = min_page_id;
+void TestWebContents::ExpectSetHistoryOffsetAndLength(int history_offset,
+ int history_length) {
+ expect_set_history_offset_and_length_ = true;
+ expect_set_history_offset_and_length_history_offset_ = history_offset;
+ expect_set_history_offset_and_length_history_length_ = history_length;
}
-void TestWebContents::SetHistoryLengthAndPrune(
- const SiteInstance* site_instance, int history_length,
- int32 min_page_id) {
- EXPECT_TRUE(expect_set_history_length_and_prune_);
- expect_set_history_length_and_prune_ = false;
- EXPECT_EQ(expect_set_history_length_and_prune_site_instance_.get(),
- site_instance);
- EXPECT_EQ(expect_set_history_length_and_prune_history_length_,
+void TestWebContents::SetHistoryOffsetAndLength(int history_offset,
+ int history_length) {
+ EXPECT_TRUE(expect_set_history_offset_and_length_);
+ expect_set_history_offset_and_length_ = false;
+ EXPECT_EQ(expect_set_history_offset_and_length_history_offset_,
+ history_offset);
+ EXPECT_EQ(expect_set_history_offset_and_length_history_length_,
history_length);
- EXPECT_EQ(expect_set_history_length_and_prune_min_page_id_, min_page_id);
}
void TestWebContents::TestDidFinishLoad(const GURL& url) {
diff --git a/content/test/test_web_contents.h b/content/test/test_web_contents.h
index 83ce185..8e3c2b5 100644
--- a/content/test/test_web_contents.h
+++ b/content/test/test_web_contents.h
@@ -78,18 +78,16 @@
// Allows us to simulate that a contents was created via CreateNewWindow.
void AddPendingContents(TestWebContents* contents);
- // Establish expected arguments for |SetHistoryLengthAndPrune()|. When
- // |SetHistoryLengthAndPrune()| is called, the arguments are compared
+ // Establish expected arguments for |SetHistoryOffsetAndLength()|. When
+ // |SetHistoryOffsetAndLength()| is called, the arguments are compared
// with the expected arguments specified here.
- void ExpectSetHistoryLengthAndPrune(const SiteInstance* site_instance,
- int history_length,
- int32 min_page_id);
+ void ExpectSetHistoryOffsetAndLength(int history_offset,
+ int history_length);
// Compares the arguments passed in with the expected arguments passed in
- // to |ExpectSetHistoryLengthAndPrune()|.
- void SetHistoryLengthAndPrune(const SiteInstance* site_instance,
- int history_length,
- int32 min_page_id) override;
+ // to |ExpectSetHistoryOffsetAndLength()|.
+ void SetHistoryOffsetAndLength(int history_offset,
+ int history_length) override;
void TestDidFinishLoad(const GURL& url);
void TestDidFailLoadWithError(const GURL& url,
@@ -121,12 +119,10 @@
RenderViewHostDelegateView* delegate_view_override_;
- // Expectations for arguments of |SetHistoryLengthAndPrune()|.
- bool expect_set_history_length_and_prune_;
- scoped_refptr<const SiteInstanceImpl>
- expect_set_history_length_and_prune_site_instance_;
- int expect_set_history_length_and_prune_history_length_;
- int32 expect_set_history_length_and_prune_min_page_id_;
+ // Expectations for arguments of |SetHistoryOffsetAndLength()|.
+ bool expect_set_history_offset_and_length_;
+ int expect_set_history_offset_and_length_history_offset_;
+ int expect_set_history_offset_and_length_history_length_;
};
} // namespace content
