diff options
author | creis <creis@chromium.org> | 2015-05-27 18:28:39 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-05-28 01:29:04 +0000 |
commit | 62109d9af7c8db2c1bcaef715f04ca92b4d59746 (patch) | |
tree | 88edf0da823ce773aceea96ade0d3ed18aa2e9a2 | |
parent | 52b220282d8fa458ca7129bb259f319968be7053 (diff) | |
download | chromium_src-62109d9af7c8db2c1bcaef715f04ca92b4d59746.zip chromium_src-62109d9af7c8db2c1bcaef715f04ca92b4d59746.tar.gz chromium_src-62109d9af7c8db2c1bcaef715f04ca92b4d59746.tar.bz2 |
Fix the commit type for out-of-process iframes.
Passes a has_committed_real_load param to the renderer to let the
(possibly new) FrameLoader know if there have been commits in that
frame in the past, perhaps in another process.
Also fixes broken invariants in NavigationController and tests.
Only affects --site-per-process mode.
BUG=464014
TEST=Navigation tests still pass.
Review URL: https://codereview.chromium.org/1148953014
Cr-Commit-Position: refs/heads/master@{#331712}
-rw-r--r-- | content/browser/frame_host/navigation_controller_impl.cc | 46 | ||||
-rw-r--r-- | content/browser/frame_host/navigation_controller_impl.h | 6 | ||||
-rw-r--r-- | content/browser/frame_host/navigation_controller_impl_browsertest.cc | 266 | ||||
-rw-r--r-- | content/browser/frame_host/navigation_entry_impl.cc | 74 | ||||
-rw-r--r-- | content/browser/frame_host/navigation_entry_impl.h | 14 | ||||
-rw-r--r-- | content/browser/frame_host/navigation_request.cc | 3 | ||||
-rw-r--r-- | content/browser/frame_host/navigator_impl.cc | 1 | ||||
-rw-r--r-- | content/common/frame_messages.h | 1 | ||||
-rw-r--r-- | content/common/navigation_params.cc | 3 | ||||
-rw-r--r-- | content/common/navigation_params.h | 7 | ||||
-rw-r--r-- | content/renderer/render_frame_impl.cc | 7 |
11 files changed, 364 insertions, 64 deletions
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc index 5ee44552..b03a66c 100644 --- a/content/browser/frame_host/navigation_controller_impl.cc +++ b/content/browser/frame_host/navigation_controller_impl.cc @@ -421,6 +421,12 @@ NavigationEntryImpl* NavigationControllerImpl::GetEntryWithPageID( return (index != -1) ? entries_[index].get() : NULL; } +bool NavigationControllerImpl::HasCommittedRealLoad( + FrameTreeNode* frame_tree_node) const { + NavigationEntryImpl* last_committed = GetLastCommittedEntry(); + return last_committed && last_committed->HasFrameEntry(frame_tree_node); +} + void NavigationControllerImpl::LoadEntry(NavigationEntryImpl* entry) { // When navigating to a new page, we don't know for sure if we will actually // end up leaving the current page. The new page load could for example @@ -833,13 +839,10 @@ bool NavigationControllerImpl::RendererDidNavigate( new_type == NAVIGATION_TYPE_EXISTING_PAGE; if (base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kSitePerProcess)) { - // In site-per-process mode, the new classifier sometimes correctly - // classifies things as auto-subframe where the old classifier incorrectly - // ignored them or called them NEW_SUBFRAME. - ignore_mismatch |= details->type == NAVIGATION_TYPE_NAV_IGNORE && - new_type == NAVIGATION_TYPE_AUTO_SUBFRAME; - ignore_mismatch |= details->type == NAVIGATION_TYPE_NEW_SUBFRAME && - new_type == NAVIGATION_TYPE_AUTO_SUBFRAME; + // We know that the old classifier is wrong for OOPIFs, so use the new one + // in --site-per-process mode. + details->type = new_type; + ignore_mismatch = true; } if (!ignore_mismatch) { DCHECK_EQ(details->type, new_type); @@ -943,10 +946,6 @@ bool NavigationControllerImpl::RendererDidNavigate( NavigationType NavigationControllerImpl::ClassifyNavigation( RenderFrameHostImpl* rfh, const FrameHostMsg_DidCommitProvisionalLoad_Params& params) const { - // Hack; remove once http://crbug.com/464014 is fixed. - if (rfh->IsCrossProcessSubframe()) - return NAVIGATION_TYPE_NEW_SUBFRAME; - if (params.page_id == -1) { // The renderer generates the page IDs, and so if it gives us the invalid // page ID (-1) we know it didn't actually navigate. This happens in a few @@ -1096,10 +1095,6 @@ NavigationType NavigationControllerImpl::ClassifyNavigation( NavigationType NavigationControllerImpl::ClassifyNavigationWithoutPageID( RenderFrameHostImpl* rfh, const FrameHostMsg_DidCommitProvisionalLoad_Params& params) const { - // Hack; remove once http://crbug.com/464014 is fixed. - if (rfh->IsCrossProcessSubframe()) - return NAVIGATION_TYPE_NEW_SUBFRAME; - if (params.did_create_new_entry) { // A new entry. We may or may not have a pending entry for the page, and // this may or may not be the main frame. @@ -1450,18 +1445,13 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe( DCHECK(GetLastCommittedEntry()); if (params.nav_entry_id) { - // If the |nav_entry_id| is non-zero, this is a browser-initiated navigation - // and is thus a "history auto" navigation. TODO(creis): Implement - // back/forward this way for site-per-process. - int entry_index = GetEntryIndexWithUniqueID(params.nav_entry_id); - if (entry_index == -1) { - NOTREACHED(); - return false; - } - // Update the current navigation entry in case we're going back/forward. - if (entry_index != last_committed_entry_index_) { + // If the |nav_entry_id| is non-zero and matches an existing entry, this is + // a history auto" navigation. Update the last committed index accordingly. + // If we don't recognize the |nav_entry_id|, it might be either a pending + // entry for a transfer or a recently pruned entry. We'll handle it below. + if (entry_index != -1 && entry_index != last_committed_entry_index_) { // Make sure that a subframe commit isn't changing the main frame's // origin. Otherwise the renderer process may be confused, leading to a // URL spoof. We can't check the path since that may change @@ -1474,6 +1464,8 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe( // kill the renderer process with bad_message::NC_AUTO_SUBFRAME. NOTREACHED() << "Unexpected main frame origin change on AUTO_SUBFRAME."; } + + // TODO(creis): Update the FrameNavigationEntry in --site-per-process. last_committed_entry_index_ = entry_index; DiscardNonCommittedEntriesInternal(); return true; @@ -1488,6 +1480,10 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe( last_committed->AddOrUpdateFrameEntry(rfh->frame_tree_node(), rfh->GetSiteInstance(), params.url, params.referrer); + + // Cross-process subframe navigations may leave a pending entry around. + // TODO(creis): Don't use pending entries for subframe navigations. + DiscardNonCommittedEntriesInternal(); } // We do not need to discard the pending entry in this case, since we will diff --git a/content/browser/frame_host/navigation_controller_impl.h b/content/browser/frame_host/navigation_controller_impl.h index ee00ca4..3a38bd0 100644 --- a/content/browser/frame_host/navigation_controller_impl.h +++ b/content/browser/frame_host/navigation_controller_impl.h @@ -20,6 +20,7 @@ struct FrameHostMsg_DidCommitProvisionalLoad_Params; namespace content { +class FrameTreeNode; class RenderFrameHostImpl; class NavigationEntryScreenshotManager; class SiteInstance; @@ -116,6 +117,11 @@ class CONTENT_EXPORT NavigationControllerImpl SiteInstance* instance, int32 page_id) const; + // Whether the given frame has committed any navigations yet. + // This currently only returns true in --site-per-process mode. + // TODO(creis): Create FrameNavigationEntries by default so this always works. + bool HasCommittedRealLoad(FrameTreeNode* frame_tree_node) const; + NavigationControllerDelegate* delegate() const { return delegate_; } diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc index 3f371bf..a1bfd03 100644 --- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc +++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc @@ -374,12 +374,25 @@ class LoadCommittedCapturer : public WebContentsObserver { private: void RenderFrameCreated(RenderFrameHost* render_frame_host) override { - // If this object was created with a specified tree frame node, there - // shouldn't be any frames being created. - DCHECK_EQ(0, frame_tree_node_id_); RenderFrameHostImpl* rfh = static_cast<RenderFrameHostImpl*>(render_frame_host); - frame_tree_node_id_ = rfh->frame_tree_node()->frame_tree_node_id(); + + // Don't pay attention to swapped out RenderFrameHosts in the main frame. + // TODO(nasko): Remove once swappedout:// is gone. + // See https://crbug.com/357747. + if (!RenderFrameHostImpl::IsRFHStateActive(rfh->rfh_state())) { + DLOG(INFO) << "Skipping swapped out RFH: " + << rfh->GetSiteInstance()->GetSiteURL(); + return; + } + + // If this object was not created with a specified frame tree node, then use + // the first created active RenderFrameHost. Once a node is selected, there + // shouldn't be any other frames being created. + int frame_tree_node_id = rfh->frame_tree_node()->frame_tree_node_id(); + DCHECK(frame_tree_node_id_ == 0 || + frame_tree_node_id_ == frame_tree_node_id); + frame_tree_node_id_ = frame_tree_node_id; } void DidCommitProvisionalLoadForFrame( @@ -969,9 +982,137 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, } } +// Verify the tree of FrameNavigationEntries after initial about:blank commits +// in subframes, which should not count as real committed loads. +IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, + FrameNavigationEntry_BlankAutoSubframe) { + GURL main_url(embedded_test_server()->GetURL( + "/navigation_controller/simple_page_1.html")); + NavigateToURL(shell(), main_url); + const NavigationControllerImpl& controller = + static_cast<const NavigationControllerImpl&>( + shell()->web_contents()->GetController()); + FrameTreeNode* root = + static_cast<WebContentsImpl*>(shell()->web_contents())-> + GetFrameTree()->root(); + + // 1. Create a iframe with no URL. + { + LoadCommittedCapturer capturer(shell()->web_contents()); + std::string script = "var iframe = document.createElement('iframe');" + "document.body.appendChild(iframe);"; + EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script)); + capturer.Wait(); + EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.transition_type()); + } + + // Check last committed NavigationEntry. + EXPECT_EQ(1, controller.GetEntryCount()); + NavigationEntryImpl* entry = controller.GetLastCommittedEntry(); + EXPECT_EQ(main_url, entry->GetURL()); + FrameNavigationEntry* root_entry = entry->root_node()->frame_entry.get(); + EXPECT_EQ(main_url, root_entry->url()); + + // Verify no subframe entries are created. + EXPECT_EQ(0U, entry->root_node()->children.size()); + EXPECT_FALSE(controller.HasCommittedRealLoad(root->child_at(0))); + + // 2. Create another iframe with an about:blank URL. + { + LoadCommittedCapturer capturer(shell()->web_contents()); + std::string script = "var iframe = document.createElement('iframe');" + "iframe.src = 'about:blank';" + "document.body.appendChild(iframe);"; + EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script)); + capturer.Wait(); + EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.transition_type()); + } + + // Check last committed NavigationEntry. + EXPECT_EQ(1, controller.GetEntryCount()); + EXPECT_EQ(entry, controller.GetLastCommittedEntry()); + + // Verify no subframe entries are created. + EXPECT_EQ(0U, entry->root_node()->children.size()); + EXPECT_FALSE(controller.HasCommittedRealLoad(root->child_at(1))); + + // 3. A real same-site navigation in the first iframe should be AUTO. + GURL frame_url(embedded_test_server()->GetURL( + "/navigation_controller/simple_page_1.html")); + { + LoadCommittedCapturer capturer(root->child_at(0)); + std::string script = "var frames = document.getElementsByTagName('iframe');" + "frames[0].src = '" + frame_url.spec() + "';"; + EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script)); + capturer.Wait(); + EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.transition_type()); + } + + // Check last committed NavigationEntry. + EXPECT_EQ(1, controller.GetEntryCount()); + EXPECT_EQ(entry, controller.GetLastCommittedEntry()); + + // Verify subframe entries if we're in --site-per-process mode. + if (base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kSitePerProcess)) { + // The entry should now have one subframe FrameNavigationEntry. + ASSERT_EQ(1U, entry->root_node()->children.size()); + FrameNavigationEntry* frame_entry = + entry->root_node()->children[0]->frame_entry.get(); + EXPECT_EQ(frame_url, frame_entry->url()); + EXPECT_TRUE(controller.HasCommittedRealLoad(root->child_at(0))); + EXPECT_FALSE(controller.HasCommittedRealLoad(root->child_at(1))); + } else { + // There are no subframe FrameNavigationEntries by default. + EXPECT_EQ(0U, entry->root_node()->children.size()); + } + + // 4. A real cross-site navigation in the second iframe should be AUTO. + GURL foo_url(embedded_test_server()->GetURL( + "foo.com", "/navigation_controller/simple_page_2.html")); + { + LoadCommittedCapturer capturer(root->child_at(1)); + std::string script = "var frames = document.getElementsByTagName('iframe');" + "frames[1].src = '" + foo_url.spec() + "';"; + EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script)); + capturer.Wait(); + EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.transition_type()); + } + + // Check last committed NavigationEntry. + EXPECT_EQ(1, controller.GetEntryCount()); + EXPECT_EQ(entry, controller.GetLastCommittedEntry()); + + // Verify subframe entries if we're in --site-per-process mode. + if (base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kSitePerProcess)) { + // The entry should now have two subframe FrameNavigationEntries. + ASSERT_EQ(2U, entry->root_node()->children.size()); + FrameNavigationEntry* frame_entry = + entry->root_node()->children[1]->frame_entry.get(); + EXPECT_EQ(foo_url, frame_entry->url()); + EXPECT_TRUE(controller.HasCommittedRealLoad(root->child_at(1))); + } else { + // There are no subframe FrameNavigationEntries by default. + EXPECT_EQ(0U, entry->root_node()->children.size()); + } + + // Check the end result of the frame tree. + if (base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kSitePerProcess)) { + FrameTreeVisualizer visualizer; + EXPECT_EQ( + " Site A ------------ proxies for B\n" + " |--Site A ------- proxies for B\n" + " +--Site B ------- proxies for A\n" + "Where A = http://127.0.0.1/\n" + " B = http://foo.com/", + visualizer.DepictFrameTree(root)); + } +} + // Verify the tree of FrameNavigationEntries after NAVIGATION_TYPE_AUTO_SUBFRAME // commits. -// TODO(creis): Test cross-site iframes. // TODO(creis): Test updating entries for history auto subframe navigations. IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, FrameNavigationEntry_AutoSubframe) { @@ -985,7 +1126,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, static_cast<WebContentsImpl*>(shell()->web_contents())-> GetFrameTree()->root(); - // Create an iframe. + // 1. Create a same-site iframe. GURL frame_url(embedded_test_server()->GetURL( "/navigation_controller/simple_page_2.html")); { @@ -1004,6 +1145,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, EXPECT_EQ(main_url, entry->GetURL()); FrameNavigationEntry* root_entry = entry->root_node()->frame_entry.get(); EXPECT_EQ(main_url, root_entry->url()); + EXPECT_FALSE(controller.GetPendingEntry()); // Verify subframe entries if we're in --site-per-process mode. if (base::CommandLine::ForCurrentProcess()->HasSwitch( @@ -1013,16 +1155,19 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, FrameNavigationEntry* frame_entry = entry->root_node()->children[0]->frame_entry.get(); EXPECT_EQ(frame_url, frame_entry->url()); + EXPECT_TRUE(controller.HasCommittedRealLoad(root->child_at(0))); } else { // There are no subframe FrameNavigationEntries by default. EXPECT_EQ(0U, entry->root_node()->children.size()); } - // Create a second iframe. + // 2. Create a second, initially cross-site iframe. + GURL foo_url(embedded_test_server()->GetURL( + "foo.com", "/navigation_controller/simple_page_1.html")); { LoadCommittedCapturer capturer(shell()->web_contents()); std::string script = "var iframe = document.createElement('iframe');" - "iframe.src = '" + frame_url.spec() + "';" + "iframe.src = '" + foo_url.spec() + "';" "document.body.appendChild(iframe);"; EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script)); capturer.Wait(); @@ -1035,6 +1180,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, EXPECT_EQ(main_url, entry->GetURL()); root_entry = entry->root_node()->frame_entry.get(); EXPECT_EQ(main_url, root_entry->url()); + EXPECT_FALSE(controller.GetPendingEntry()); // Verify subframe entries if we're in --site-per-process mode. if (base::CommandLine::ForCurrentProcess()->HasSwitch( @@ -1043,17 +1189,18 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, ASSERT_EQ(2U, entry->root_node()->children.size()); FrameNavigationEntry* frame_entry = entry->root_node()->children[1]->frame_entry.get(); - EXPECT_EQ(frame_url, frame_entry->url()); + EXPECT_EQ(foo_url, frame_entry->url()); + EXPECT_TRUE(controller.HasCommittedRealLoad(root->child_at(1))); } else { // There are no subframe FrameNavigationEntries by default. EXPECT_EQ(0U, entry->root_node()->children.size()); } - // Create a nested iframe in the second subframe. + // 3. Create a nested iframe in the second subframe. { LoadCommittedCapturer capturer(shell()->web_contents()); std::string script = "var iframe = document.createElement('iframe');" - "iframe.src = '" + frame_url.spec() + "';" + "iframe.src = '" + foo_url.spec() + "';" "document.body.appendChild(iframe);"; EXPECT_TRUE(content::ExecuteScript(root->child_at(1)->current_frame_host(), script)); @@ -1076,11 +1223,106 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, ASSERT_EQ(1U, entry->root_node()->children[1]->children.size()); FrameNavigationEntry* frame_entry = entry->root_node()->children[1]->children[0]->frame_entry.get(); - EXPECT_EQ(frame_url, frame_entry->url()); + EXPECT_EQ(foo_url, frame_entry->url()); } else { // There are no subframe FrameNavigationEntries by default. EXPECT_EQ(0U, entry->root_node()->children.size()); } + + // TODO(creis): Add tests for another subframe on B, and for a subframe on A + // within it. Both are currently broken. + + // Check the end result of the frame tree. + if (base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kSitePerProcess)) { + FrameTreeVisualizer visualizer; + EXPECT_EQ( + " Site A ------------ proxies for B\n" + " |--Site A ------- proxies for B\n" + " +--Site B ------- proxies for A\n" + " +--Site B -- proxies for A\n" + "Where A = http://127.0.0.1/\n" + " B = http://foo.com/", + visualizer.DepictFrameTree(root)); + } +} + +// Verify the tree of FrameNavigationEntries after NAVIGATION_TYPE_NEW_SUBFRAME +// commits. +IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, + FrameNavigationEntry_NewSubframe) { + GURL main_url(embedded_test_server()->GetURL( + "/navigation_controller/simple_page_1.html")); + NavigateToURL(shell(), main_url); + FrameTreeNode* root = + static_cast<WebContentsImpl*>(shell()->web_contents())-> + GetFrameTree()->root(); + + // 1. Create a same-site iframe. + GURL frame_url(embedded_test_server()->GetURL( + "/navigation_controller/simple_page_2.html")); + { + LoadCommittedCapturer capturer(shell()->web_contents()); + std::string script = "var iframe = document.createElement('iframe');" + "iframe.src = '" + frame_url.spec() + "';" + "document.body.appendChild(iframe);"; + EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script)); + capturer.Wait(); + } + + // 2. Navigate in the first subframe same-site. + GURL frame_url2(embedded_test_server()->GetURL( + "/navigation_controller/page_with_links.html")); + { + FrameNavigateParamsCapturer capturer(root->child_at(0)); + NavigateFrameToURL(root->child_at(0), frame_url2); + capturer.Wait(); + EXPECT_EQ(ui::PAGE_TRANSITION_MANUAL_SUBFRAME, + capturer.params().transition); + EXPECT_EQ(NAVIGATION_TYPE_NEW_SUBFRAME, capturer.details().type); + } + + // 3. Create a second, initially cross-site iframe. + GURL foo_url(embedded_test_server()->GetURL( + "foo.com", "/navigation_controller/simple_page_1.html")); + { + LoadCommittedCapturer capturer(shell()->web_contents()); + std::string script = "var iframe = document.createElement('iframe');" + "iframe.src = '" + foo_url.spec() + "';" + "document.body.appendChild(iframe);"; + EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script)); + capturer.Wait(); + } + + // 4. Navigate in the second subframe cross-site. + GURL bar_url(embedded_test_server()->GetURL( + "bar.com", "/navigation_controller/simple_page_1.html")); + { + FrameNavigateParamsCapturer capturer(root->child_at(1)); + std::string script = "var frames = document.getElementsByTagName('iframe');" + "frames[1].src = '" + bar_url.spec() + "';"; + EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script)); + capturer.Wait(); + EXPECT_EQ(ui::PAGE_TRANSITION_MANUAL_SUBFRAME, + capturer.params().transition); + EXPECT_EQ(NAVIGATION_TYPE_NEW_SUBFRAME, capturer.details().type); + } + + // TODO(creis): Expand this test once we clone FrameNavigationEntries for + // NEW_SUBFRAME navigations. + + // Check the end result of the frame tree. + if (base::CommandLine::ForCurrentProcess()->HasSwitch( + switches::kSitePerProcess)) { + FrameTreeVisualizer visualizer; + EXPECT_EQ( + " Site A ------------ proxies for B\n" + " |--Site A ------- proxies for B\n" + " +--Site B ------- proxies for A\n" + "Where A = http://127.0.0.1/\n" + " B = http://bar.com/", + visualizer.DepictFrameTree(root)); + } } namespace { diff --git a/content/browser/frame_host/navigation_entry_impl.cc b/content/browser/frame_host/navigation_entry_impl.cc index 2723606..7ba5cf3 100644 --- a/content/browser/frame_host/navigation_entry_impl.cc +++ b/content/browser/frame_host/navigation_entry_impl.cc @@ -33,6 +33,17 @@ NavigationEntryImpl::TreeNode::TreeNode(FrameNavigationEntry* frame_entry) NavigationEntryImpl::TreeNode::~TreeNode() { } +bool NavigationEntryImpl::TreeNode::MatchesFrame( + FrameTreeNode* frame_tree_node) const { + if (frame_tree_node->frame_tree_node_id() == + frame_entry->frame_tree_node_id()) + return true; + + // For now, we set the root FNE's FrameTreeNode ID to -1. + return frame_tree_node->IsMainFrame() && + frame_entry->frame_tree_node_id() == -1; +} + NavigationEntryImpl::TreeNode* NavigationEntryImpl::TreeNode::Clone() const { // Clone the tree using a copy of the FrameNavigationEntry, without sharing. NavigationEntryImpl::TreeNode* copy = @@ -419,6 +430,7 @@ StartNavigationParams NavigationEntryImpl::ConstructStartNavigationParams() RequestNavigationParams NavigationEntryImpl::ConstructRequestNavigationParams( base::TimeTicks navigation_start, + bool has_committed_real_load, bool intended_as_new_entry, int pending_history_list_offset, int current_history_list_offset, @@ -444,9 +456,9 @@ RequestNavigationParams NavigationEntryImpl::ConstructRequestNavigationParams( return RequestNavigationParams( GetIsOverridingUserAgent(), navigation_start, redirects, GetCanLoadLocalResources(), base::Time::Now(), GetPageState(), - GetPageID(), GetUniqueID(), intended_as_new_entry, pending_offset_to_send, - current_offset_to_send, current_length_to_send, - should_clear_history_list()); + GetPageID(), GetUniqueID(), has_committed_real_load, + intended_as_new_entry, pending_offset_to_send, current_offset_to_send, + current_length_to_send, should_clear_history_list()); } void NavigationEntryImpl::ResetForCommit() { @@ -477,27 +489,9 @@ void NavigationEntryImpl::AddOrUpdateFrameEntry(FrameTreeNode* frame_tree_node, // We should already have a TreeNode for the parent node by the time this node // commits. Find it first. DCHECK(frame_tree_node->parent()); - int parent_ftn_id = frame_tree_node->parent()->frame_tree_node_id(); - bool found = false; - NavigationEntryImpl::TreeNode* parent_node = nullptr; - std::queue<NavigationEntryImpl::TreeNode*> work_queue; - work_queue.push(root_node()); - while (!found && !work_queue.empty()) { - parent_node = work_queue.front(); - work_queue.pop(); - // The root FNE will have an ID of -1, so check for that as well. - if (parent_node->frame_entry->frame_tree_node_id() == parent_ftn_id || - (parent_node->frame_entry->frame_tree_node_id() == -1 && - parent_node == root_node() && - frame_tree_node->parent()->IsMainFrame())) { - found = true; - break; - } - // Enqueue any children and keep looking. - for (auto& child : parent_node->children) - work_queue.push(child); - } - if (!found) { + NavigationEntryImpl::TreeNode* parent_node = + FindFrameEntry(frame_tree_node->parent()); + if (!parent_node) { // The renderer should not send a commit for a subframe before its parent. // TODO(creis): This can currently happen because we don't yet clone the // FrameNavigationEntry tree on manual subframe navigations. Once that's @@ -515,14 +509,21 @@ void NavigationEntryImpl::AddOrUpdateFrameEntry(FrameTreeNode* frame_tree_node, } } - // No entry exists yet, so create a new one. Unordered list, since we expect - // to look up entries by frame sequence number or unique name. + // No entry exists yet, so create a new one unless it's for about:blank. + // Unordered list, since we expect to look up entries by frame sequence number + // or unique name. + if (url == GURL(url::kAboutBlankURL)) + return; FrameNavigationEntry* frame_entry = new FrameNavigationEntry( frame_tree_node_id, site_instance, url, referrer); parent_node->children.push_back( new NavigationEntryImpl::TreeNode(frame_entry)); } +bool NavigationEntryImpl::HasFrameEntry(FrameTreeNode* frame_tree_node) const { + return FindFrameEntry(frame_tree_node) != nullptr; +} + void NavigationEntryImpl::SetScreenshotPNGData( scoped_refptr<base::RefCountedBytes> png_data) { screenshot_ = png_data; @@ -534,4 +535,25 @@ GURL NavigationEntryImpl::GetHistoryURLForDataURL() const { return GetBaseURLForDataURL().is_empty() ? GURL() : GetVirtualURL(); } +NavigationEntryImpl::TreeNode* NavigationEntryImpl::FindFrameEntry( + FrameTreeNode* frame_tree_node) const { + NavigationEntryImpl::TreeNode* node = nullptr; + std::queue<NavigationEntryImpl::TreeNode*> work_queue; + work_queue.push(root_node()); + while (!work_queue.empty()) { + node = work_queue.front(); + work_queue.pop(); + if (node->MatchesFrame(frame_tree_node)) { + // Only the root TreeNode should have a FTN ID of -1. + DCHECK_IMPLIES(node->frame_entry->frame_tree_node_id() == -1, + node == root_node()); + return node; + } + // Enqueue any children and keep looking. + for (auto& child : node->children) + work_queue.push(child); + } + return nullptr; +} + } // namespace content diff --git a/content/browser/frame_host/navigation_entry_impl.h b/content/browser/frame_host/navigation_entry_impl.h index d5f43ea..8fb41c3 100644 --- a/content/browser/frame_host/navigation_entry_impl.h +++ b/content/browser/frame_host/navigation_entry_impl.h @@ -34,6 +34,9 @@ class CONTENT_EXPORT NavigationEntryImpl TreeNode(FrameNavigationEntry* frame_entry); ~TreeNode(); + // Returns whether this TreeNode corresponds to |frame_tree_node|. + bool MatchesFrame(FrameTreeNode* frame_tree_node) const; + // Returns a deep copy of the tree with copies of each node's // FrameNavigationEntries. We do not yet share FrameNavigationEntries // across trees. @@ -134,6 +137,7 @@ class CONTENT_EXPORT NavigationEntryImpl StartNavigationParams ConstructStartNavigationParams() const; RequestNavigationParams ConstructRequestNavigationParams( base::TimeTicks navigation_start, + bool has_committed_real_load, bool intended_as_new_entry, int pending_offset_to_send, int current_offset_to_send, @@ -156,11 +160,17 @@ class CONTENT_EXPORT NavigationEntryImpl // its FrameNavigationEntry. A new FrameNavigationEntry is added if none // exists, or else the existing one (which might be shared with other // NavigationEntries) is updated with the given parameters. + // Does nothing if there is no entry already and |url| is about:blank, since + // that does not count as a real commit. void AddOrUpdateFrameEntry(FrameTreeNode* frame_tree_node, SiteInstanceImpl* site_instance, const GURL& url, const Referrer& referrer); + // Returns whether this entry has a FrameNavigationEntry for the given + // |frame_tree_node|. + bool HasFrameEntry(FrameTreeNode* frame_tree_node) const; + void set_unique_id(int unique_id) { unique_id_ = unique_id; } @@ -307,6 +317,10 @@ class CONTENT_EXPORT NavigationEntryImpl #endif private: + // Finds the TreeNode associated with |frame_tree_node|, if any. + NavigationEntryImpl::TreeNode* FindFrameEntry( + FrameTreeNode* frame_tree_node) const; + // WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING // Session/Tab restore save portions of this class so that it can be recreated // later. If you add a new field that needs to be persisted you'll have to diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc index ce8b2ae..b5f959b 100644 --- a/content/browser/frame_host/navigation_request.cc +++ b/content/browser/frame_host/navigation_request.cc @@ -90,7 +90,7 @@ scoped_ptr<NavigationRequest> NavigationRequest::CreateBrowserInitiated( BeginNavigationParams(method, headers.ToString(), LoadFlagFromNavigationType(navigation_type), false), entry.ConstructRequestNavigationParams( - navigation_start, + navigation_start, controller->HasCommittedRealLoad(frame_tree_node), controller->GetPendingEntryIndex() == -1, controller->GetIndexOfEntry(&entry), controller->GetLastCommittedEntryIndex(), @@ -113,6 +113,7 @@ scoped_ptr<NavigationRequest> NavigationRequest::CreateRendererInitiated( // TODO(clamy): See if the navigation start time should be measured in the // renderer and sent to the browser instead of being measured here. // TODO(clamy): The pending history list offset should be properly set. + // TODO(clamy): Set has_committed_real_load. RequestNavigationParams request_params; request_params.current_history_list_offset = current_history_list_offset; request_params.current_history_list_length = current_history_list_length; diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc index b9dc7c0..8219a3d 100644 --- a/content/browser/frame_host/navigator_impl.cc +++ b/content/browser/frame_host/navigator_impl.cc @@ -325,6 +325,7 @@ bool NavigatorImpl::NavigateToEntry( entry.ConstructStartNavigationParams(), entry.ConstructRequestNavigationParams( navigation_start, + controller_->HasCommittedRealLoad(frame_tree_node), controller_->GetPendingEntryIndex() == -1, controller_->GetIndexOfEntry(&entry), controller_->GetLastCommittedEntryIndex(), diff --git a/content/common/frame_messages.h b/content/common/frame_messages.h index 4187c13..3ad3deb 100644 --- a/content/common/frame_messages.h +++ b/content/common/frame_messages.h @@ -285,6 +285,7 @@ IPC_STRUCT_TRAITS_BEGIN(content::RequestNavigationParams) IPC_STRUCT_TRAITS_MEMBER(page_state) IPC_STRUCT_TRAITS_MEMBER(page_id) IPC_STRUCT_TRAITS_MEMBER(nav_entry_id) + IPC_STRUCT_TRAITS_MEMBER(has_committed_real_load) IPC_STRUCT_TRAITS_MEMBER(intended_as_new_entry) IPC_STRUCT_TRAITS_MEMBER(pending_history_list_offset) IPC_STRUCT_TRAITS_MEMBER(current_history_list_offset) diff --git a/content/common/navigation_params.cc b/content/common/navigation_params.cc index 57149de..33d1c8e 100644 --- a/content/common/navigation_params.cc +++ b/content/common/navigation_params.cc @@ -85,6 +85,7 @@ RequestNavigationParams::RequestNavigationParams() request_time(base::Time::Now()), page_id(-1), nav_entry_id(0), + has_committed_real_load(false), intended_as_new_entry(false), pending_history_list_offset(-1), current_history_list_offset(-1), @@ -101,6 +102,7 @@ RequestNavigationParams::RequestNavigationParams( const PageState& page_state, int32 page_id, int nav_entry_id, + bool has_committed_real_load, bool intended_as_new_entry, int pending_history_list_offset, int current_history_list_offset, @@ -114,6 +116,7 @@ RequestNavigationParams::RequestNavigationParams( page_state(page_state), page_id(page_id), nav_entry_id(nav_entry_id), + has_committed_real_load(has_committed_real_load), intended_as_new_entry(intended_as_new_entry), pending_history_list_offset(pending_history_list_offset), current_history_list_offset(current_history_list_offset), diff --git a/content/common/navigation_params.h b/content/common/navigation_params.h index 90eb70e..8059471 100644 --- a/content/common/navigation_params.h +++ b/content/common/navigation_params.h @@ -170,6 +170,7 @@ struct CONTENT_EXPORT RequestNavigationParams { const PageState& page_state, int32 page_id, int nav_entry_id, + bool has_committed_real_load, bool intended_as_new_entry, int pending_history_list_offset, int current_history_list_offset, @@ -211,6 +212,12 @@ struct CONTENT_EXPORT RequestNavigationParams { // the resulting FrameHostMsg_DidCommitProvisionalLoad message. int nav_entry_id; + // Whether the frame being navigated has already committed a real page, which + // affects how new navigations are classified in the renderer process. + // This currently is only ever set to true in --site-per-process mode. + // TODO(creis): Create FrameNavigationEntries by default so this always works. + bool has_committed_real_load; + // For browser-initiated navigations, this is true if this is a new entry // being navigated to. This is false otherwise. TODO(avi): Remove this when // the pending entry situation is made sane and the browser keeps them around diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc index f9c71e7..2bfd535 100644 --- a/content/renderer/render_frame_impl.cc +++ b/content/renderer/render_frame_impl.cc @@ -4406,6 +4406,13 @@ void RenderFrameImpl::NavigateInternal( GetContentClient()->SetActiveURL(common_params.url); + // If this frame isn't in the same process as the main frame, it may naively + // assume that this is the first navigation in the iframe, but this may not + // actually be the case. Inform the frame's state machine if this frame has + // already committed other loads. + if (request_params.has_committed_real_load && frame_->parent()) + frame_->setCommittedFirstRealLoad(); + if (is_reload && !render_view_->history_controller()->GetCurrentEntry()) { // We cannot reload if we do not have any history state. This happens, for // example, when recovering from a crash. |