Skip to content

Commit

Permalink
Mojofy DidCommitProvisionalLoad.
Browse files Browse the repository at this point in the history 
Refactor FrameHostMsg_DidCommitProvisionalLoad from being a legacy IPC
message into a method on mojom::FrameHost, a frame-scoped,
Channel-associated interface. There is no change to functionality.

This is a reland of crrev.com/500294, now that the reentrancy issue in
GpuChannelHost::Send has been resolved by crrev.com/502124. TBR'ing
original reviewers.

TBR=altimin@chromium.org,rockot@chromium.org,nasko@chromium.org

Bug: 729021
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Ic5bd3e1a69c8d4b7a58ae843ccbfe0e6d6241db4
Reviewed-on: https://chromium-review.googlesource.com/667115
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502254}
  • Loading branch information
Balazs Engedy authored and Commit Bot committed Sep 15, 2017
1 parent 387cce3 commit a40712f
Show file tree
Hide file tree
Showing 30 changed files with 477 additions and 270 deletions.
6 changes: 3 additions & 3 deletions chrome/browser/password_manager/credential_manager_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
// 1.) FrameHostMsg_DidStartProvisionalLoad
// 2.) FrameLoader::PrepareForCommit
// 2.1) Document::Shutdown (old Document)
// 3.) FrameHostMsg_DidCommitProvisionalLoad (new load)
// 3.) mojom::FrameHost::DidCommitProvisionalLoad (new load)
// ... loading ...
// 4.) FrameHostMsg_DidStopLoading
// 5.) content::WaitForLoadStop inside NavigateToURL returns
Expand All @@ -200,7 +200,7 @@ class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
// associated interface to the ContentCredentialManager is retrieved, is
// itself Channel-associated, any InterfaceRequest messages that may have
// been issued before or during Step 2.1, will be guaranteed to arrive to
// the browser side before FrameHostMsg_DidCommitProvisionalLoad in Step 3.
// the browser side before DidCommitProvisionalLoad in Step 3.
//
// Hence it is sufficient to check that the Mojo connection is closed now.
EXPECT_FALSE(client->has_binding_for_credential_manager());
Expand All @@ -212,7 +212,7 @@ class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
// ordering with legacy IPC messages is preserved. Therefore, servicing the
// store() called from the `unload` handler, triggered from
// FrameLoader::PrepareForCommit, will be serviced before
// FrameHostMsg_DidCommitProvisionalLoad, thus before DidFinishNavigation,
// DidCommitProvisionalLoad, thus before DidFinishNavigation,
ASSERT_TRUE(client->was_store_ever_called());

BubbleObserver prompt_observer(WebContents());
Expand Down
75 changes: 29 additions & 46 deletions content/browser/frame_host/navigation_controller_impl_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#include "content/shell/browser/shell.h"
#include "content/shell/common/shell_switches.h"
#include "content/test/content_browser_test_utils_internal.h"
#include "content/test/did_commit_provisional_load_interceptor.h"
#include "net/dns/mock_host_resolver.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "net/test/embedded_test_server/http_request.h"
Expand Down Expand Up @@ -6109,56 +6110,39 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
EXPECT_EQ(start_url.GetOrigin().spec(), origin + "/");
}

