Remove DumpWithoutCrashing from ShouldAllowOpenURL.

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

Index: chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
diff --git a/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc b/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
index b76e1dc5d980d46dc6710173bc320d3988d04dc9..4d48771304b16a442285bd84e11f6acd9e4fe5fe 100644
--- a/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
+++ b/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
@@ -106,6 +106,34 @@ enum ShouldAllowOpenURLFailureReason {
   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) {
@@ -228,9 +256,47 @@ void OnHttpHeaderReceived(const std::string& header,
   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
@@ -587,9 +653,9 @@ bool ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL(
   // 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
@@ -637,23 +703,14 @@ bool ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL(
   }
 
   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;