Add an API for adding browser-side UseCounters to //content

This CL:
1. Adds method to ContentBrowserClient which allows logging blink
   UseCounters scoped to a given RenderFrameHost.

2. Uses this new API to log UseCounters for opener navigations resulting
   in downloads.

Bug: 632514
Change-Id: I4459281c5a104fac776a13f9f003b1d75349354f
Reviewed-on: https://chromium-review.googlesource.com/c/1324244
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: Luna Lu <loonybear@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613351}

chrome/browser/chrome_content_browser_client.cc

@@ -66,6 +66,7 @@
 #include "chrome/browser/net_benchmarking.h"
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
 #include "chrome/browser/page_load_metrics/metrics_navigation_throttle.h"
+#include "chrome/browser/page_load_metrics/metrics_web_contents_observer.h"
 #include "chrome/browser/page_load_metrics/page_load_metrics_util.h"
 #include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "chrome/browser/payments/payment_request_display_manager_factory.h"
@@ -5279,3 +5280,12 @@ content::PreviewsState ChromeContentBrowserClient::DetermineCommittedPreviews(
 
   return committed_state;
 }
+
+void ChromeContentBrowserClient::LogWebFeatureForCurrentPage(
+    content::RenderFrameHost* render_frame_host,
+    blink::mojom::WebFeature feature) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  page_load_metrics::mojom::PageLoadFeatures new_features({feature}, {}, {});
+  page_load_metrics::MetricsWebContentsObserver::RecordFeatureUsage(
+      render_frame_host, new_features);
+}

chrome/browser/chrome_content_browser_client.h

@@ -541,6 +541,8 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
       content::PreviewsState initial_state,
       content::NavigationHandle* navigation_handle,
       const net::HttpResponseHeaders* response_headers) override;
+  void LogWebFeatureForCurrentPage(content::RenderFrameHost* render_frame_host,
+                                   blink::mojom::WebFeature feature) override;
 
   // Determines the committed previews state for the passed in params.
   static content::PreviewsState DetermineCommittedPreviewsForURL(

chrome/browser/chrome_navigation_browsertest.cc

@@ -5,6 +5,7 @@
 #include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/run_loop.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/test_timeouts.h"
 #include "build/build_config.h"
@@ -16,11 +17,14 @@
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_features.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/network_session_configurator/common/network_switches.h"
+#include "components/prefs/pref_service.h"
 #include "components/url_formatter/url_formatter.h"
+#include "content/public/browser/download_manager_delegate.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/notification_service.h"
@@ -33,6 +37,7 @@
 #include "content/public/common/context_menu_params.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/download_test_observer.h"
 #include "content/public/test/navigation_handle_observer.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/url_loader_interceptor.h"
@@ -55,6 +60,10 @@ class ChromeNavigationBrowserTest : public InProcessBrowserTest {
     ASSERT_TRUE(embedded_test_server()->Start());
   }
 
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+  }
+
   void StartServerWithExpiredCert() {
     expired_https_server_.reset(
         new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS));
@@ -369,11 +378,6 @@ class CtrlClickShouldEndUpInSameProcessTest : public CtrlClickProcessTest {
     content::RenderProcessHost::SetMaxRendererProcessCount(1);
   }
 
-  void SetUpOnMainThread() override {
-    CtrlClickProcessTest::SetUpOnMainThread();
-    host_resolver()->AddRule("*", "127.0.0.1");
-  }
-
  protected:
   void VerifyProcessExpectations(content::WebContents* contents1,
                                  content::WebContents* contents2) override {
@@ -756,7 +760,6 @@ class SignInIsolationBrowserTest : public ChromeNavigationBrowserTest {
   }
 
   void SetUpOnMainThread() override {
-    host_resolver()->AddRule("*", "127.0.0.1");
     https_server_.StartAcceptingConnections();
     ChromeNavigationBrowserTest::SetUpOnMainThread();
   }
@@ -969,6 +972,60 @@ IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest,
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
 }
 
