From 45b3bba45f5c1276726541db2f6f8ca6c5e05b65 Mon Sep 17 00:00:00 2001 From: creis Date: Thu, 22 Sep 2016 15:47:13 -0700 Subject: [PATCH] Notify the renderer if a history navigation has no subframe items. In this case, the renderer does not need to consult the browser process if subframes are created during the navigation. Since there are no history items for it, the renderer can just load the default URL. BUG=638088, 639842 TEST=Restore chrome://settings after disabling MD settings mode. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2316003002 Cr-Commit-Position: refs/heads/master@{#420486} --- .../navigation_controller_impl_browsertest.cc | 72 +++++++++++++++++++ .../frame_host/navigation_entry_impl.cc | 13 +++- .../frame_host/navigation_entry_impl.h | 10 +++ .../browser/frame_host/navigation_request.cc | 2 + content/browser/frame_host/navigator_impl.cc | 1 + content/common/frame_messages.h | 1 + content/common/navigation_params.cc | 3 + content/common/navigation_params.h | 9 +++ content/renderer/render_frame_impl.cc | 25 ++++++- content/renderer/render_frame_impl.h | 11 +++ 10 files changed, 141 insertions(+), 6 deletions(-) diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc index 2db9ee3d9053f..d732bd929d916 100644 --- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc +++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc @@ -3642,6 +3642,78 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, } } +// Verify that we can finish loading a page on restore if the PageState is +// missing subframes. See https://crbug.com/638088. +IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, + FrameNavigationEntry_RestoreViaPartialPageState) { + GURL main_url(embedded_test_server()->GetURL( + "a.com", "/navigation_controller/inject_into_blank_iframe.html")); + GURL blank_url(url::kAboutBlankURL); + NavigationControllerImpl& controller = + static_cast( + shell()->web_contents()->GetController()); + FrameTreeNode* root = + static_cast(shell()->web_contents()) + ->GetFrameTree() + ->root(); + + // Create a NavigationEntry to restore, as if it had been loaded before. The + // page has an about:blank iframe and injects content into it, but the + // PageState lacks any subframe history items. This may happen during a + // restore of a bad session or if the page has changed since the last visit. + // Chrome should be robust to this and should be able to load the frame from + // its default URL. + std::unique_ptr restored_entry = + NavigationEntryImpl::FromNavigationEntry( + NavigationControllerImpl::CreateNavigationEntry( + main_url, Referrer(), ui::PAGE_TRANSITION_RELOAD, false, + std::string(), controller.GetBrowserContext())); + restored_entry->SetPageID(0); + restored_entry->SetPageState(PageState::CreateFromURL(main_url)); + EXPECT_EQ(0U, restored_entry->root_node()->children.size()); + + // Restore the new entry in a new tab and verify the iframe loads and has + // content injected into it. + std::vector> entries; + entries.push_back(std::move(restored_entry)); + controller.Restore(entries.size() - 1, + RestoreType::LAST_SESSION_EXITED_CLEANLY, &entries); + ASSERT_EQ(0u, entries.size()); + { + TestNavigationObserver restore_observer(shell()->web_contents()); + controller.LoadIfNecessary(); + restore_observer.Wait(); + } + ASSERT_EQ(1U, root->child_count()); + EXPECT_EQ(main_url, root->current_url()); + EXPECT_EQ(blank_url, root->child_at(0)->current_url()); + + EXPECT_EQ(1, controller.GetEntryCount()); + EXPECT_EQ(0, controller.GetLastCommittedEntryIndex()); + NavigationEntryImpl* new_entry = controller.GetLastCommittedEntry(); + + // Verify subframe entries if they're enabled (e.g. in --site-per-process). + if (SiteIsolationPolicy::UseSubframeNavigationEntries()) { + // The entry should have a FrameNavigationEntry for the blank subframe. + EXPECT_EQ(main_url, new_entry->root_node()->frame_entry->url()); + ASSERT_EQ(1U, new_entry->root_node()->children.size()); + EXPECT_EQ(blank_url, + new_entry->root_node()->children[0]->frame_entry->url()); + } else { + EXPECT_EQ(0U, new_entry->root_node()->children.size()); + } + + // Verify that the parent was able to script the iframe. + std::string expected_text("Injected text"); + { + std::string value; + EXPECT_TRUE(ExecuteScriptAndExtractString( + root->child_at(0), + "domAutomationController.send(document.body.innerHTML)", &value)); + EXPECT_EQ(expected_text, value); + } +} + // Verifies that the |frame_unique_name| is set to the correct frame, so that we // can match subframe FrameNavigationEntries to newly created frames after // back/forward and restore. diff --git a/content/browser/frame_host/navigation_entry_impl.cc b/content/browser/frame_host/navigation_entry_impl.cc index 31b1d508e8ee0..fbf56542c1609 100644 --- a/content/browser/frame_host/navigation_entry_impl.cc +++ b/content/browser/frame_host/navigation_entry_impl.cc @@ -691,6 +691,7 @@ RequestNavigationParams NavigationEntryImpl::ConstructRequestNavigationParams( const FrameNavigationEntry& frame_entry, bool is_same_document_history_load, bool is_history_navigation_in_new_child, + bool has_subtree_history_items, bool has_committed_real_load, bool intended_as_new_entry, int pending_history_list_offset, @@ -719,9 +720,9 @@ RequestNavigationParams NavigationEntryImpl::ConstructRequestNavigationParams( GetIsOverridingUserAgent(), redirects, GetCanLoadLocalResources(), base::Time::Now(), frame_entry.page_state(), GetPageID(), GetUniqueID(), is_same_document_history_load, is_history_navigation_in_new_child, - has_committed_real_load, intended_as_new_entry, pending_offset_to_send, - current_offset_to_send, current_length_to_send, IsViewSourceMode(), - should_clear_history_list()); + has_subtree_history_items, has_committed_real_load, intended_as_new_entry, + pending_offset_to_send, current_offset_to_send, current_length_to_send, + IsViewSourceMode(), should_clear_history_list()); #if defined(OS_ANDROID) if (GetDataURLAsString() && GetDataURLAsString()->size() <= kMaxLengthOfDataURLString) { @@ -846,6 +847,12 @@ FrameNavigationEntry* NavigationEntryImpl::GetFrameEntry( return tree_node ? tree_node->frame_entry.get() : nullptr; } +bool NavigationEntryImpl::HasSubtreeHistoryItems( + FrameTreeNode* frame_tree_node) const { + NavigationEntryImpl::TreeNode* tree_node = FindFrameEntry(frame_tree_node); + return tree_node && !tree_node->children.empty(); +} + void NavigationEntryImpl::ClearStaleFrameEntriesForNewFrame( FrameTreeNode* frame_tree_node) { DCHECK(!frame_tree_node->IsMainFrame()); diff --git a/content/browser/frame_host/navigation_entry_impl.h b/content/browser/frame_host/navigation_entry_impl.h index 4c96d55e2b734..fa2c88e9614ec 100644 --- a/content/browser/frame_host/navigation_entry_impl.h +++ b/content/browser/frame_host/navigation_entry_impl.h @@ -185,6 +185,7 @@ class CONTENT_EXPORT NavigationEntryImpl const FrameNavigationEntry& frame_entry, bool is_same_document_history_load, bool is_history_navigation_in_new_child, + bool has_subtree_history_items, bool has_committed_real_load, bool intended_as_new_entry, int pending_offset_to_send, @@ -228,6 +229,15 @@ class CONTENT_EXPORT NavigationEntryImpl // there is one in this NavigationEntry. FrameNavigationEntry* GetFrameEntry(FrameTreeNode* frame_tree_node) const; + // Returns whether the TreeNode associated with |frame_tree_node| has any + // children. If not, the renderer process does not need to ask the browser + // when new subframes are created during a back/forward navigation. + // TODO(creis): Send a data structure with all unique names in the subtree, + // along with any corresponding same-process PageStates. The renderer only + // needs to ask the browser process to handle the cross-process cases. + // See https://crbug.com/639842. + bool HasSubtreeHistoryItems(FrameTreeNode* frame_tree_node) const; + // Removes any subframe FrameNavigationEntries that match the unique name of // |frame_tree_node|, and all of their children. There should be at most one, // since collisions are avoided but leave old FrameNavigationEntries in the diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc index 63679331dcc37..11e987c4ada6b 100644 --- a/content/browser/frame_host/navigation_request.cc +++ b/content/browser/frame_host/navigation_request.cc @@ -137,6 +137,7 @@ std::unique_ptr NavigationRequest::CreateBrowserInitiated( entry.ConstructRequestNavigationParams( frame_entry, is_same_document_history_load, is_history_navigation_in_new_child, + entry.HasSubtreeHistoryItems(frame_tree_node), frame_tree_node->has_committed_real_load(), controller->GetPendingEntryIndex() == -1, controller->GetIndexOfEntry(&entry), @@ -169,6 +170,7 @@ std::unique_ptr NavigationRequest::CreateRendererInitiated( 0, // nav_entry_id false, // is_same_document_history_load false, // is_history_navigation_in_new_child + false, // has_subtree_history_items frame_tree_node->has_committed_real_load(), false, // intended_as_new_entry -1, // pending_history_list_offset diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc index ca9ce6b7dafe9..a25c56910565a 100644 --- a/content/browser/frame_host/navigator_impl.cc +++ b/content/browser/frame_host/navigator_impl.cc @@ -396,6 +396,7 @@ bool NavigatorImpl::NavigateToEntry( entry.ConstructRequestNavigationParams( frame_entry, is_same_document_history_load, is_history_navigation_in_new_child, + entry.HasSubtreeHistoryItems(frame_tree_node), frame_tree_node->has_committed_real_load(), controller_->GetPendingEntryIndex() == -1, controller_->GetIndexOfEntry(&entry), diff --git a/content/common/frame_messages.h b/content/common/frame_messages.h index 31c2b05502286..1a15567b6fed0 100644 --- a/content/common/frame_messages.h +++ b/content/common/frame_messages.h @@ -379,6 +379,7 @@ IPC_STRUCT_TRAITS_BEGIN(content::RequestNavigationParams) IPC_STRUCT_TRAITS_MEMBER(nav_entry_id) IPC_STRUCT_TRAITS_MEMBER(is_same_document_history_load) IPC_STRUCT_TRAITS_MEMBER(is_history_navigation_in_new_child) + IPC_STRUCT_TRAITS_MEMBER(has_subtree_history_items) IPC_STRUCT_TRAITS_MEMBER(has_committed_real_load) IPC_STRUCT_TRAITS_MEMBER(intended_as_new_entry) IPC_STRUCT_TRAITS_MEMBER(pending_history_list_offset) diff --git a/content/common/navigation_params.cc b/content/common/navigation_params.cc index b1ce7ea811d75..bbb431bf3559c 100644 --- a/content/common/navigation_params.cc +++ b/content/common/navigation_params.cc @@ -139,6 +139,7 @@ RequestNavigationParams::RequestNavigationParams() nav_entry_id(0), is_same_document_history_load(false), is_history_navigation_in_new_child(false), + has_subtree_history_items(false), has_committed_real_load(false), intended_as_new_entry(false), pending_history_list_offset(-1), @@ -159,6 +160,7 @@ RequestNavigationParams::RequestNavigationParams( int nav_entry_id, bool is_same_document_history_load, bool is_history_navigation_in_new_child, + bool has_subtree_history_items, bool has_committed_real_load, bool intended_as_new_entry, int pending_history_list_offset, @@ -175,6 +177,7 @@ RequestNavigationParams::RequestNavigationParams( nav_entry_id(nav_entry_id), is_same_document_history_load(is_same_document_history_load), is_history_navigation_in_new_child(is_history_navigation_in_new_child), + has_subtree_history_items(has_subtree_history_items), has_committed_real_load(has_committed_real_load), intended_as_new_entry(intended_as_new_entry), pending_history_list_offset(pending_history_list_offset), diff --git a/content/common/navigation_params.h b/content/common/navigation_params.h index 5dde81d283baa..f737d0d90e28e 100644 --- a/content/common/navigation_params.h +++ b/content/common/navigation_params.h @@ -228,6 +228,7 @@ struct CONTENT_EXPORT RequestNavigationParams { int nav_entry_id, bool is_same_document_history_load, bool is_history_navigation_in_new_child, + bool has_subtree_history_items, bool has_committed_real_load, bool intended_as_new_entry, int pending_history_list_offset, @@ -278,6 +279,14 @@ struct CONTENT_EXPORT RequestNavigationParams { // a URL from a session history item. Defaults to false. bool is_history_navigation_in_new_child; + // If this is a history navigation, this indicates whether the browser process + // is aware of any subframe history items for the given frame. If not, the + // renderer does not need to check with the browser if any subframes are + // created during the navigation. + // TODO(creis): Expand this to a data structure of unique names and + // corresponding PageStates in https://crbug.com/639842. + bool has_subtree_history_items; + // 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. diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc index 621826336e4ff..0483c3d0781c3 100644 --- a/content/renderer/render_frame_impl.cc +++ b/content/renderer/render_frame_impl.cc @@ -1043,6 +1043,7 @@ RenderFrameImpl::RenderFrameImpl(const CreateParams& params) render_view_(params.render_view->AsWeakPtr()), routing_id_(params.routing_id), proxy_routing_id_(MSG_ROUTING_NONE), + browser_has_subtree_history_items_(false), #if defined(ENABLE_PLUGINS) plugin_power_saver_helper_(nullptr), plugin_find_handler_(nullptr), @@ -1165,6 +1166,11 @@ void RenderFrameImpl::Initialize() { RenderFrameImpl* parent_frame = RenderFrameImpl::FromWebFrame( frame_->parent()); if (parent_frame) { + // Inherit knowledge of whether we need to consult the browser process for + // a history item on the first navigation. This is inherited by further + // subframes and cleared at didStopLoading. + browser_has_subtree_history_items_ = + parent_frame->browser_has_subtree_history_items_; is_using_lofi_ = parent_frame->IsUsingLoFi(); effective_connection_type_ = parent_frame->getEffectiveConnectionType(); } @@ -4819,6 +4825,12 @@ void RenderFrameImpl::didStartLoading(bool to_different_document) { void RenderFrameImpl::didStopLoading() { TRACE_EVENT1("navigation,rail", "RenderFrameImpl::didStopLoading", "id", routing_id_); + + // Any subframes created after this point won't be considered part of the + // current history navigation (if this was one), so we don't need to track + // this state anymore. + browser_has_subtree_history_items_ = false; + render_view_->FrameDidStopLoading(frame_); Send(new FrameHostMsg_DidStopLoading(routing_id_)); } @@ -4963,10 +4975,12 @@ WebNavigationPolicy RenderFrameImpl::decidePolicyForNavigation( } // In OOPIF-enabled modes, back/forward navigations in newly created subframes - // should be sent to the browser in case there is a matching - // FrameNavigationEntry. If none is found, fall back to the default url. + // should be sent to the browser if there is a chance there is a matching + // FrameNavigationEntry. If none is found (or if the browser has indicated it + // has no subtree history items), fall back to loading the default url. if (SiteIsolationPolicy::UseSubframeNavigationEntries() && - info.isHistoryNavigationInNewChildFrame && is_content_initiated) { + info.isHistoryNavigationInNewChildFrame && is_content_initiated && + browser_has_subtree_history_items_) { // Don't do this if |info| also says it is a client redirect, in which case // JavaScript on the page is trying to interrupt the history navigation. if (!info.isClientRedirect) { @@ -5570,6 +5584,11 @@ void RenderFrameImpl::NavigateInternal( : blink::WebFrameLoadType::BackForward; should_load_request = true; + // Remember whether we should consult the browser process for any + // subframes created during this history navigation. + browser_has_subtree_history_items_ = + request_params.has_subtree_history_items; + if (history_load_type == blink::WebHistorySameDocumentLoad) { // If this is marked as a same document load but we haven't committed // anything, treat it as a new load. The browser shouldn't let this diff --git a/content/renderer/render_frame_impl.h b/content/renderer/render_frame_impl.h index 43cc21620f94d..b90ce57a4997d 100644 --- a/content/renderer/render_frame_impl.h +++ b/content/renderer/render_frame_impl.h @@ -1094,6 +1094,17 @@ class CONTENT_EXPORT RenderFrameImpl // didCreateDataSource(). std::unique_ptr pending_navigation_params_; + // Keeps track of whether the browser process has any history items that need + // to be used for subframes of this frame (in the case of a history + // navigation). If not, the renderer can skip sending an IPC to the browser + // and directly load any initial URLs for children itself. This state is + // cleared during didStopLoading, since it is not needed after the first load + // completes and is never used after the initial navigation. It is inherited + // by subframes. + // TODO(creis): Switch this to a data structure of unique names and + // corresponding same-process PageStates in https://crbug.com/639842. + bool browser_has_subtree_history_items_; + // Stores the current history item for this frame, so that updates to it can // be reported to the browser process via SendUpdateState. blink::WebHistoryItem current_history_item_;