Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Notify the renderer if a history navigation has no subframe items.
Browse files Browse the repository at this point in the history
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}
  • Loading branch information
creis authored and Commit bot committed Sep 22, 2016
1 parent 2305bf5 commit 45b3bba
Show file tree
Hide file tree
Showing 10 changed files with 141 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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<NavigationControllerImpl&>(
shell()->web_contents()->GetController());
FrameTreeNode* root =
static_cast<WebContentsImpl*>(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<NavigationEntryImpl> 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<std::unique_ptr<NavigationEntry>> 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.
Expand Down
13 changes: 10 additions & 3 deletions content/browser/frame_host/navigation_entry_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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());
Expand Down
10 changes: 10 additions & 0 deletions content/browser/frame_host/navigation_entry_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions content/browser/frame_host/navigation_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ std::unique_ptr<NavigationRequest> 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),
Expand Down Expand Up @@ -169,6 +170,7 @@ std::unique_ptr<NavigationRequest> 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
Expand Down
1 change: 1 addition & 0 deletions content/browser/frame_host/navigator_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
1 change: 1 addition & 0 deletions content/common/frame_messages.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions content/common/navigation_params.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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,
Expand All @@ -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),
Expand Down
9 changes: 9 additions & 0 deletions content/common/navigation_params.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
25 changes: 22 additions & 3 deletions content/renderer/render_frame_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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_));
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
11 changes: 11 additions & 0 deletions content/renderer/render_frame_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -1094,6 +1094,17 @@ class CONTENT_EXPORT RenderFrameImpl
// didCreateDataSource().
std::unique_ptr<NavigationParams> 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_;
Expand Down

0 comments on commit 45b3bba

Please sign in to comment.