+// TODO(http://crbug.com/632514): This test currently expects opener downloads
+// go through and UMA is logged, but when the linked bug is resolved the
+// download should be disallowed.
+IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest,
+                       OpenerNavigation_DownloadPolicy) {
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload,
+                                               false);
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("a.com", "/title1.html"));
+
+  // Open a popup.
+  bool opened = false;
+  content::WebContents* opener =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  const char* kScriptFormat =
+      "window.domAutomationController.send(!!window.open('%s'));";
+  GURL popup_url = embedded_test_server()->GetURL("a.com", "/title1.html");
+  content::TestNavigationObserver popup_waiter(nullptr, 1);
+  popup_waiter.StartWatchingNewWebContents();
+  EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+      opener, base::StringPrintf(kScriptFormat, popup_url.spec().c_str()),
+      &opened));
+  EXPECT_TRUE(opened);
+  popup_waiter.Wait();
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+
+  // Using the popup, navigate its opener to a download.
+  base::HistogramTester histograms;
+  content::WebContents* popup =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_NE(popup, opener);
+  content::DownloadTestObserverInProgress observer(
+      content::BrowserContext::GetDownloadManager(browser()->profile()),
+      1 /* wait_count */);
+  EXPECT_TRUE(content::ExecuteScriptWithoutUserGesture(
+      popup,
+      "window.opener.location ='data:html/text;base64,'+btoa('payload');"));
+  observer.WaitForFinished();
+  histograms.ExpectBucketCount(
+      "Blink.UseCounter.Features",
+      blink::mojom::WebFeature::kOpenerNavigationDownloadCrossOriginNoGesture,
+      1);
+
+  // Delete any pending download.
+  std::vector<download::DownloadItem*> download_items;
+  content::DownloadManager* manager =
+      content::BrowserContext::GetDownloadManager(browser()->profile());
+  manager->GetAllDownloads(&download_items);
+  for (auto* item : download_items) {
+    if (!item->IsDone())
+      item->Cancel(true);
+  }
+}
+
 // TODO(csharrison): These tests should become tentative WPT, once the feature
 // is enabled by default.
 using NavigationConsumingTest = ChromeNavigationBrowserTest;

content/browser/frame_host/navigation_request.cc

@@ -74,6 +74,7 @@
 #include "third_party/blink/public/common/service_worker/service_worker_utils.h"
 #include "third_party/blink/public/mojom/fetch/fetch_api_request.mojom.h"
 #include "third_party/blink/public/platform/resource_request_blocked_reason.h"
+#include "third_party/blink/public/platform/web_feature.mojom.h"
 #include "third_party/blink/public/platform/web_mixed_content_context_type.h"
 #include "url/url_constants.h"
 
