Skip to content

Commit

Permalink
Revert "Mojofy DidCommitProvisionalLoad."
Browse files Browse the repository at this point in the history 
This reverts commit fa8adf8.

Reason for revert:

Seeing new Win10 failures in navigation_controller_impl_browsertest.cc post this commit. 

Original change's description:
> Mojofy DidCommitProvisionalLoad.
> 
> 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.
> 
> Bug: 729021
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
> Change-Id: I794a8f1cb433a82e2c1d9351630116b584f8fcf9
> Reviewed-on: https://chromium-review.googlesource.com/607607
> Commit-Queue: Balazs Engedy <engedy@chromium.org>
> Reviewed-by: Alexander Timin <altimin@chromium.org>
> Reviewed-by: Ken Rockot <rockot@chromium.org>
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#500294}

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

Change-Id: I2d8798575907a7d195e65ee2bc9fd49e190f84f9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 729021
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Reviewed-on: https://chromium-review.googlesource.com/655142
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Commit-Queue: Roger McFarlane <rogerm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500308}
  • Loading branch information
@rogerm
rogerm authored and Commit Bot committed Sep 7, 2017
1 parent 27dd5a5 commit e0d59a3
Show file tree
Hide file tree
Showing 30 changed files with 270 additions and 477 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.) mojom::FrameHost::DidCommitProvisionalLoad (new load)
// 3.) FrameHostMsg_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 DidCommitProvisionalLoad in Step 3.
// the browser side before FrameHostMsg_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
// DidCommitProvisionalLoad, thus before DidFinishNavigation,
// FrameHostMsg_DidCommitProvisionalLoad, thus before DidFinishNavigation,
ASSERT_TRUE(client->was_store_ever_called());

BubbleObserver prompt_observer(WebContents());
Expand Down
75 changes: 46 additions & 29 deletions content/browser/frame_host/navigation_controller_impl_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
#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 @@ -6100,39 +6099,56 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
EXPECT_EQ(start_url.GetOrigin().spec(), origin + "/");
}

// 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 {
// 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 {
public:
HistoryNavigationBeforeCommitInjector(WebContentsImpl* web_contents,
const GURL& url)
: DidCommitProvisionalLoadInterceptor(web_contents),
did_trigger_history_navigation_(false),
url_(url) {}
~HistoryNavigationBeforeCommitInjector() override {}
GoBackAndCommitFilter(const GURL& url, WebContentsImpl* web_contents)
: BrowserMessageFilter(FrameMsgStart),
url_(url),
web_contents_(web_contents) {}

bool did_trigger_history_navigation() const {
return did_trigger_history_navigation_;
}
protected:
~GoBackAndCommitFilter() override {}

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();
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);
}

// 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;
}

// 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(HistoryNavigationBeforeCommitInjector);
DISALLOW_COPY_AND_ASSIGN(GoBackAndCommitFilter);
};

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

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

// Navigate cross-origin, waiting for the commit to occur.
UrlCommitObserver cross_origin_commit_observer(root, cross_origin_url);
Expand All @@ -6172,7 +6190,6 @@ 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 DidCommitProvisionalLoad.
// Invoke OnDidCommitProvisionalLoad.
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 DidCommitProvisionalLoad.
// Invoke OnDidCommitProvisionalLoad.
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 in
// FrameHostMsg_DidCommitProvisionalLoad_Params. 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 for
// FrameHostMsg_DidCommitProvisionalLoad. 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 e0d59a3

Please sign in to comment.