Remove DumpWithoutCrashing from ShouldAllowOpenURL.

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

 // Copyright 2014 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 "chrome/browser/extensions/chrome_content_browser_client_extensions_part.h"
 
 #include <stddef.h>
 
 #include <set>
 
@@ skipping 11 matching lines @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/renderer_host/chrome_extension_message_filter.h"
 #include "chrome/browser/sync_file_system/local/sync_file_system_backend.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/extension_process_policy.h"
 #include "chrome/common/url_constants.h"
+#include "components/dom_distiller/core/url_constants.h"
 #include "components/guest_view/browser/guest_view_message_filter.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/browser_url_handler.h"
 #include "content/public/browser/page_navigator.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/resource_dispatcher_host.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/vpn_service_proxy.h"
@@ skipping 43 matching lines @@
 // below.  Extension, and isolated apps require different privileges to be
 // granted to their RenderProcessHosts.  This classification allows us to make
 // sure URLs are served by hosts with the right set of privileges.
 enum RenderProcessHostPrivilege {
   PRIV_NORMAL,
   PRIV_HOSTED,
   PRIV_ISOLATED,
   PRIV_EXTENSION,
 };
 
-// Specifies reasons why web-accessible resource checks in ShouldAllowOpenURL
-// might fail.
+// Specifies the scheme of the SiteInstance responsible for a failed
+// web-accessible resource check in ShouldAllowOpenURL.
 //
 // This enum backs an UMA histogram.  The order of existing values
-// should not be changed, and new values should only be added before
-// FAILURE_LAST.
-enum ShouldAllowOpenURLFailureReason {
-  FAILURE_FILE_SYSTEM_URL = 0,
-  FAILURE_BLOB_URL,
-  FAILURE_SCHEME_NOT_HTTP_OR_HTTPS_OR_EXTENSION,
-  FAILURE_RESOURCE_NOT_WEB_ACCESSIBLE,
-  FAILURE_LAST,
+// should not be changed.  Add any new values before SCHEME_LAST, and also run
+// update_should_allow_open_url_histograms.py to update the corresponding enum
+// in histograms.xml.  This enum must also be synchronized to kSchemeNames in
+// RecordShouldAllowOpenURLFailure.
+enum ShouldAllowOpenURLFailureScheme {
+  SCHEME_UNKNOWN,
+  SCHEME_EMPTY,
+  SCHEME_HTTP,
+  SCHEME_HTTPS,
+  SCHEME_FILE,
+  SCHEME_FTP,
+  SCHEME_DATA,
+  SCHEME_JAVASCRIPT,
+  SCHEME_ABOUT,
+  SCHEME_CHROME,
+  SCHEME_DEVTOOLS,
+  SCHEME_GUEST,
+  SCHEME_VIEWSOURCE,
+  SCHEME_CHROME_SEARCH,
+  SCHEME_CHROME_NATIVE,
+  SCHEME_DOM_DISTILLER,
+  SCHEME_CHROME_EXTENSION,
+  SCHEME_CONTENT,
+  SCHEME_BLOB,
+  SCHEME_FILESYSTEM,
+  // Add new entries above and make sure to update histograms.xml by running
+  // update_should_allow_open_url_histograms.py.
+  SCHEME_LAST,
 };
 
 RenderProcessHostPrivilege GetPrivilegeRequiredByUrl(
     const GURL& url,
     ExtensionRegistry* registry) {
   // Default to a normal renderer cause it is lower privileged. This should only
   // occur if the URL on a site instance is either malformed, or uninitialized.
   // If it is malformed, then there is no need for better privileges anyways.
   // If it is uninitialized, but eventually settles on being an a scheme other
   // than normal webrenderer, the navigation logic will correct us out of band
@@ skipping 104 matching lines @@
                           content::ResourceContext* resource_context,
                           content::OnHeaderProcessedCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   GURL origin(value);
   DCHECK(origin.SchemeIs(extensions::kExtensionScheme));
 
   callback.Run(CheckOriginHeader(resource_context, child_id, origin));
 }
 
-void RecordShowAllowOpenURLFailure(ShouldAllowOpenURLFailureReason reason) {
-  UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure", reason,
-                            FAILURE_LAST);
-}
-
 }  // namespace
 
 ChromeContentBrowserClientExtensionsPart::
     ChromeContentBrowserClientExtensionsPart() {
 }
 
 ChromeContentBrowserClientExtensionsPart::
     ~ChromeContentBrowserClientExtensionsPart() {
 }
 