@@ -919,8 +920,21 @@ void NavigationRequest::OnResponseStarted(
     std::unique_ptr<NavigationData> navigation_data,
     const GlobalRequestID& request_id,
     bool is_download,
+    NavigationDownloadPolicy download_policy,
     bool is_stream,
     base::Optional<SubresourceLoaderParams> subresource_loader_params) {
+  is_download_ = is_download && IsNavigationDownloadAllowed(download_policy);
+
+  // Log UseCounters for opener navigations.
+  if (is_download &&
+      download_policy ==
+          NavigationDownloadPolicy::kAllowOpenerCrossOriginNoGesture) {
+    GetContentClient()->browser()->LogWebFeatureForCurrentPage(
+        frame_tree_node_->current_frame_host(),
+        blink::mojom::WebFeature::
+            kOpenerNavigationDownloadCrossOriginNoGesture);
+  }
+
   // TODO(https://crbug.com/880741): Remove this once the bug is fixed.
   if (state_ != STARTED) {
     DEBUG_ALIAS_FOR_GURL(url, navigation_handle_->GetURL());
@@ -934,9 +948,9 @@ void NavigationRequest::OnResponseStarted(
 
   // Check if the response should be sent to a renderer.
   response_should_be_rendered_ =
-      !is_download && (!response->head.headers.get() ||
-                       (response->head.headers->response_code() != 204 &&
-                        response->head.headers->response_code() != 205));
+      !is_download_ && (!response->head.headers.get() ||
+                        (response->head.headers->response_code() != 204 &&
+                         response->head.headers->response_code() != 205));
 
   // Response that will not commit should be marked as aborted in the
   // NavigationHandle.
@@ -1052,7 +1066,6 @@ void NavigationRequest::OnResponseStarted(
   url_loader_client_endpoints_ = std::move(url_loader_client_endpoints);
   ssl_info_ = response->head.ssl_info.has_value() ? *response->head.ssl_info
                                                   : net::SSLInfo();
-  is_download_ = is_download;
 
   subresource_loader_params_ = std::move(subresource_loader_params);
 
@@ -1091,8 +1104,8 @@ void NavigationRequest::OnResponseStarted(
   // know how to display the content.  We follow Firefox here and show our
   // own error page instead of intercepting the request as a stream or a
   // download.
-  if (is_download && (response->head.headers.get() &&
-                      (response->head.headers->response_code() / 100 != 2))) {
+  if (is_download_ && (response->head.headers.get() &&
+                       (response->head.headers->response_code() / 100 != 2))) {
     OnRequestFailedInternal(
         network::URLLoaderCompletionStatus(net::ERR_INVALID_RESPONSE),
         false /* skip_throttles */, base::nullopt /* error_page_content */,
@@ -1124,7 +1137,7 @@ void NavigationRequest::OnResponseStarted(
   navigation_handle_->WillProcessResponse(
       render_frame_host, response->head.headers.get(),
       response->head.connection_info, response->head.socket_address, ssl_info_,
-      request_id, common_params_.should_replace_current_entry, is_download,
+      request_id, common_params_.should_replace_current_entry, is_download_,
       is_stream, response->head.is_signed_exchange_inner_response,
       response->head.was_fetched_via_cache,
       base::Bind(&NavigationRequest::OnWillProcessResponseChecksComplete,

content/browser/frame_host/navigation_request.h

@@ -237,6 +237,7 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
       std::unique_ptr<NavigationData> navigation_data,
       const GlobalRequestID& request_id,
       bool is_download,
+      NavigationDownloadPolicy download_policy,
       bool is_stream,
       base::Optional<SubresourceLoaderParams> subresource_loader_params)
       override;

content/browser/loader/navigation_url_loader_delegate.h

@@ -11,6 +11,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/optional.h"
 #include "content/common/content_export.h"
+#include "content/common/navigation_params.h"
 #include "services/network/public/mojom/url_loader.mojom.h"
 
 namespace net {
@@ -45,12 +46,18 @@ class CONTENT_EXPORT NavigationURLLoaderDelegate {
   // necessary info to create a custom subresource loader in the renderer
   // process if the navigated context is controlled by a request interceptor
   // like AppCache or ServiceWorker.
+  //
+  // |is_download| is true if the request must be downloaded, if it isn't
+  // disallowed.
+  //
+  // |download_policy| specifies if downloading is disallowed.
   virtual void OnResponseStarted(
       const scoped_refptr<network::ResourceResponse>& response,
       network::mojom::URLLoaderClientEndpointsPtr url_loader_client_endpoints,
       std::unique_ptr<NavigationData> navigation_data,
       const GlobalRequestID& request_id,
       bool is_download,
+      NavigationDownloadPolicy download_policy,
       bool is_stream,
       base::Optional<SubresourceLoaderParams> subresource_loader_params) = 0;
 

content/browser/loader/navigation_url_loader_impl.cc

@@ -1779,8 +1779,8 @@ void NavigationURLLoaderImpl::OnReceiveResponse(
   // NavigationResourceHandler::OnResponseStarted() does.
   delegate_->OnResponseStarted(
       std::move(response), std::move(url_loader_client_endpoints),
-      std::move(navigation_data), global_request_id,
-      is_download && IsNavigationDownloadAllowed(download_policy_), is_stream,
+      std::move(navigation_data), global_request_id, is_download,
+      download_policy_, is_stream,
       request_controller_->TakeSubresourceLoaderParams());
 }
 

content/common/navigation_params.h

@@ -117,7 +117,8 @@ enum class NavigationDownloadPolicy {
 // Returns whether the given |policy| should allow for a download. This function
 // should be removed when http://crbug.com/632514 is resolved, when callers will
 // just compare with kAllow.
-bool IsNavigationDownloadAllowed(NavigationDownloadPolicy policy);
+bool CONTENT_EXPORT
+IsNavigationDownloadAllowed(NavigationDownloadPolicy policy);
 
 // Used by all navigation IPCs.
 struct CONTENT_EXPORT CommonNavigationParams {

content/public/browser/content_browser_client.h

@@ -50,6 +50,7 @@
 #include "storage/browser/fileapi/file_system_context.h"
 #include "storage/browser/quota/quota_manager.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_registry.h"
+#include "third_party/blink/public/platform/web_feature.mojom.h"
 #include "third_party/blink/public/web/window_features.mojom.h"
 #include "ui/base/page_transition_types.h"
 #include "ui/base/window_open_disposition.h"
@@ -1437,6 +1438,12 @@ class CONTENT_EXPORT ContentBrowserClient {
       content::PreviewsState initial_state,
       content::NavigationHandle* navigation_handle,
       const net::HttpResponseHeaders* response_headers);
+
+  // Browser-side API to log blink UseCounters for events that don't occur in
+  // the renderer.
+  virtual void LogWebFeatureForCurrentPage(
+      content::RenderFrameHost* render_frame_host,
+      blink::mojom::WebFeature feature) {}
 };
 
 }  // namespace content

content/test/test_navigation_url_loader.cc

@@ -91,7 +91,8 @@ void TestNavigationURLLoader::CallOnResponseStarted(
 
   delegate_->OnResponseStarted(response, std::move(url_loader_client_endpoints),
                                std::move(navigation_data), global_id, false,
-                               false, base::nullopt);
+                               NavigationDownloadPolicy::kAllow, false,
+                               base::nullopt);
 }
 
 TestNavigationURLLoader::~TestNavigationURLLoader() {}

content/test/test_navigation_url_loader_delegate.cc

@@ -5,6 +5,7 @@
 #include "content/test/test_navigation_url_loader_delegate.h"
 
 #include "base/run_loop.h"
+#include "content/common/navigation_params.h"
 #include "content/common/navigation_subresource_loader_params.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/browser/navigation_data.h"
@@ -61,13 +62,14 @@ void TestNavigationURLLoaderDelegate::OnResponseStarted(
     std::unique_ptr<NavigationData> navigation_data,
     const GlobalRequestID& request_id,
     bool is_download,
+    NavigationDownloadPolicy download_policy,
     bool is_stream,
     base::Optional<SubresourceLoaderParams> subresource_loader_params) {
   response_ = response;
   url_loader_client_endpoints_ = std::move(url_loader_client_endpoints);
   if (response->head.ssl_info.has_value())
     ssl_info_ = *response->head.ssl_info;
-  is_download_ = is_download;
+  is_download_ = is_download && IsNavigationDownloadAllowed(download_policy);
   if (response_started_)
     response_started_->Quit();
 }

content/test/test_navigation_url_loader_delegate.h

@@ -66,6 +66,7 @@ class TestNavigationURLLoaderDelegate : public NavigationURLLoaderDelegate {
       std::unique_ptr<NavigationData> navigation_data,
       const GlobalRequestID& request_id,
       bool is_download,
+      NavigationDownloadPolicy download_policy,
       bool is_stream,
       base::Optional<SubresourceLoaderParams> subresource_loader_params)
       override;

third_party/blink/public/platform/web_feature.mojom

@@ -2098,6 +2098,7 @@ enum WebFeature {
   kCSSSelectorNotWithInvalidList = 2646,
   kCSSSelectorNotWithPartiallyValidList = 2647,
   kV8IDBFactory_Databases_Method = 2648,
+  kOpenerNavigationDownloadCrossOriginNoGesture = 2649,
 
   // Add new features immediately above this line. Don't change assigned
   // numbers of any item, and don't reuse removed slots.

tools/metrics/histograms/enums.xml

@@ -20932,6 +20932,7 @@ Called by update_net_error_codes.py.-->
   <int value="2646" label="CSSSelectorNotWithInvalidList"/>
   <int value="2647" label="CSSSelectorNotWithPartiallyValidList"/>
   <int value="2648" label="V8IDBFactory_Databases_Method"/>
+  <int value="2649" label="OpenerNavigationDownloadCrossOriginNoGesture"/>
 </enum>
 
 <enum name="FeaturePolicyFeature">