// A BrowserMessageFilter that delays FrameHostMsg_DidCommitProvisionalLoad IPC
// message for a specified URL, navigates the WebContents back and then
// processes the commit message.
class GoBackAndCommitFilter : public BrowserMessageFilter {
// Helper to trigger a history-back navigation in the WebContents after the
// renderer has committed a same-process and cross-origin navigation to the
// given |url|, but before the browser side has had a chance to process the
// DidCommitProvisionalLoad message.
class HistoryNavigationBeforeCommitInjector
: public DidCommitProvisionalLoadInterceptor {
public:
GoBackAndCommitFilter(const GURL& url, WebContentsImpl* web_contents)
: BrowserMessageFilter(FrameMsgStart),
url_(url),
web_contents_(web_contents) {}

protected:
~GoBackAndCommitFilter() override {}
HistoryNavigationBeforeCommitInjector(WebContentsImpl* web_contents,
const GURL& url)
: DidCommitProvisionalLoadInterceptor(web_contents),
did_trigger_history_navigation_(false),
url_(url) {}
~HistoryNavigationBeforeCommitInjector() override {}

private:
static void NavigateBackAndCommit(const IPC::Message& message,
WebContentsImpl* web_contents) {
web_contents->GetController().GoBack();

RenderFrameHostImpl* rfh = web_contents->GetMainFrame();
DCHECK_EQ(rfh->routing_id(), message.routing_id());
rfh->OnMessageReceived(message);
bool did_trigger_history_navigation() const {
return did_trigger_history_navigation_;
}

// BrowserMessageFilter:
bool OnMessageReceived(const IPC::Message& message) override {
if (message.type() != FrameHostMsg_DidCommitProvisionalLoad::ID)
return false;

// Parse the IPC message so the URL can be checked against the expected one.
base::PickleIterator iter(message);
FrameHostMsg_DidCommitProvisionalLoad_Params validated_params;
if (!IPC::ParamTraits<FrameHostMsg_DidCommitProvisionalLoad_Params>::Read(
&message, &iter, &validated_params)) {
return false;
private:
// DidCommitProvisionalLoadInterceptor:
void WillDispatchDidCommitProvisionalLoad(
RenderFrameHost* render_frame_host,
::FrameHostMsg_DidCommitProvisionalLoad_Params* params) override {
if (!render_frame_host->GetParent() && params->url == url_) {
did_trigger_history_navigation_ = true;
web_contents()->GetController().GoBack();
}

// Only handle the message if the URLs are matching.
if (validated_params.url != url_)
return false;

BrowserThread::PostTask(
BrowserThread::UI, FROM_HERE,
base::BindOnce(&NavigateBackAndCommit, message, web_contents_));
return true;
}

bool did_trigger_history_navigation_;
GURL url_;
WebContentsImpl* web_contents_;

DISALLOW_COPY_AND_ASSIGN(GoBackAndCommitFilter);
DISALLOW_COPY_AND_ASSIGN(HistoryNavigationBeforeCommitInjector);
};

// Test which simulates a race condition between a cross-origin, same-process
Expand All @@ -6185,13 +6169,11 @@ IN_PROC_BROWSER_TEST_F(
EXPECT_TRUE(NavigateToURL(shell(), same_document_url));
EXPECT_EQ(2, web_contents->GetController().GetEntryCount());

// Create a GoBackAndCommitFilter, which will delay the commit IPC for a
// cross-origin, same process navigation and will perform a GoBack.
// Create a HistoryNavigationBeforeCommitInjector, which will perform a
// GoBack() just before a cross-origin, same process navigation commits.
GURL cross_origin_url(
embedded_test_server()->GetURL("suborigin.a.com", "/title2.html"));
scoped_refptr<GoBackAndCommitFilter> filter =
new GoBackAndCommitFilter(cross_origin_url, web_contents);
web_contents->GetMainFrame()->GetProcess()->AddFilter(filter.get());
HistoryNavigationBeforeCommitInjector trigger(web_contents, cross_origin_url);

// Navigate cross-origin, waiting for the commit to occur.
UrlCommitObserver cross_origin_commit_observer(root, cross_origin_url);
Expand All @@ -6200,6 +6182,7 @@ IN_PROC_BROWSER_TEST_F(
cross_origin_commit_observer.Wait();
EXPECT_EQ(cross_origin_url, web_contents->GetLastCommittedURL());
EXPECT_EQ(2, web_contents->GetController().GetLastCommittedEntryIndex());
EXPECT_TRUE(trigger.did_trigger_history_navigation());

if (IsBrowserSideNavigationEnabled()) {
// With browser-side-navigation, the history navigation is dropped.
Expand Down
10 changes: 5 additions & 5 deletions content/browser/frame_host/navigator_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -947,7 +947,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
EXPECT_EQ(site_instance_id, speculative_rfh->GetSiteInstance()->GetId());
EXPECT_FALSE(node->render_manager()->pending_frame_host());

// Invoke OnDidCommitProvisionalLoad.
// Invoke DidCommitProvisionalLoad.
speculative_rfh->SendNavigate(entry_id, true, kUrl);
EXPECT_EQ(site_instance_id, main_test_rfh()->GetSiteInstance()->GetId());
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
Expand Down Expand Up @@ -1018,7 +1018,7 @@ TEST_F(NavigatorTestWithBrowserSideNavigation,
EXPECT_NE(init_site_instance_id, redirect_site_instance_id);
EXPECT_NE(site_instance_id, redirect_site_instance_id);

// Invoke OnDidCommitProvisionalLoad.
// Invoke DidCommitProvisionalLoad.
speculative_rfh->SendNavigate(entry_id, true, kUrlRedirect);

// Check that the speculative RenderFrameHost was swapped in.
Expand Down Expand Up @@ -1205,9 +1205,9 @@ void SetWithinSameDocument(
}

// A renderer process might try and claim that a cross site navigation was
// within the same document by setting was_within_same_document = true for
// FrameHostMsg_DidCommitProvisionalLoad. Such case should be detected on the
// browser side and the renderer process should be killed.
// within the same document by setting was_within_same_document = true in
// FrameHostMsg_DidCommitProvisionalLoad_Params. Such case should be detected on
// the browser side and the renderer process should be killed.
TEST_F(NavigatorTestWithBrowserSideNavigation, CrossSiteClaimWithinPage) {
const GURL kUrl1("http://www.chromium.org/");
const GURL kUrl2("http://www.google.com/");
Expand Down
Loading

0 comments on commit a40712f

Please sign in to comment.