diff --git a/chrome/browser/password_manager/credential_manager_browsertest.cc b/chrome/browser/password_manager/credential_manager_browsertest.cc index 3e54f06a818cbe..9f655eaa9042bf 100644 --- a/chrome/browser/password_manager/credential_manager_browsertest.cc +++ b/chrome/browser/password_manager/credential_manager_browsertest.cc @@ -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 @@ -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()); @@ -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()); diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc index eb61ca056d88a7..956bed484953d5 100644 --- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc +++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc @@ -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" @@ -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::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 @@ -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 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); @@ -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. diff --git a/content/browser/frame_host/navigator_impl_unittest.cc b/content/browser/frame_host/navigator_impl_unittest.cc index c6d003cc12abf8..4c45871e86b22e 100644 --- a/content/browser/frame_host/navigator_impl_unittest.cc +++ b/content/browser/frame_host/navigator_impl_unittest.cc @@ -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)); @@ -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. @@ -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/"); diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc index 80baeaa1e922b9..5c392bd4940a5f 100644 --- a/content/browser/frame_host/render_frame_host_impl.cc +++ b/content/browser/frame_host/render_frame_host_impl.cc @@ -48,6 +48,7 @@ #include "content/browser/installedapp/installed_app_provider_impl_default.h" #include "content/browser/keyboard_lock/keyboard_lock_service_impl.h" #include "content/browser/loader/resource_dispatcher_host_impl.h" +#include "content/browser/loader/resource_scheduler_filter.h" #include "content/browser/media/media_interface_proxy.h" #include "content/browser/media/session/media_session_service_impl.h" #include "content/browser/payments/payment_app_context_impl.h" @@ -230,7 +231,7 @@ base::i18n::TextDirection WebTextDirectionToChromeTextDirection( } } -// Ensure that we reset nav_entry_id_ in OnDidCommitProvisionalLoad if any of +// Ensure that we reset nav_entry_id_ in DidCommitProvisionalLoad if any of // the validations fail and lead to an early return. Call disable() once we // know the commit will be successful. Resetting nav_entry_id_ avoids acting on // any UpdateState or UpdateTitle messages after an ignored commit. @@ -383,6 +384,23 @@ void CreatePaymentManager(RenderFrameHostImpl* rfh, std::move(request)); } +void NotifyResourceSchedulerOfNavigation( + int render_process_id, + const FrameHostMsg_DidCommitProvisionalLoad_Params& params) { + // TODO(csharrison): This isn't quite right for OOPIF, as we *do* want to + // propagate OnNavigate to the client associated with the OOPIF's RVH. This + // should not result in show-stopping bugs, just poorer loading performance. + if (!ui::PageTransitionIsMainFrame(params.transition) || + params.was_within_same_document) { + return; + } + + BrowserThread::PostTask( + BrowserThread::IO, FROM_HERE, + base::Bind(&ResourceSchedulerFilter::OnDidCommitMainframeNavigation, + render_process_id, params.render_view_routing_id)); +} + } // namespace // static @@ -883,8 +901,6 @@ bool RenderFrameHostImpl::OnMessageReceived(const IPC::Message &msg) { OnDidFailProvisionalLoadWithError) IPC_MESSAGE_HANDLER(FrameHostMsg_DidFailLoadWithError, OnDidFailLoadWithError) - IPC_MESSAGE_HANDLER_GENERIC(FrameHostMsg_DidCommitProvisionalLoad, - OnDidCommitProvisionalLoad(msg)) IPC_MESSAGE_HANDLER(FrameHostMsg_UpdateState, OnUpdateState) IPC_MESSAGE_HANDLER(FrameHostMsg_OpenURL, OnOpenURL) IPC_MESSAGE_HANDLER(FrameHostMsg_CancelInitialHistoryLoad, @@ -1479,28 +1495,23 @@ void RenderFrameHostImpl::OnDidFailLoadWithError( // Called when the renderer navigates. For every frame loaded, we'll get this // notification containing parameters identifying the navigation. -void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) { +void RenderFrameHostImpl::DidCommitProvisionalLoad( + std::unique_ptr + validated_params) { ScopedCommitStateResetter commit_state_resetter(this); RenderProcessHost* process = GetProcess(); - // Read the parameters out of the IPC message directly to avoid making another - // copy when we filter the URLs. - base::PickleIterator iter(msg); - FrameHostMsg_DidCommitProvisionalLoad_Params validated_params; - if (!IPC::ParamTraits:: - Read(&msg, &iter, &validated_params)) { - bad_message::ReceivedBadMessage( - process, bad_message::RFH_COMMIT_DESERIALIZATION_FAILED); - return; - } - TRACE_EVENT2("navigation", "RenderFrameHostImpl::OnDidCommitProvisionalLoad", + TRACE_EVENT2("navigation", "RenderFrameHostImpl::DidCommitProvisionalLoad", "frame_tree_node", frame_tree_node_->frame_tree_node_id(), "url", - validated_params.url.possibly_invalid_spec()); + validated_params->url.possibly_invalid_spec()); // Sanity-check the page transition for frame type. - DCHECK_EQ(ui::PageTransitionIsMainFrame(validated_params.transition), + DCHECK_EQ(ui::PageTransitionIsMainFrame(validated_params->transition), !GetParent()); + // Notify the resource scheduler of the navigation committing. + NotifyResourceSchedulerOfNavigation(process->GetID(), *validated_params); + // If we're waiting for a cross-site beforeunload ack from this renderer and // we receive a Navigate message from the main frame, then the renderer was // navigating already and sent it before hearing the FrameMsg_Stop message. @@ -1521,18 +1532,18 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) { if (IsWaitingForUnloadACK()) return; - if (validated_params.report_type == + if (validated_params->report_type == FrameMsg_UILoadMetricsReportType::REPORT_LINK) { UMA_HISTOGRAM_CUSTOM_TIMES( "Navigation.UI_OnCommitProvisionalLoad.Link", - base::TimeTicks::Now() - validated_params.ui_timestamp, + base::TimeTicks::Now() - validated_params->ui_timestamp, base::TimeDelta::FromMilliseconds(10), base::TimeDelta::FromMinutes(10), 100); - } else if (validated_params.report_type == + } else if (validated_params->report_type == FrameMsg_UILoadMetricsReportType::REPORT_INTENT) { UMA_HISTOGRAM_CUSTOM_TIMES( "Navigation.UI_OnCommitProvisionalLoad.Intent", - base::TimeTicks::Now() - validated_params.ui_timestamp, + base::TimeTicks::Now() - validated_params->ui_timestamp, base::TimeDelta::FromMilliseconds(10), base::TimeDelta::FromMinutes(10), 100); } @@ -1540,8 +1551,8 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) { // Attempts to commit certain off-limits URL should be caught more strictly // than our FilterURL checks below. If a renderer violates this policy, it // should be killed. - if (!CanCommitURL(validated_params.url)) { - VLOG(1) << "Blocked URL " << validated_params.url.spec(); + if (!CanCommitURL(validated_params->url)) { + VLOG(1) << "Blocked URL " << validated_params->url.spec(); // Kills the process. bad_message::ReceivedBadMessage(process, bad_message::RFH_CAN_COMMIT_URL_BLOCKED); @@ -1550,7 +1561,7 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) { // Verify that the origin passed from the renderer process is valid and can // be allowed to commit in this RenderFrameHost. - if (!CanCommitOrigin(validated_params.origin, validated_params.url)) { + if (!CanCommitOrigin(validated_params->origin, validated_params->url)) { bad_message::ReceivedBadMessage(GetProcess(), bad_message::RFH_INVALID_ORIGIN_ON_COMMIT); return; @@ -1563,17 +1574,23 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) { // renderer to load the URL and grant the renderer the privileges to request // the URL. To prevent this attack, we block the renderer from inserting // banned URLs into the navigation controller in the first place. - process->FilterURL(false, &validated_params.url); - process->FilterURL(true, &validated_params.referrer.url); - for (std::vector::iterator it(validated_params.redirects.begin()); - it != validated_params.redirects.end(); ++it) { + // + // TODO(crbug.com/172694): Currently, when FilterURL detects a bad URL coming + // from the renderer, it overwrites that URL to about:blank, which requires + // |validated_params| to be mutable. Once we kill the renderer instead, the + // signature of RenderFrameHostImpl::DidCommitProvisionalLoad can be modified + // to take |validated_params| by const reference. + process->FilterURL(false, &validated_params->url); + process->FilterURL(true, &validated_params->referrer.url); + for (std::vector::iterator it(validated_params->redirects.begin()); + it != validated_params->redirects.end(); ++it) { process->FilterURL(false, &(*it)); } - process->FilterURL(true, &validated_params.searchable_form_url); + process->FilterURL(true, &validated_params->searchable_form_url); // Without this check, the renderer can trick the browser into using // filenames it can't access in a future session restore. - if (!CanAccessFilesOfPageState(validated_params.page_state)) { + if (!CanAccessFilesOfPageState(validated_params->page_state)) { bad_message::ReceivedBadMessage( GetProcess(), bad_message::RFH_CAN_ACCESS_FILES_OF_PAGE_STATE); return; @@ -1594,37 +1611,37 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) { // Find the appropriate NavigationHandle for this navigation. std::unique_ptr navigation_handle = - TakeNavigationHandleForCommit(validated_params); + TakeNavigationHandleForCommit(*validated_params); DCHECK(navigation_handle); // Update the site url if the navigation was successful and the page is not an // interstitial. - if (validated_params.url_is_unreachable || + if (validated_params->url_is_unreachable || delegate_->GetAsInterstitialPage()) { SetLastCommittedSiteUrl(GURL()); } else { - SetLastCommittedSiteUrl(validated_params.url); + SetLastCommittedSiteUrl(validated_params->url); } // PlzNavigate sends searchable form data in the BeginNavigation message // while non-PlzNavigate sends it in the DidCommitProvisionalLoad message. // Update |navigation_handle| if necessary. if (!IsBrowserSideNavigationEnabled() && - !validated_params.searchable_form_url.is_empty()) { + !validated_params->searchable_form_url.is_empty()) { navigation_handle->set_searchable_form_url( - validated_params.searchable_form_url); + validated_params->searchable_form_url); navigation_handle->set_searchable_form_encoding( - validated_params.searchable_form_encoding); + validated_params->searchable_form_encoding); // Reset them so that they are consistent in both the PlzNavigate and // non-PlzNavigate case. Users should use those values from // NavigationHandle. - validated_params.searchable_form_url = GURL(); - validated_params.searchable_form_encoding = std::string(); + validated_params->searchable_form_url = GURL(); + validated_params->searchable_form_encoding = std::string(); } accessibility_reset_count_ = 0; - frame_tree_node()->navigator()->DidNavigate(this, validated_params, + frame_tree_node()->navigator()->DidNavigate(this, *validated_params, std::move(navigation_handle)); // Since we didn't early return, it's safe to keep the commit state. @@ -1638,9 +1655,9 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) { // situation) then we clear it after a while anyway. // See https://crbug.com/497588. if (frame_tree_node_->IsMainFrame() && GetView() && - !validated_params.was_within_same_document) { + !validated_params->was_within_same_document) { RenderWidgetHostImpl::From(GetView()->GetRenderWidgetHost()) - ->StartNewContentRenderingTimeout(validated_params.content_source_id); + ->StartNewContentRenderingTimeout(validated_params->content_source_id); } } @@ -1744,12 +1761,12 @@ void RenderFrameHostImpl::OnBeforeUnloadACK( "FrameTreeNode id", frame_tree_node_->frame_tree_node_id()); // If this renderer navigated while the beforeunload request was in flight, we - // may have cleared this state in OnDidCommitProvisionalLoad, in which case we + // may have cleared this state in DidCommitProvisionalLoad, in which case we // can ignore this message. // However renderer might also be swapped out but we still want to proceed // with navigation, otherwise it would block future navigations. This can // happen when pending cross-site navigation is canceled by a second one just - // before OnDidCommitProvisionalLoad while current RVH is waiting for commit + // before DidCommitProvisionalLoad while current RVH is waiting for commit // but second navigation is started from the beginning. if (!is_waiting_for_beforeunload_ack_) { return; @@ -3076,7 +3093,7 @@ void RenderFrameHostImpl::ResetWaitingState() { // Whenever we reset the RFH state, we should not be waiting for beforeunload // or close acks. We clear them here to be safe, since they can cause - // navigations to be ignored in OnDidCommitProvisionalLoad. + // navigations to be ignored in DidCommitProvisionalLoad. if (is_waiting_for_beforeunload_ack_) { is_waiting_for_beforeunload_ack_ = false; if (beforeunload_timeout_) diff --git a/content/browser/frame_host/render_frame_host_impl.h b/content/browser/frame_host/render_frame_host_impl.h index 97034be2f812f8..c06ab200e8ff27 100644 --- a/content/browser/frame_host/render_frame_host_impl.h +++ b/content/browser/frame_host/render_frame_host_impl.h @@ -663,6 +663,11 @@ class CONTENT_EXPORT RenderFrameHostImpl return stream_handle_.get(); } + // Exposed so that tests can swap out the implementation and intercept calls. + mojo::AssociatedBinding& frame_host_binding_for_testing() { + return frame_host_associated_binding_; + } + protected: friend class RenderFrameHostFactory; @@ -732,7 +737,6 @@ class CONTENT_EXPORT RenderFrameHostImpl void OnDidFailLoadWithError(const GURL& url, int error_code, const base::string16& error_description); - void OnDidCommitProvisionalLoad(const IPC::Message& msg); void OnUpdateState(const PageState& state); void OnBeforeUnloadACK( bool proceed, @@ -841,6 +845,9 @@ class CONTENT_EXPORT RenderFrameHostImpl void CreateNewWindow(mojom::CreateNewWindowParamsPtr params, CreateNewWindowCallback callback) override; void IssueKeepAliveHandle(mojom::KeepAliveHandleRequest request) override; + void DidCommitProvisionalLoad( + std::unique_ptr + validated_params) override; void RunCreateWindowCompleteCallback(CreateNewWindowCallback callback, mojom::CreateNewWindowReplyPtr reply, diff --git a/content/browser/loader/resource_scheduler_filter.cc b/content/browser/loader/resource_scheduler_filter.cc index 3d970572b72672..6852f2202eee47 100644 --- a/content/browser/loader/resource_scheduler_filter.cc +++ b/content/browser/loader/resource_scheduler_filter.cc @@ -8,42 +8,41 @@ #include "content/browser/loader/resource_scheduler.h" #include "content/common/frame_messages.h" #include "ipc/ipc_message_macros.h" -#include "ui/base/page_transition_types.h" namespace content { +// Some tests are lacking a ResourceDispatcherHostImpl. +ResourceScheduler* GetResourceSchedulerOrNullptr() { + if (!ResourceDispatcherHostImpl::Get()) + return nullptr; + return ResourceDispatcherHostImpl::Get()->scheduler(); +} + ResourceSchedulerFilter::ResourceSchedulerFilter(int child_id) : BrowserMessageFilter(FrameMsgStart), child_id_(child_id) {} ResourceSchedulerFilter::~ResourceSchedulerFilter() {} bool ResourceSchedulerFilter::OnMessageReceived(const IPC::Message& message) { - ResourceScheduler* scheduler = ResourceDispatcherHostImpl::Get()->scheduler(); - if (!scheduler) - return false; - IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(ResourceSchedulerFilter, message, scheduler) - IPC_MESSAGE_HANDLER(FrameHostMsg_DidCommitProvisionalLoad, - OnDidCommitProvisionalLoad) + IPC_BEGIN_MESSAGE_MAP(ResourceSchedulerFilter, message) IPC_MESSAGE_HANDLER(FrameHostMsg_WillInsertBody, OnWillInsertBody) IPC_END_MESSAGE_MAP() return false; } -void ResourceSchedulerFilter::OnDidCommitProvisionalLoad( - ResourceScheduler* scheduler, - const FrameHostMsg_DidCommitProvisionalLoad_Params& params) { - // TODO(csharrison): This isn't quite right for OOPIF, as we *do* want to - // propagate OnNavigate to the client associated with the OOPIF's RVH. This - // should not result in show-stopping bugs, just poorer loading performance. - if (ui::PageTransitionIsMainFrame(params.transition) && - !params.was_within_same_document) { - scheduler->OnNavigate(child_id_, params.render_view_routing_id); - } +// static +void ResourceSchedulerFilter::OnDidCommitMainframeNavigation( + int render_process_id, + int render_view_routing_id) { + auto* scheduler = GetResourceSchedulerOrNullptr(); + if (scheduler) + scheduler->OnNavigate(render_process_id, render_view_routing_id); } -void ResourceSchedulerFilter::OnWillInsertBody(ResourceScheduler* scheduler, - int render_view_routing_id) { - scheduler->OnWillInsertBody(child_id_, render_view_routing_id); +void ResourceSchedulerFilter::OnWillInsertBody(int render_view_routing_id) { + auto* scheduler = GetResourceSchedulerOrNullptr(); + if (scheduler) + scheduler->OnWillInsertBody(child_id_, render_view_routing_id); } } // namespace content diff --git a/content/browser/loader/resource_scheduler_filter.h b/content/browser/loader/resource_scheduler_filter.h index 8a6fd065cbbba2..71a7b0098093bf 100644 --- a/content/browser/loader/resource_scheduler_filter.h +++ b/content/browser/loader/resource_scheduler_filter.h @@ -8,12 +8,8 @@ #include "base/macros.h" #include "content/public/browser/browser_message_filter.h" -struct FrameHostMsg_DidCommitProvisionalLoad_Params; - namespace content { -class ResourceScheduler; - // This class listens for incoming ViewHostMsgs that are applicable to the // ResourceScheduler and invokes the appropriate notifications. It must be // inserted before the RenderMessageFilter, because the ResourceScheduler runs @@ -23,17 +19,18 @@ class ResourceSchedulerFilter : public BrowserMessageFilter { public: explicit ResourceSchedulerFilter(int child_id); + // Informs the ResourceScheduler that a main-frame, non-same-document + // navigation has just committed. + static void OnDidCommitMainframeNavigation(int render_process_id, + int render_view_routing_id); + // BrowserMessageFilter: bool OnMessageReceived(const IPC::Message& message) override; private: ~ResourceSchedulerFilter() override; - void OnDidCommitProvisionalLoad( - ResourceScheduler* scheduler, - const FrameHostMsg_DidCommitProvisionalLoad_Params& params); - void OnWillInsertBody(ResourceScheduler* scheduler, - int render_view_routing_id); + void OnWillInsertBody(int render_view_routing_id); int child_id_; diff --git a/content/browser/renderer_host/render_widget_host_view_browsertest.cc b/content/browser/renderer_host/render_widget_host_view_browsertest.cc index 4214b88c787279..33a38a7360ab9f 100644 --- a/content/browser/renderer_host/render_widget_host_view_browsertest.cc +++ b/content/browser/renderer_host/render_widget_host_view_browsertest.cc @@ -34,6 +34,7 @@ #include "content/public/test/content_browser_test_utils.h" #include "content/public/test/test_utils.h" #include "content/shell/browser/shell.h" +#include "content/test/did_commit_provisional_load_interceptor.h" #include "media/base/video_frame.h" #include "media/renderers/skcanvas_video_renderer.h" #include "net/base/filename_util.h" @@ -208,27 +209,22 @@ class RenderWidgetHostViewBrowserTest : public ContentBrowserTest { // Helps to ensure that a navigation is committed after a compositor frame was // submitted by the renderer, but before corresponding ACK is sent back. -class CommitBeforeSwapAckSentHelper : public WebContentsObserver { +class CommitBeforeSwapAckSentHelper + : public DidCommitProvisionalLoadInterceptor { public: explicit CommitBeforeSwapAckSentHelper(WebContents* web_contents) - : WebContentsObserver(web_contents) {} + : DidCommitProvisionalLoadInterceptor(web_contents) {} private: - void WaitForSwapCompositorFrame() { + // DidCommitProvisionalLoadInterceptor: + void WillDispatchDidCommitProvisionalLoad( + RenderFrameHost* render_frame_host, + ::FrameHostMsg_DidCommitProvisionalLoad_Params*) override { base::MessageLoop::ScopedNestableTaskAllower allow( base::MessageLoop::current()); FrameWatcher(web_contents()).WaitFrames(1); } - bool OnMessageReceived(const IPC::Message& message, - RenderFrameHost* rfh) override { - IPC_BEGIN_MESSAGE_MAP(CommitBeforeSwapAckSentHelper, message) - IPC_MESSAGE_HANDLER_GENERIC(FrameHostMsg_DidCommitProvisionalLoad, - WaitForSwapCompositorFrame()) - IPC_END_MESSAGE_MAP() - return false; - } - DISALLOW_COPY_AND_ASSIGN(CommitBeforeSwapAckSentHelper); }; diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc index 434cb9fc8e8306..79c941567a8684 100644 --- a/content/browser/security_exploit_browsertest.cc +++ b/content/browser/security_exploit_browsertest.cc @@ -7,6 +7,7 @@ #include "base/command_line.h" #include "base/containers/hash_tables.h" #include "base/macros.h" +#include "base/memory/ptr_util.h" #include "base/strings/utf_string_conversions.h" #include "build/build_config.h" #include "content/browser/bad_message.h" @@ -548,22 +549,21 @@ IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, MismatchedOriginOnCommit) { // Create commit params with different origins in params.url and // params.origin. - FrameHostMsg_DidCommitProvisionalLoad_Params params; - params.nav_entry_id = 0; - params.did_create_new_entry = false; - params.url = url; - params.transition = ui::PAGE_TRANSITION_LINK; - params.should_update_history = false; - params.gesture = NavigationGestureAuto; - params.was_within_same_document = false; - params.method = "GET"; - params.page_state = PageState::CreateFromURL(url); - params.origin = url::Origin(GURL("http://bar.com/")); - - FrameHostMsg_DidCommitProvisionalLoad msg( - root->current_frame_host()->routing_id(), params); - IPC::IpcSecurityTestUtil::PwnMessageReceived( - root->current_frame_host()->GetProcess()->GetChannel(), msg); + std::unique_ptr params = + base::MakeUnique(); + params->nav_entry_id = 0; + params->did_create_new_entry = false; + params->url = url; + params->transition = ui::PAGE_TRANSITION_LINK; + params->should_update_history = false; + params->gesture = NavigationGestureAuto; + params->was_within_same_document = false; + params->method = "GET"; + params->page_state = PageState::CreateFromURL(url); + params->origin = url::Origin(GURL("http://bar.com/")); + + static_cast(root->current_frame_host()) + ->DidCommitProvisionalLoad(std::move(params)); // When the IPC message is received and validation fails, the process is // terminated. However, the notification for that should be processed in a diff --git a/content/common/BUILD.gn b/content/common/BUILD.gn index ca2c585d219504..1897f0536a994e 100644 --- a/content/common/BUILD.gn +++ b/content/common/BUILD.gn @@ -155,6 +155,7 @@ source_set("common") { "font_list_win.cc", "frame_message_enums.h", "frame_messages.h", + "frame_messages_forward.h", "frame_owner_properties.cc", "frame_owner_properties.h", "frame_replication_state.cc", diff --git a/content/common/frame.mojom b/content/common/frame.mojom index 3cc68cce954513..2960e7c7a0106b 100644 --- a/content/common/frame.mojom +++ b/content/common/frame.mojom @@ -96,6 +96,9 @@ struct CreateNewWindowReply { // the frame is detached. Used by resource requests with "keepalive" specified. interface KeepAliveHandle {}; +[Native] +struct DidCommitProvisionalLoadParams; + // Implemented by the frame server (i.e. the browser process). For messages that // must be associated with the IPC channel. interface FrameHost { @@ -107,4 +110,8 @@ interface FrameHost { // Creates and returns a KeepAliveHandle. IssueKeepAliveHandle(KeepAliveHandle& keep_alive_handle); + + // Sent by the renderer when a navigation commits in the frame. + DidCommitProvisionalLoad( + DidCommitProvisionalLoadParams params); }; diff --git a/content/common/frame_messages.h b/content/common/frame_messages.h index 10f9be2d05ecee..19b5439c918aef 100644 --- a/content/common/frame_messages.h +++ b/content/common/frame_messages.h @@ -229,8 +229,8 @@ IPC_STRUCT_TRAITS_BEGIN(content::FrameNavigateParams) IPC_STRUCT_TRAITS_MEMBER(socket_address) IPC_STRUCT_TRAITS_END() -// Parameters structure for FrameHostMsg_DidCommitProvisionalLoad, which has -// too many data parameters to be reasonably put in a predefined IPC message. +// Parameters structure for mojom::FrameHost::DidCommitProvisionalLoad. +// TODO(https://crbug.com/729021): Convert this to a Mojo struct. IPC_STRUCT_BEGIN_WITH_PARENT(FrameHostMsg_DidCommitProvisionalLoad_Params, content::FrameNavigateParams) IPC_STRUCT_TRAITS_PARENT(content::FrameNavigateParams) @@ -1120,12 +1120,6 @@ IPC_MESSAGE_ROUTED3(FrameHostMsg_DidStartProvisionalLoad, IPC_MESSAGE_ROUTED1(FrameHostMsg_DidFailProvisionalLoadWithError, FrameHostMsg_DidFailProvisionalLoadWithError_Params) -// Notifies the browser that a frame in the view has changed. This message -// has a lot of parameters and is packed/unpacked by functions defined in -// render_messages.h. -IPC_MESSAGE_ROUTED1(FrameHostMsg_DidCommitProvisionalLoad, - FrameHostMsg_DidCommitProvisionalLoad_Params) - // Notifies the browser that a document has been loaded. IPC_MESSAGE_ROUTED0(FrameHostMsg_DidFinishDocumentLoad) diff --git a/content/common/frame_messages.typemap b/content/common/frame_messages.typemap new file mode 100644 index 00000000000000..49385119f816c1 --- /dev/null +++ b/content/common/frame_messages.typemap @@ -0,0 +1,8 @@ +# Copyright 2017 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +mojom = "//content/common/frame.mojom" +public_headers = [ "//content/common/frame_messages_forward.h" ] +traits_headers = [ "//content/common/frame_messages.h" ] +type_mappings = [ "content.mojom.DidCommitProvisionalLoadParams=std::unique_ptr<::FrameHostMsg_DidCommitProvisionalLoad_Params>[move_only,nullable_is_same_type]" ] diff --git a/content/common/frame_messages_forward.h b/content/common/frame_messages_forward.h new file mode 100644 index 00000000000000..3e83e199d73fc4 --- /dev/null +++ b/content/common/frame_messages_forward.h @@ -0,0 +1,25 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CONTENT_COMMON_FRAME_MESSAGES_FORWARD_H_ +#define CONTENT_COMMON_FRAME_MESSAGES_FORWARD_H_ + +// Forward-declaration for FrameHostMsg_DidCommitProvisionalLoad_Params, which +// is typemapped to a Mojo [Native] type used by content::mojom::FrameHost. This +// means that the generated header for mojom::FrameHost requires (at least) a +// forward declaration of the legacy IPC struct type. +// +// Including content/common/frame_messages.h in the generated header for +// mojom::FrameHost is, however, not possible because legacy IPC struct type +// definition headers must be included exactly once in each translation unit +// using them, so any .cc files directly/indirectly including frame_messages.h +// could no longer include the generated header for mojom::FrameHost. Hence the +// generated header for mojom::FrameHost does not include frame_messages.h, but +// instead includes the forward-declaration below. +// +// TODO(https://crbug.com/729021): Eventually convert this legacy IPC struct to +// a proper Mojo type. +struct FrameHostMsg_DidCommitProvisionalLoad_Params; + +#endif // CONTENT_COMMON_FRAME_MESSAGES_FORWARD_H_ diff --git a/content/common/navigation_params.h b/content/common/navigation_params.h index a73ae233c75eeb..fc5936faf9985c 100644 --- a/content/common/navigation_params.h +++ b/content/common/navigation_params.h @@ -310,7 +310,7 @@ struct CONTENT_EXPORT RequestNavigationParams { // For browser-initiated navigations, this is the unique id of the // NavigationEntry being navigated to. (For renderer-initiated navigations it // is 0.) If the load succeeds, then this nav_entry_id will be reflected in - // the resulting FrameHostMsg_DidCommitProvisionalLoad message. + // the resulting FrameHostMsg_DidCommitProvisionalLoad_Params. int nav_entry_id; // Whether this is a history navigation in a newly created child frame, in diff --git a/content/common/typemaps.gni b/content/common/typemaps.gni index c5664d19a95db8..8ac58a5c262dac 100644 --- a/content/common/typemaps.gni +++ b/content/common/typemaps.gni @@ -4,6 +4,7 @@ typemaps = [ "//content/common/background_fetch/background_fetch_types.typemap", + "//content/common/frame_messages.typemap", "//content/common/native_types.typemap", "//content/common/media/media_devices.typemap", "//content/common/media/media_stream.typemap", diff --git a/content/public/test/test_renderer_host.h b/content/public/test/test_renderer_host.h index b1003163a08c76..2ef6e8922dea31 100644 --- a/content/public/test/test_renderer_host.h +++ b/content/public/test/test_renderer_host.h @@ -83,7 +83,7 @@ class RenderFrameHostTester { // Simulates a navigation stopping in the RenderFrameHost. virtual void SimulateNavigationStop() = 0; - // Calls OnDidCommitProvisionalLoad on the RenderFrameHost with the given + // Calls DidCommitProvisionalLoad on the RenderFrameHost with the given // information with various sets of parameters. These are helper functions for // simulating the most common types of loads. // diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc index 5af21cc0992dc7..085a35c4b8e655 100644 --- a/content/renderer/render_frame_impl.cc +++ b/content/renderer/render_frame_impl.cc @@ -1250,10 +1250,10 @@ RenderFrameImpl::RenderFrameImpl(const CreateParams& params) sizeof(RenderThreadImpl::RendererMemoryMetrics)); } -mojom::FrameHostAssociatedPtr RenderFrameImpl::GetFrameHost() { - mojom::FrameHostAssociatedPtr frame_host_ptr; - GetRemoteAssociatedInterfaces()->GetInterface(&frame_host_ptr); - return frame_host_ptr; +mojom::FrameHost* RenderFrameImpl::GetFrameHost() { + if (!frame_host_ptr_.is_bound()) + GetRemoteAssociatedInterfaces()->GetInterface(&frame_host_ptr_); + return frame_host_ptr_.get(); } RenderFrameImpl::~RenderFrameImpl() { @@ -4947,17 +4947,18 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad( high_media_engagement_origin_ = url::Origin(); } - FrameHostMsg_DidCommitProvisionalLoad_Params params; - params.http_status_code = response.HttpStatusCode(); - params.url_is_unreachable = document_loader->HasUnreachableURL(); - params.method = "GET"; - params.intended_as_new_entry = + std::unique_ptr params = + base::MakeUnique(); + params->http_status_code = response.HttpStatusCode(); + params->url_is_unreachable = document_loader->HasUnreachableURL(); + params->method = "GET"; + params->intended_as_new_entry = navigation_state->request_params().intended_as_new_entry; - params.did_create_new_entry = commit_type == blink::kWebStandardCommit; - params.should_replace_current_entry = + params->did_create_new_entry = commit_type == blink::kWebStandardCommit; + params->should_replace_current_entry = document_loader->ReplacesCurrentHistoryItem(); - params.post_id = -1; - params.nav_entry_id = navigation_state->request_params().nav_entry_id; + params->post_id = -1; + params->nav_entry_id = navigation_state->request_params().nav_entry_id; // We need to track the RenderViewHost routing_id because of downstream // dependencies (crbug.com/392171 DownloadRequestHandle, SaveFileManager, // ResourceDispatcherHostImpl, MediaStreamUIProxy, @@ -4965,61 +4966,62 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad( // based on the ID stored in the resource requests. Once those dependencies // are unwound or moved to RenderFrameHost (crbug.com/304341) we can move the // client to be based on the routing_id of the RenderFrameHost. - params.render_view_routing_id = render_view_->routing_id(); - params.socket_address.set_host(response.RemoteIPAddress().Utf8()); - params.socket_address.set_port(response.RemotePort()); - params.was_within_same_document = navigation_state->WasWithinSameDocument(); + params->render_view_routing_id = render_view_->routing_id(); + params->socket_address.set_host(response.RemoteIPAddress().Utf8()); + params->socket_address.set_port(response.RemotePort()); + params->was_within_same_document = navigation_state->WasWithinSameDocument(); WebDocument frame_document = frame->GetDocument(); // Set the origin of the frame. This will be replicated to the corresponding // RenderFrameProxies in other processes. WebSecurityOrigin frame_origin = frame_document.GetSecurityOrigin(); - params.origin = frame_origin; + params->origin = frame_origin; - params.insecure_request_policy = frame->GetInsecureRequestPolicy(); + params->insecure_request_policy = frame->GetInsecureRequestPolicy(); - params.has_potentially_trustworthy_unique_origin = + params->has_potentially_trustworthy_unique_origin = frame_origin.IsUnique() && frame_origin.IsPotentiallyTrustworthy(); // Set the URL to be displayed in the browser UI to the user. - params.url = GetLoadingUrl(); - if (GURL(frame_document.BaseURL()) != params.url) - params.base_url = frame_document.BaseURL(); + params->url = GetLoadingUrl(); + if (GURL(frame_document.BaseURL()) != params->url) + params->base_url = frame_document.BaseURL(); - GetRedirectChain(document_loader, ¶ms.redirects); - params.should_update_history = + GetRedirectChain(document_loader, ¶ms->redirects); + params->should_update_history = !document_loader->HasUnreachableURL() && response.HttpStatusCode() != 404; - params.searchable_form_url = internal_data->searchable_form_url(); - params.searchable_form_encoding = internal_data->searchable_form_encoding(); + params->searchable_form_url = internal_data->searchable_form_url(); + params->searchable_form_encoding = internal_data->searchable_form_encoding(); - params.gesture = render_view_->navigation_gesture_; + params->gesture = render_view_->navigation_gesture_; render_view_->navigation_gesture_ = NavigationGestureUnknown; // Make navigation state a part of the DidCommitProvisionalLoad message so // that committed entry has it at all times. Send a single HistoryItem for // this frame, rather than the whole tree. It will be stored in the // corresponding FrameNavigationEntry. - params.page_state = SingleHistoryItemToPageState(current_history_item_); + params->page_state = SingleHistoryItemToPageState(current_history_item_); - params.content_source_id = GetRenderWidget()->GetContentSourceId(); + params->content_source_id = GetRenderWidget()->GetContentSourceId(); - params.method = request.HttpMethod().Latin1(); - if (params.method == "POST") - params.post_id = ExtractPostId(current_history_item_); + params->method = request.HttpMethod().Latin1(); + if (params->method == "POST") + params->post_id = ExtractPostId(current_history_item_); - params.frame_unique_name = current_history_item_.Target().Utf8(); - params.item_sequence_number = current_history_item_.ItemSequenceNumber(); - params.document_sequence_number = + params->frame_unique_name = current_history_item_.Target().Utf8(); + params->item_sequence_number = current_history_item_.ItemSequenceNumber(); + params->document_sequence_number = current_history_item_.DocumentSequenceNumber(); // If the page contained a client redirect (meta refresh, document.loc...), // set the referrer appropriately. if (document_loader->IsClientRedirect()) { - params.referrer = Referrer( - params.redirects[0], document_loader->GetRequest().GetReferrerPolicy()); + params->referrer = + Referrer(params->redirects[0], + document_loader->GetRequest().GetReferrerPolicy()); } else { - params.referrer = RenderViewImpl::GetReferrerFromRequest( + params->referrer = RenderViewImpl::GetReferrerFromRequest( frame, document_loader->GetRequest()); } @@ -5060,33 +5062,34 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad( } // Update contents MIME type for main frame. - params.contents_mime_type = + params->contents_mime_type = document_loader->GetResponse().MimeType().Utf8(); - params.transition = navigation_state->GetTransitionType(); - DCHECK(ui::PageTransitionIsMainFrame(params.transition)); + params->transition = navigation_state->GetTransitionType(); + DCHECK(ui::PageTransitionIsMainFrame(params->transition)); // If the page contained a client redirect (meta refresh, document.loc...), // set the transition appropriately. if (document_loader->IsClientRedirect()) { - params.transition = ui::PageTransitionFromInt( - params.transition | ui::PAGE_TRANSITION_CLIENT_REDIRECT); + params->transition = ui::PageTransitionFromInt( + params->transition | ui::PAGE_TRANSITION_CLIENT_REDIRECT); } // Send the user agent override back. - params.is_overriding_user_agent = internal_data->is_overriding_user_agent(); + params->is_overriding_user_agent = + internal_data->is_overriding_user_agent(); // Track the URL of the original request. We use the first entry of the // redirect chain if it exists because the chain may have started in another // process. - params.original_request_url = GetOriginalRequestURL(document_loader); + params->original_request_url = GetOriginalRequestURL(document_loader); - params.history_list_was_cleared = + params->history_list_was_cleared = navigation_state->request_params().should_clear_history_list; - params.report_type = static_cast( + params->report_type = static_cast( frame->GetDocumentLoader()->GetRequest().InputPerfMetricReportPolicy()); - params.ui_timestamp = + params->ui_timestamp = base::TimeTicks() + base::TimeDelta::FromSecondsD( frame->GetDocumentLoader()->GetRequest().UiStartTime()); @@ -5096,13 +5099,13 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad( // history entry, it means the user initiated the navigation and we should // mark it as such. if (commit_type == blink::kWebStandardCommit) - params.transition = ui::PAGE_TRANSITION_MANUAL_SUBFRAME; + params->transition = ui::PAGE_TRANSITION_MANUAL_SUBFRAME; else - params.transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME; + params->transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME; DCHECK(!navigation_state->request_params().should_clear_history_list); - params.history_list_was_cleared = false; - params.report_type = FrameMsg_UILoadMetricsReportType::NO_REPORT; + params->history_list_was_cleared = false; + params->report_type = FrameMsg_UILoadMetricsReportType::NO_REPORT; // Subframes should match the zoom level of the main frame. render_view_->SetZoomLevel(render_view_->page_zoom_level()); } @@ -5110,22 +5113,22 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad( // Standard URLs must match the reported origin, when it is not unique. // This check is very similar to RenderFrameHostImpl::CanCommitOrigin, but // adapted to the renderer process side. - if (!params.origin.unique() && params.url.IsStandard() && + if (!params->origin.unique() && params->url.IsStandard() && render_view_->GetWebkitPreferences().web_security_enabled) { // Exclude file: URLs when settings allow them access any origin. - if (params.origin.scheme() != url::kFileScheme || + if (params->origin.scheme() != url::kFileScheme || !render_view_->GetWebkitPreferences() .allow_universal_access_from_file_urls) { - CHECK(params.origin.IsSamePhysicalOriginWith(url::Origin(params.url))) - << " url:" << params.url << " origin:" << params.origin; + CHECK(params->origin.IsSamePhysicalOriginWith(url::Origin(params->url))) + << " url:" << params->url << " origin:" << params->origin; } } - // This message needs to be sent before any of allowScripts(), - // allowImages(), allowPlugins() is called for the new page, so that when - // these functions send a ViewHostMsg_ContentBlocked message, it arrives - // after the FrameHostMsg_DidCommitProvisionalLoad message. - Send(new FrameHostMsg_DidCommitProvisionalLoad(routing_id_, params)); + // This invocation must precede any calls to allowScripts(), allowImages(), or + // allowPlugins() for the new page. This ensures that when these functions + // send ViewHostMsg_ContentBlocked messages, those arrive after the browser + // process has already been informed of the provisional load committing. + GetFrameHost()->DidCommitProvisionalLoad(std::move(params)); // If we end up reusing this WebRequest (for example, due to a #ref click), // we don't want the transition type to persist. Just clear it. diff --git a/content/renderer/render_frame_impl.h b/content/renderer/render_frame_impl.h index 329950c7c0c1e4..449d2f949d701d 100644 --- a/content/renderer/render_frame_impl.h +++ b/content/renderer/render_frame_impl.h @@ -712,8 +712,8 @@ class CONTENT_EXPORT RenderFrameImpl mojom::FrameRequest request, mojom::FrameHostInterfaceBrokerPtr frame_host); - // Virtual so the test render frame can flush the interface. - virtual mojom::FrameHostAssociatedPtr GetFrameHost(); + // Virtual so that a TestRenderFrame can mock out the interface. + virtual mojom::FrameHost* GetFrameHost(); void BindFrameBindingsControl( mojom::FrameBindingsControlAssociatedRequest request); @@ -1480,6 +1480,7 @@ class CONTENT_EXPORT RenderFrameImpl service_manager::BindSourceInfo browser_info_; + mojom::FrameHostAssociatedPtr frame_host_ptr_; mojo::BindingSet interface_provider_bindings_; diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc index b339b5a4d8d978..963e4fb74124ed 100644 --- a/content/renderer/render_view_browsertest.cc +++ b/content/renderer/render_view_browsertest.cc @@ -617,20 +617,14 @@ TEST_F(RenderViewImplTest, OnNavigationHttpPost) { frame()->Navigate(common_params, start_params, request_params); base::RunLoop().RunUntilIdle(); - const IPC::Message* frame_navigate_msg = - render_thread_->sink().GetUniqueMessageMatching( - FrameHostMsg_DidCommitProvisionalLoad::ID); - EXPECT_TRUE(frame_navigate_msg); - - FrameHostMsg_DidCommitProvisionalLoad::Param host_nav_params; - FrameHostMsg_DidCommitProvisionalLoad::Read(frame_navigate_msg, - &host_nav_params); - EXPECT_EQ("POST", std::get<0>(host_nav_params).method); + auto last_commit_params = frame()->TakeLastCommitParams(); + ASSERT_TRUE(last_commit_params); + EXPECT_EQ("POST", last_commit_params->method); // Check post data sent to browser matches - EXPECT_TRUE(std::get<0>(host_nav_params).page_state.IsValid()); + EXPECT_TRUE(last_commit_params->page_state.IsValid()); std::unique_ptr entry = - PageStateToHistoryEntry(std::get<0>(host_nav_params).page_state); + PageStateToHistoryEntry(last_commit_params->page_state); blink::WebHTTPBody body = entry->root().HttpBody(); blink::WebHTTPBody::Element element; bool successful = body.ElementAt(0, element); diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc index 853d6f3e125222..2c088141cc3206 100644 --- a/content/renderer/render_view_impl.cc +++ b/content/renderer/render_view_impl.cc @@ -1356,8 +1356,8 @@ WebView* RenderViewImpl::CreateView(WebLocalFrame* creator, bool opened_by_user_gesture = params->user_gesture; mojom::CreateNewWindowReplyPtr reply; - mojom::FrameHostAssociatedPtr frame_host_ptr = creator_frame->GetFrameHost(); - bool err = !frame_host_ptr->CreateNewWindow(std::move(params), &reply); + auto* frame_host = creator_frame->GetFrameHost(); + bool err = !frame_host->CreateNewWindow(std::move(params), &reply); if (err || reply->route_id == MSG_ROUTING_NONE) return nullptr; diff --git a/content/test/BUILD.gn b/content/test/BUILD.gn index a081b21725a343..2b25c8cbd5ad4c 100644 --- a/content/test/BUILD.gn +++ b/content/test/BUILD.gn @@ -167,6 +167,8 @@ static_library("test_support") { "content_browser_sanity_checker.h", "content_test_suite.cc", "content_test_suite.h", + "did_commit_provisional_load_interceptor.cc", + "did_commit_provisional_load_interceptor.h", "dummy_render_widget_host_delegate.h", "dwrite_font_fake_sender_win.cc", "dwrite_font_fake_sender_win.h", diff --git a/content/test/did_commit_provisional_load_interceptor.cc b/content/test/did_commit_provisional_load_interceptor.cc new file mode 100644 index 00000000000000..4a8484399aefc4 --- /dev/null +++ b/content/test/did_commit_provisional_load_interceptor.cc @@ -0,0 +1,80 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "content/test/did_commit_provisional_load_interceptor.h" + +#include "base/memory/ptr_util.h" +#include "content/browser/frame_host/render_frame_host_impl.h" +#include "content/common/frame_messages.h" +#include "content/public/browser/render_frame_host.h" +#include "content/public/browser/web_contents.h" + +namespace content { + +// Responsible for intercepting DidCommitProvisionalLoad's being disptached to +// a given RenderFrameHostImpl. +class DidCommitProvisionalLoadInterceptor::FrameAgent + : public mojom::FrameHostInterceptorForTesting { + public: + FrameAgent(DidCommitProvisionalLoadInterceptor* interceptor, + RenderFrameHost* rfh) + : interceptor_(interceptor), + rfhi_(static_cast(rfh)), + impl_(binding().SwapImplForTesting(this)) {} + + ~FrameAgent() override { + auto* old_impl = binding().SwapImplForTesting(impl_); + // TODO(https://crbug.com/729021): Investigate the scenario where + // |old_impl| can be nullptr if the renderer process is killed. + DCHECK_EQ(this, old_impl); + } + + protected: + mojo::AssociatedBinding& binding() { + return rfhi_->frame_host_binding_for_testing(); + } + + // mojom::FrameHostInterceptorForTesting: + FrameHost* GetForwardingInterface() override { return impl_; } + void DidCommitProvisionalLoad( + std::unique_ptr<::FrameHostMsg_DidCommitProvisionalLoad_Params> params) + override { + interceptor_->WillDispatchDidCommitProvisionalLoad(rfhi_, params.get()); + GetForwardingInterface()->DidCommitProvisionalLoad(std::move(params)); + } + + private: + DidCommitProvisionalLoadInterceptor* interceptor_; + + RenderFrameHostImpl* rfhi_; + mojom::FrameHost* impl_; + + DISALLOW_COPY_AND_ASSIGN(FrameAgent); +}; + +DidCommitProvisionalLoadInterceptor::DidCommitProvisionalLoadInterceptor( + WebContents* web_contents) + : WebContentsObserver(web_contents) { + for (auto* rfh : web_contents->GetAllFrames()) + RenderFrameCreated(rfh); +} + +DidCommitProvisionalLoadInterceptor::~DidCommitProvisionalLoadInterceptor() = + default; + +void DidCommitProvisionalLoadInterceptor::RenderFrameCreated( + RenderFrameHost* render_frame_host) { + bool did_insert; + std::tie(std::ignore, did_insert) = frame_agents_.emplace( + render_frame_host, base::MakeUnique(this, render_frame_host)); + DCHECK(did_insert); +} + +void DidCommitProvisionalLoadInterceptor::RenderFrameDeleted( + RenderFrameHost* render_frame_host) { + bool did_remove = !!frame_agents_.erase(render_frame_host); + DCHECK(did_remove); +} + +} // namespace content diff --git a/content/test/did_commit_provisional_load_interceptor.h b/content/test/did_commit_provisional_load_interceptor.h new file mode 100644 index 00000000000000..2a04624762de89 --- /dev/null +++ b/content/test/did_commit_provisional_load_interceptor.h @@ -0,0 +1,50 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CONTENT_TEST_DID_COMMIT_PROVISIONAL_LOAD_INTERCEPTOR_H_ +#define CONTENT_TEST_DID_COMMIT_PROVISIONAL_LOAD_INTERCEPTOR_H_ + +#include +#include + +#include "base/macros.h" +#include "content/common/frame.mojom.h" +#include "content/public/browser/web_contents_observer.h" + +namespace content { + +class RenderFrameHost; + +// Allows intercepting calls to RFHI::DidCommitProvisionalLoad just before they +// are dispatched to the implementation. This enables unit/browser tests to +// scrutinize/alter the parameters, or simulate race conditions by triggering +// other calls just before dispatching DidCommitProvisionalLoad. +class DidCommitProvisionalLoadInterceptor : public WebContentsObserver { + public: + // Constructs an instance that will intercept DidCommitProvisionalLoad calls + // in any frame of the |web_contents| while the instance is in scope. + explicit DidCommitProvisionalLoadInterceptor(WebContents* web_contents); + ~DidCommitProvisionalLoadInterceptor() override; + + // Called just before DidCommitProvisionalLoad with |params| would be + // dispatched to |render_frame_host|. + virtual void WillDispatchDidCommitProvisionalLoad( + RenderFrameHost* render_frame_host, + ::FrameHostMsg_DidCommitProvisionalLoad_Params* params) = 0; + + private: + class FrameAgent; + + // WebContentsObserver: + void RenderFrameCreated(RenderFrameHost* render_frame_host) override; + void RenderFrameDeleted(RenderFrameHost* render_frame_host) override; + + std::map> frame_agents_; + + DISALLOW_COPY_AND_ASSIGN(DidCommitProvisionalLoadInterceptor); +}; + +} // namespace content + +#endif // CONTENT_TEST_DID_COMMIT_PROVISIONAL_LOAD_INTERCEPTOR_H_ diff --git a/content/test/test_render_frame.cc b/content/test/test_render_frame.cc index d9005b194d29dd..d499cea95cf759 100644 --- a/content/test/test_render_frame.cc +++ b/content/test/test_render_frame.cc @@ -19,26 +19,41 @@ namespace content { class MockFrameHost : public mojom::FrameHost { public: - MockFrameHost() : binding_(this) {} + MockFrameHost() {} ~MockFrameHost() override = default; - void CreateNewWindow(mojom::CreateNewWindowParamsPtr params, - CreateNewWindowCallback callback) override { - mojom::CreateNewWindowReplyPtr reply = mojom::CreateNewWindowReply::New(); + std::unique_ptr + TakeLastCommitParams() { + return std::move(last_commit_params_); + } + + protected: + // mojom::FrameHost: + void CreateNewWindow(mojom::CreateNewWindowParamsPtr, + CreateNewWindowCallback) override { + NOTREACHED() << "We should never dispatch to the service side signature."; + } + + bool CreateNewWindow(mojom::CreateNewWindowParamsPtr params, + mojom::CreateNewWindowReplyPtr* reply) override { + *reply = mojom::CreateNewWindowReply::New(); MockRenderThread* mock_render_thread = static_cast(RenderThread::Get()); - mock_render_thread->OnCreateWindow(*params, reply.get()); - std::move(callback).Run(std::move(reply)); + mock_render_thread->OnCreateWindow(*params, reply->get()); + return true; } void IssueKeepAliveHandle(mojom::KeepAliveHandleRequest request) override {} - void Bind(mojo::ScopedInterfaceEndpointHandle handle) { - binding_.Bind(mojom::FrameHostAssociatedRequest(std::move(handle))); + void DidCommitProvisionalLoad( + std::unique_ptr params) + override { + last_commit_params_ = std::move(params); } private: - mojo::AssociatedBinding binding_; + std::unique_ptr + last_commit_params_; DISALLOW_COPY_AND_ASSIGN(MockFrameHost); }; @@ -52,14 +67,9 @@ RenderFrameImpl* TestRenderFrame::CreateTestRenderFrame( TestRenderFrame::TestRenderFrame(const RenderFrameImpl::CreateParams& params) : RenderFrameImpl(params), mock_frame_host_(base::MakeUnique()) { - GetRemoteAssociatedInterfaces()->OverrideBinderForTesting( - mojom::FrameHost::Name_, - base::Bind(&MockFrameHost::Bind, - base::Unretained(mock_frame_host_.get()))); } -TestRenderFrame::~TestRenderFrame() { -} +TestRenderFrame::~TestRenderFrame() {} void TestRenderFrame::Navigate(const CommonNavigationParams& common_params, const StartNavigationParams& start_params, @@ -132,12 +142,35 @@ std::unique_ptr TestRenderFrame::CreateURLLoader( nullptr, base::ThreadTaskRunnerHandle::Get(), nullptr); } -mojom::FrameHostAssociatedPtr TestRenderFrame::GetFrameHost() { - mojom::FrameHostAssociatedPtr ptr = RenderFrameImpl::GetFrameHost(); +std::unique_ptr +TestRenderFrame::TakeLastCommitParams() { + return mock_frame_host_->TakeLastCommitParams(); +} - // Needed to ensure no deadlocks when waiting for sync IPC. - ptr.FlushForTesting(); - return ptr; +mojom::FrameHost* TestRenderFrame::GetFrameHost() { + // Need to mock this interface directly without going through a binding, + // otherwise calling its sync methods could lead to a deadlock. + // + // Imagine the following sequence of events take place: + // + // 1.) GetFrameHost() called for the first time + // 1.1.) GetRemoteAssociatedInterfaces()->GetInterface(&frame_host_ptr_) + // 1.1.1) ... plumbing ... + // 1.1.2) Task posted to bind the request end to the Mock implementation + // 1.2) The interface pointer end is returned to the caller + // 2.) GetFrameHost()->CreateNewWindow(...) sync method invoked + // 2.1.) Mojo sync request sent + // 2.2.) Waiting for sync response while dispatching incoming sync requests + // + // Normally the sync Mojo request would be processed in 2.2. However, the + // implementation is not yet bound at that point, and will never be, because + // only sync IPCs are dispatched by 2.2, not posted tasks. So the sync request + // is never dispatched, the response never arrives. + // + // Because the first invocation to GetFrameHost() may come while we are inside + // a message loop already, pumping messags before 1.2 would constitute a + // nested message loop and is therefore undesired. + return mock_frame_host_.get(); } } // namespace content diff --git a/content/test/test_render_frame.h b/content/test/test_render_frame.h index 2391218ee01a69..5e0764e4975a39 100644 --- a/content/test/test_render_frame.h +++ b/content/test/test_render_frame.h @@ -58,12 +58,14 @@ class TestRenderFrame : public RenderFrameImpl { const blink::WebURLRequest& request, scoped_refptr task_runner) override; - mojom::FrameHostAssociatedPtr GetFrameHost() override; + std::unique_ptr + TakeLastCommitParams(); private: - void BindFrameHost(mojo::ScopedInterfaceEndpointHandle handle); explicit TestRenderFrame(const RenderFrameImpl::CreateParams& params); + mojom::FrameHost* GetFrameHost() override; + std::unique_ptr mock_frame_host_; DISALLOW_COPY_AND_ASSIGN(TestRenderFrame); diff --git a/content/test/test_render_frame_host.cc b/content/test/test_render_frame_host.cc index ae29c1a3dea20d..8d477a430bcfb3 100644 --- a/content/test/test_render_frame_host.cc +++ b/content/test/test_render_frame_host.cc @@ -418,8 +418,8 @@ void TestRenderFrameHost::SendNavigateWithParams( std::string("Content-Type: ") + contents_mime_type_); navigation_handle()->set_response_headers_for_testing(response_headers); } - FrameHostMsg_DidCommitProvisionalLoad msg(GetRoutingID(), *params); - OnDidCommitProvisionalLoad(msg); + DidCommitProvisionalLoad( + base::MakeUnique(*params)); last_commit_was_error_page_ = params->url_is_unreachable; } diff --git a/extensions/common/extension_messages.h b/extensions/common/extension_messages.h index 450b5c9f7430e1..8093a5fbb04111 100644 --- a/extensions/common/extension_messages.h +++ b/extensions/common/extension_messages.h @@ -881,8 +881,8 @@ IPC_MESSAGE_CONTROL2(ExtensionHostMsg_AddDOMActionToActivityLog, // // * ExtensionMsg_WatchPages was received, updating the set of conditions. // * A new page is loaded. This will be sent after -// FrameHostMsg_DidCommitProvisionalLoad. Currently this only fires for the -// main frame. +// mojom::FrameHost::DidCommitProvisionalLoad. Currently this only fires for +// the main frame. // * Something changed on an existing frame causing the set of matching searches // to change. IPC_MESSAGE_ROUTED1(ExtensionHostMsg_OnWatchedPageChange, diff --git a/mojo/public/cpp/bindings/associated_binding.h b/mojo/public/cpp/bindings/associated_binding.h index c34e971b23a587..c100a1dd42390e 100644 --- a/mojo/public/cpp/bindings/associated_binding.h +++ b/mojo/public/cpp/bindings/associated_binding.h @@ -135,6 +135,13 @@ class AssociatedBinding : public AssociatedBindingBase { // Returns the interface implementation that was previously specified. Interface* impl() { return ImplRefTraits::GetRawPointer(&stub_.sink()); } + // Allows test code to swap the interface implementation. + ImplPointerType SwapImplForTesting(ImplPointerType new_impl) { + Interface* old_impl = impl(); + stub_.set_sink(std::move(new_impl)); + return old_impl; + } + private: typename Interface::template Stub_ stub_; diff --git a/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc b/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc index 01baee56f33c2c..675958d132421b 100644 --- a/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc +++ b/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc @@ -1713,7 +1713,7 @@ void RendererSchedulerImpl::DidCommitProvisionalLoad( bool is_reload, bool is_main_frame) { TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("renderer.scheduler"), - "RendererSchedulerImpl::OnDidCommitProvisionalLoad"); + "RendererSchedulerImpl::DidCommitProvisionalLoad"); // Initialize |max_queueing_time_metric| lazily so that // |SingleSampleMetricsFactory::SetFactory()| is called before // |SingleSampleMetricsFactory::Get()|