@@ skipping 334 matching lines @@
       registry->enabled_extensions().GetExtensionOrAppByURL(site_url);
   if (from_extension && from_extension == to_extension) {
     *result = true;
     return true;
   }
 
   // Blob and filesystem URLs are never considered web-accessible.  See
   // https://crbug.com/656752.
   if (to_url.SchemeIsFileSystem() || to_url.SchemeIsBlob()) {
     if (to_url.SchemeIsFileSystem())
-      RecordShowAllowOpenURLFailure(FAILURE_FILE_SYSTEM_URL);
+      RecordShouldAllowOpenURLFailure(FAILURE_FILE_SYSTEM_URL, site_url);
     else
-      RecordShowAllowOpenURLFailure(FAILURE_BLOB_URL);
+      RecordShouldAllowOpenURLFailure(FAILURE_BLOB_URL, site_url);
 
     // TODO(alexmos): Temporary instrumentation to find any regressions for
     // this blocking.  Remove after verifying that this is not breaking any
     // legitimate use cases.
     char site_url_copy[256];
     base::strlcpy(site_url_copy, site_url.spec().c_str(),
                   arraysize(site_url_copy));
     base::debug::Alias(&site_url_copy);
     char to_origin_copy[256];
     base::strlcpy(to_origin_copy, to_origin.Serialize().c_str(),
@@ skipping 27 matching lines @@
     return true;
   }
 
   if (WebAccessibleResourcesInfo::IsResourceWebAccessible(to_extension,
                                                           to_url.path())) {
     *result = true;
     return true;
   }
 
   if (!site_url.SchemeIsHTTPOrHTTPS() && !site_url.SchemeIs(kExtensionScheme)) {
-    RecordShowAllowOpenURLFailure(
-        FAILURE_SCHEME_NOT_HTTP_OR_HTTPS_OR_EXTENSION);
-
-    // TODO(alexmos): Previous version of this function skipped the
-    // web-accessible resource checks in this case.  Collect data to catch
-    // any regressions, and then remove this.
-    char site_url_copy[256];
-    base::strlcpy(site_url_copy, site_url.spec().c_str(),
-                  arraysize(site_url_copy));
-    base::debug::Alias(&site_url_copy);
-    char to_origin_copy[256];
-    base::strlcpy(to_origin_copy, to_origin.Serialize().c_str(),
-                  arraysize(to_origin_copy));
-    base::debug::Alias(&to_origin_copy);
-    base::debug::DumpWithoutCrashing();
+    // This function used to incorrectly skip the web-accessible resource
+    // checks in this case. Measure how often this happens.  See also
+    // https://crbug.com/696034.
+    RecordShouldAllowOpenURLFailure(
+        FAILURE_SCHEME_NOT_HTTP_OR_HTTPS_OR_EXTENSION, site_url);
   } else {
-    RecordShowAllowOpenURLFailure(FAILURE_RESOURCE_NOT_WEB_ACCESSIBLE);
+    RecordShouldAllowOpenURLFailure(FAILURE_RESOURCE_NOT_WEB_ACCESSIBLE,
+                                    site_url);
   }
 
   *result = false;
   return true;
 }
 
 // static
 std::unique_ptr<content::VpnServiceProxy>
 ChromeContentBrowserClientExtensionsPart::GetVpnServiceProxy(
     content::BrowserContext* browser_context) {
 #if defined(OS_CHROMEOS)
   chromeos::VpnService* vpn_service =
       chromeos::VpnServiceFactory::GetForBrowserContext(browser_context);
   if (!vpn_service)
     return nullptr;
   return vpn_service->GetVpnServiceProxy();
 #else
   return nullptr;
 #endif
 }
 
+// static
+void ChromeContentBrowserClientExtensionsPart::RecordShouldAllowOpenURLFailure(
+    ShouldAllowOpenURLFailureReason reason,
+    GURL site_url) {
+  UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure", reason,
+                            FAILURE_LAST);
+
+  // Must be kept in sync with the ShouldAllowOpenURLFailureScheme enum.
+  const char* const kSchemeNames[] = {
+      "unknown",
+      "",
+      url::kHttpScheme,
+      url::kHttpsScheme,
+      url::kFileScheme,
+      url::kFtpScheme,
+      url::kDataScheme,
+      url::kJavaScriptScheme,
+      url::kAboutScheme,
+      content::kChromeUIScheme,
+      content::kChromeDevToolsScheme,
+      content::kGuestScheme,
+      content::kViewSourceScheme,
+      chrome::kChromeSearchScheme,
+      chrome::kChromeNativeScheme,
+      dom_distiller::kDomDistillerScheme,
+      extensions::kExtensionScheme,
+      url::kContentScheme,
+      url::kBlobScheme,
+      url::kFileSystemScheme,
+      "last",
+  };
+
+  static_assert(arraysize(kSchemeNames) == SCHEME_LAST + 1,
+                "kSchemeNames should have SCHEME_LAST + 1 elements");
+
+  ShouldAllowOpenURLFailureScheme scheme = SCHEME_UNKNOWN;
+  for (int i = 1; i < SCHEME_LAST; i++) {
+    if (site_url.SchemeIs(kSchemeNames[i])) {
+      scheme = static_cast<ShouldAllowOpenURLFailureScheme>(i);
+      break;
+    }
+  }
+
+  UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure.Scheme",

[Devlin, 2017/04/05 01:11:57]

I don't think I've seen us have Histogram.Foo and Histogram.Foo.Bar be distinct
entries before.  I'd assume it would work, though?

[alexmos, 2017/04/05 17:24:51]

Yes, it'd be good to get confirmation from asvitkine@ that this is ok.

+                            scheme, SCHEME_LAST);
+}
+
 void ChromeContentBrowserClientExtensionsPart::RenderProcessWillLaunch(
     content::RenderProcessHost* host) {
   int id = host->GetID();
   Profile* profile = Profile::FromBrowserContext(host->GetBrowserContext());
 
   host->AddFilter(new ChromeExtensionMessageFilter(id, profile));
   host->AddFilter(new ExtensionMessageFilter(id, profile));
   host->AddFilter(new IOThreadExtensionMessageFilter(id, profile));
   host->AddFilter(new ExtensionsGuestViewMessageFilter(id, profile));
   if (extensions::ExtensionsClient::Get()
@@ skipping 125 matching lines @@
     command_line->AppendSwitch(switches::kExtensionProcess);
   }
 }
 
 void ChromeContentBrowserClientExtensionsPart::ResourceDispatcherHostCreated() {
   content::ResourceDispatcherHost::Get()->RegisterInterceptor(
       "Origin", kExtensionScheme, base::Bind(&OnHttpHeaderReceived));
 }
 
 }  // namespace extensions