Centralize UKM scheme checks to be in ukm_recorder_impl.cc.

Includes a unit test. Also fixes some lint warnings.

BUG=792553
TBR=khushalsagar@chromium.org

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I4d582129d50bc2a7403b0bc0028bbf0ff4fd6b4e
Reviewed-on: https://chromium-review.googlesource.com/811627
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522467}

cc/trees/layer_tree_host_unittest.cc

@@ -8209,7 +8209,7 @@ MULTI_THREAD_TEST_F(LayerTreeHostTestImageDecodingHints);
 
 class LayerTreeHostTestCheckerboardUkm : public LayerTreeHostTest {
  public:
-  LayerTreeHostTestCheckerboardUkm() : url_(GURL("chrome://test")) {}
+  LayerTreeHostTestCheckerboardUkm() : url_(GURL("https://example.com")) {}
 
   void BeginTest() override {
     PostSetNeedsCommitToMainThread();

cc/trees/ukm_manager_unittest.cc

@@ -10,8 +10,8 @@
 namespace cc {
 namespace {
 
-const char kTestUrl1[] = "chrome://test";
-const char kTestUrl2[] = "chrome://test2";
+const char kTestUrl1[] = "https://example.com/foo";
+const char kTestUrl2[] = "https://example.com/bar";
 
 const char kUserInteraction[] = "Compositor.UserInteraction";
 const char kCheckerboardArea[] = "CheckerboardedContentArea";

components/ukm/content/source_url_recorder.cc

@@ -18,10 +18,6 @@
 
 namespace {
 
-bool IsValidUkmUrl(const GURL& url) {
-  return url.SchemeIsHTTPOrHTTPS();
-}
-
 // SourceUrlRecorderWebContentsObserver is responsible for recording UKM source
 // URLs, for all main frame navigations in a given WebContents.
 // SourceUrlRecorderWebContentsObserver records both the final URL for a
@@ -122,12 +118,10 @@ void SourceUrlRecorderWebContentsObserver::MaybeRecordUrl(
 
   const ukm::SourceId source_id = ukm::ConvertToSourceId(
       navigation_handle->GetNavigationId(), ukm::SourceIdType::NAVIGATION_ID);
-
-  if (IsValidUkmUrl(initial_url))
-    ukm_recorder->UpdateSourceURL(source_id, initial_url);
+  ukm_recorder->UpdateSourceURL(source_id, initial_url);
 
   const GURL& final_url = navigation_handle->GetURL();
-  if (final_url != initial_url && IsValidUkmUrl(final_url))
+  if (final_url != initial_url)
     ukm_recorder->UpdateSourceURL(source_id, final_url);
 }
 

components/ukm/content/source_url_recorder_browsertest.cc

@@ -89,16 +89,6 @@ IN_PROC_BROWSER_TEST_F(SourceUrlRecorderWebContentsObserverBrowserTest, Basic) {
   EXPECT_EQ(url, GetAssociatedURLForWebContentsDocument());
 }
 
-IN_PROC_BROWSER_TEST_F(SourceUrlRecorderWebContentsObserverBrowserTest,
-                       IgnoreUnsupportedScheme) {
-  GURL url("about:blank");
-  content::NavigationHandleObserver observer(shell()->web_contents(), url);
-  content::NavigateToURL(shell(), url);
-  EXPECT_TRUE(observer.has_committed());
-  EXPECT_EQ(nullptr, GetSourceForNavigationId(observer.navigation_id()));
-  EXPECT_EQ(GURL(), GetAssociatedURLForWebContentsDocument());
-}
-
 IN_PROC_BROWSER_TEST_F(SourceUrlRecorderWebContentsObserverBrowserTest,
                        IgnoreUrlInSubframe) {
   GURL main_url = embedded_test_server()->GetURL("/page_with_iframe.html");

components/ukm/content/source_url_recorder_test.cc

@@ -62,14 +62,6 @@ TEST_F(SourceUrlRecorderWebContentsObserverTest, InitialUrl) {
   EXPECT_EQ(final_url, GetAssociatedURLForWebContentsDocument());
 }
 
-TEST_F(SourceUrlRecorderWebContentsObserverTest, IgnoreUnsupportedScheme) {
-  NavigationSimulator::NavigateAndCommitFromBrowser(web_contents(),
-                                                    GURL("about:blank"));
-  EXPECT_EQ(0ul, test_ukm_recorder_.sources_count());
-
-  EXPECT_EQ(GURL(), GetAssociatedURLForWebContentsDocument());
-}
-
 TEST_F(SourceUrlRecorderWebContentsObserverTest, IgnoreUrlInSubframe) {
   GURL main_frame_url("https://www.example.com/");
   GURL sub_frame_url("https://www.example.com/iframe.html");

components/ukm/ukm_recorder_impl.cc

@@ -4,6 +4,10 @@
 
 #include "components/ukm/ukm_recorder_impl.h"
 
+#include <memory>
+#include <string>
+#include <utility>
+
 #include "base/metrics/field_trial.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
@@ -55,6 +59,19 @@ size_t GetMaxEntries() {
       kUkmFeature, "MaxEntries", kDefaultMaxEntries));
 }
 
+// Returns whether |url| has one of the schemes supported for logging to UKM.
+// URLs with other schemes will not be logged.
+// Note: This currently excludes chrome-extension:// URLs as in order to log
+// them, UKM needs to take into account extension-sync consent, which is not
+// yet done.
+bool HasSupportedScheme(const GURL& url) {
+  // TODO(asvitkine): Support chrome:// and about: URLs here once we have
+  // approval (and necessary logic) for them. Via:
+  //   url.SchemeIs(url::kAboutScheme) || url.SchemeIs("chrome")
+  // Also add FTP Scheme once approved: url.SchemeIs(url::kFtpScheme)
+  return url.SchemeIsHTTPOrHTTPS();
+}
+
 // True if we should record the initial_url field of the UKM Source proto.
 bool ShouldRecordInitialUrl() {
   return base::GetFieldTrialParamByFeatureAsBool(kUkmFeature,
@@ -66,6 +83,7 @@ enum class DroppedDataReason {
   RECORDING_DISABLED = 1,
   MAX_HIT = 2,
   NOT_WHITELISTED = 3,
+  UNSUPPORTED_URL_SCHEME = 4,
   NUM_DROPPED_DATA_REASONS
 };
 
@@ -189,6 +207,11 @@ void UkmRecorderImpl::UpdateSourceURL(SourceId source_id, const GURL& url) {
     return;
   }
 
+  if (!HasSupportedScheme(url)) {
+    RecordDroppedSource(DroppedDataReason::UNSUPPORTED_URL_SCHEME);
+    return;
+  }
+
   // Update the pre-existing source if there is any. This happens when the
   // initial URL is different from the committed URL for the same source, e.g.,
   // when there is redirection.

components/ukm/ukm_service_unittest.cc

@@ -36,11 +36,11 @@ namespace ukm {
 // A small shim exposing UkmRecorder methods to tests.
 class TestRecordingHelper {
  public:
-  TestRecordingHelper(UkmRecorder* recorder) : recorder_(recorder) {}
+  explicit TestRecordingHelper(UkmRecorder* recorder) : recorder_(recorder) {}
 
   void UpdateSourceURL(SourceId source_id, const GURL& url) {
     recorder_->UpdateSourceURL(source_id, url);
-  };
+  }
 
   std::unique_ptr<UkmEntryBuilder> GetEntryBuilder(SourceId source_id,
                                                    const char* event_name) {
@@ -49,6 +49,8 @@ class TestRecordingHelper {
 
  private:
   UkmRecorder* recorder_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestRecordingHelper);
 };
 
 namespace {
@@ -720,4 +722,58 @@ TEST_F(UkmServiceTest, UnreferencedNonWhitelistedSources) {
   }
 }
 
+TEST_F(UkmServiceTest, UnsupportedSchemes) {
+  struct {
+    const char* url;
+    bool expected_kept;
+  } test_cases[] = {
+      {"http://google.ca/", true},
+      {"https://google.ca/", true},
+      {"ftp://google.ca/", false},
+      {"about:blank", false},
+      {"chrome://version/", false},
+      {"file:///tmp/", false},
+      {"chrome-extension://bhcnanendmgjjeghamaccjnochlnhcgj", false},
+      {"abc://google.ca/", false},
+      {"www.google.ca/", false},
+  };
+
+  base::FieldTrialList field_trial_list(nullptr /* entropy_provider */);
+  ScopedUkmFeatureParams params(base::FeatureList::OVERRIDE_ENABLE_FEATURE, {});
+  UkmService service(&prefs_, &client_);
+  TestRecordingHelper recorder(&service);
+
+  EXPECT_EQ(GetPersistedLogCount(), 0);
+  service.Initialize();
+  task_runner_->RunUntilIdle();
+  service.EnableRecording();
+  service.EnableReporting();
+
+  int64_t id_counter = 1;
+  int expected_kept_count = 0;
+  for (const auto& test : test_cases) {
+    auto source_id = GetWhitelistedSourceId(id_counter++);
+    recorder.UpdateSourceURL(source_id, GURL(test.url));
+    recorder.GetEntryBuilder(source_id, "FakeEntry");
+    if (test.expected_kept)
+      ++expected_kept_count;
+  }
+
+  service.Flush();
+  EXPECT_EQ(GetPersistedLogCount(), 1);
+  Report proto_report = GetPersistedReport();
+
+  EXPECT_EQ(expected_kept_count, proto_report.sources_size());
+  for (const auto& test : test_cases) {
+    bool found = false;
+    for (int i = 0; i < proto_report.sources_size(); ++i) {
+      if (proto_report.sources(i).url() == test.url) {
+        found = true;
+        break;
+      }
+    }
+    EXPECT_EQ(test.expected_kept, found) << test.url;
+  }
+}
+
 }  // namespace ukm

tools/metrics/histograms/enums.xml

@@ -41868,6 +41868,7 @@ Full version information for the fingerprint enum values:
   <int value="1" label="Recording disabled"/>
   <int value="2" label="Max hit"/>
   <int value="3" label="Not whitelisted"/>
+  <int value="4" label="Unsupported URL scheme"/>
 </enum>
 
 <enum name="UmaCleanExitConsistency">