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 88 matching lines @@
 // 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,
 };
 
+// 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
+// SCHEME_LAST.  It must also be synchronized to kSchemeNames in
+// RecordShowAllowOpenURLFailure.

[ncarter (slow), 2017/03/29 22:59:21]

It's possible to add a presubmit that will fail if this winds up mismatching the
xml.

You can also declare these enums in such a way that the .xml can be auto-updated
from the .cc file. We do this for bad_message, see the scripts here: --

https://cs.chromium.org/chromium/src/tools/metrics/histograms/update_histogra...

and the other update_ scripts (which just call into the above).

LMK if you want more guidance here -- I'm very familiar with those scripts.

[alexmos, 2017/03/31 01:11:04]

Done.  This is awesome, thanks for the pointers!  I had no idea these existed.

+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_EXTENSION,
+  SCHEME_BLOB,
+  SCHEME_FILESYSTEM,

[ncarter (slow), 2017/03/29 22:59:21]

content://  (that's the android-only file:// like scheme)
chrome-native://
chrome-distiller://

[alexmos, 2017/03/31 01:11:04]

Done.  content:// should never show up for this uma (no extensions on android),
but still good to have it in the list (and the same is true for things like
view-source).

+  SCHEME_LAST,

[ncarter (slow), 2017/03/29 22:59:21]

I recommend adding a unittest here that: grab a set of registered schemes from
somewhere (maybe from the ContentBrowserClient impl, maybe from GURL) and pass
it through your function.

Then use histogram_tester to assert that the histogram has no samples in the
UNKNOWN bucket.

[alexmos, 2017/03/31 01:11:04]

Done.  Had to move some things around to let the test access the recording
function (and fixed a typo in the name along the way).  I didn't find a good
list of schemes; I ended up pulling out some known schemes out of
ContentBrowserClient (the ones in GURL aren't exposed publicly) in a pretty
arbitrary way, plus adding a few manually, but let me know if you see a better
way.

+};
+
 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
   // anyways.
   if (!url.is_valid())
@@ skipping 102 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) {
+// Helper to record metrics when ShouldAllowOpenURL blocks a load.  |site_url|
+// corresponds to the SiteInstance that initiated the blocked load.
+void RecordShowAllowOpenURLFailure(ShouldAllowOpenURLFailureReason reason,
+                                   GURL site_url) {
   UMA_HISTOGRAM_ENUMERATION("Extensions.ShouldAllowOpenURL.Failure", reason,
                             FAILURE_LAST);
+
+  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,
+      extensions::kExtensionScheme,
+      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",
+                            scheme, SCHEME_LAST);
 }
 
 }  // namespace
 
 ChromeContentBrowserClientExtensionsPart::
     ChromeContentBrowserClientExtensionsPart() {
 }
 
 ChromeContentBrowserClientExtensionsPart::
     ~ChromeContentBrowserClientExtensionsPart() {
@@ skipping 336 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);
+      RecordShowAllowOpenURLFailure(FAILURE_FILE_SYSTEM_URL, site_url);
     else
-      RecordShowAllowOpenURLFailure(FAILURE_BLOB_URL);
+      RecordShowAllowOpenURLFailure(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.
+    RecordShowAllowOpenURLFailure(FAILURE_SCHEME_NOT_HTTP_OR_HTTPS_OR_EXTENSION,
+                                  site_url);
   } else {
-    RecordShowAllowOpenURLFailure(FAILURE_RESOURCE_NOT_WEB_ACCESSIBLE);
+    RecordShowAllowOpenURLFailure(FAILURE_RESOURCE_NOT_WEB_ACCESSIBLE,
+                                  site_url);
   }
 
   *result = false;
   return true;
 }
 
 // static
 std::unique_ptr<content::VpnServiceProxy>
 ChromeContentBrowserClientExtensionsPart::GetVpnServiceProxy(
     content::BrowserContext* browser_context) {
@@ skipping 146 matching lines @@
     command_line->AppendSwitch(switches::kExtensionProcess);
   }
 }
 
 void ChromeContentBrowserClientExtensionsPart::ResourceDispatcherHostCreated() {
   content::ResourceDispatcherHost::Get()->RegisterInterceptor(
       "Origin", kExtensionScheme, base::Bind(&OnHttpHeaderReceived));
 }
 
 }  // namespace extensions