Add tab_id/previous_source_id/opener_source_id to UkmSource

This CL adds three new fields to UkmSource. A follow-up will propagate
changes to the underlying proto fields.

Bug: 873316
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ic691221c8165df7cee16b089407d8ddd346e78c3
Reviewed-on: https://chromium-review.googlesource.com/1175033
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Robert Kaplow (slow) <rkaplow@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585824}

chrome/browser/metrics/ukm_browsertest.cc

@@ -21,19 +21,27 @@
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/unified_consent/unified_consent_service_factory.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/metrics_services_manager/metrics_services_manager.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/test/fake_server/fake_server_network_resources.h"
+#include "components/ukm/content/source_url_recorder.h"
 #include "components/ukm/ukm_service.h"
 #include "components/unified_consent/scoped_unified_consent.h"
 #include "components/variations/service/variations_field_trial_creator.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browsing_data_remover.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/browsing_data_remover_test_util.h"
+#include "content/public/test/navigation_handle_observer.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
 #include "services/metrics/public/cpp/ukm_source.h"
@@ -152,7 +160,22 @@ class UkmBrowserTestBase : public SyncTest {
     // Can be non-zero only if UpdateUploadPermissions(true) has been called.
     return service->client_id_;
   }
-  bool HasDummySource(ukm::SourceId source_id) const {
+  ukm::UkmSource* GetSource(ukm::SourceId source_id) {
+    auto* service = ukm_service();
+    if (!service)
+      return nullptr;
+    auto it = service->sources().find(source_id);
+    return it == service->sources().end() ? nullptr : it->second.get();
+  }
+  ukm::UkmSource* NavigateAndGetSource(Browser* browser, const GURL& url) {
+    content::NavigationHandleObserver observer(
+        browser->tab_strip_model()->GetActiveWebContents(), url);
+    ui_test_utils::NavigateToURL(browser, url);
+    const ukm::SourceId source_id = ukm::ConvertToSourceId(
+        observer.navigation_id(), ukm::SourceIdType::NAVIGATION_ID);
+    return GetSource(source_id);
+  }
+  bool HasSource(ukm::SourceId source_id) const {
     auto* service = ukm_service();
     return service ? !!service->sources().count(source_id) : false;
   }
@@ -689,6 +712,114 @@ IN_PROC_BROWSER_TEST_P(UkmBrowserTest, MultiDisableExtensionsSyncCheck) {
   CloseBrowserSynchronously(browser1);
 }
 
+IN_PROC_BROWSER_TEST_P(UkmBrowserTest, LogsTabId) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  MetricsConsentOverride metrics_consent(true);
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  std::unique_ptr<ProfileSyncServiceHarness> harness =
+      EnableSyncForProfile(profile);
+  Browser* sync_browser = CreateBrowser(profile);
+
+  const ukm::UkmSource* first_source = NavigateAndGetSource(
+      sync_browser, embedded_test_server()->GetURL("/title1.html"));
+
+  // Tab ids are incremented starting from 1. Since we started a new sync
+  // browser, this is the second tab.
+  EXPECT_EQ(2, first_source->navigation_data().tab_id);
+
+  // Ensure the tab id is constant in a single tab.
+  const ukm::UkmSource* second_source = NavigateAndGetSource(
+      sync_browser, embedded_test_server()->GetURL("/title2.html"));
+  EXPECT_EQ(first_source->navigation_data().tab_id,
+            second_source->navigation_data().tab_id);
+
+  // Add a new tab, it should get a new tab id.
+  chrome::NewTab(sync_browser);
+  const ukm::UkmSource* third_source = NavigateAndGetSource(
+      sync_browser, embedded_test_server()->GetURL("/title3.html"));
+  EXPECT_EQ(3, third_source->navigation_data().tab_id);
+}
+
+IN_PROC_BROWSER_TEST_P(UkmBrowserTest, LogsPreviousSourceId) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  MetricsConsentOverride metrics_consent(true);
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  std::unique_ptr<ProfileSyncServiceHarness> harness =
+      EnableSyncForProfile(profile);
+  Browser* sync_browser = CreateBrowser(profile);
+
+  const ukm::UkmSource* first_source = NavigateAndGetSource(
+      sync_browser, embedded_test_server()->GetURL("/title1.html"));
+
+  const ukm::UkmSource* second_source = NavigateAndGetSource(
+      sync_browser, embedded_test_server()->GetURL("/title2.html"));
+  EXPECT_EQ(first_source->id(),
+            second_source->navigation_data().previous_source_id);
+
+  // Open a new tab with window.open.
+  content::WebContents* opener =
+      sync_browser->tab_strip_model()->GetActiveWebContents();
+  GURL new_tab_url = embedded_test_server()->GetURL("/title3.html");
+  content::TestNavigationObserver waiter(new_tab_url);
+  waiter.StartWatchingNewWebContents();
+  EXPECT_TRUE(content::ExecuteScript(
+      opener, content::JsReplace("window.open($1)", new_tab_url)));
+  waiter.Wait();
+  EXPECT_NE(opener, sync_browser->tab_strip_model()->GetActiveWebContents());
+  ukm::SourceId new_id = ukm::GetSourceIdForWebContentsDocument(
+      sync_browser->tab_strip_model()->GetActiveWebContents());
+  ukm::UkmSource* new_tab_source = GetSource(new_id);
+  EXPECT_NE(nullptr, new_tab_source);
+  EXPECT_EQ(ukm::kInvalidSourceId,
+            new_tab_source->navigation_data().previous_source_id);
+
+  // Subsequent navigations within the tab should get a previous_source_id field
+  // set.
+  const ukm::UkmSource* subsequent_source = NavigateAndGetSource(
+      sync_browser, embedded_test_server()->GetURL("/title3.html"));
+  EXPECT_EQ(new_tab_source->id(),
+            subsequent_source->navigation_data().previous_source_id);
+}
+
+IN_PROC_BROWSER_TEST_P(UkmBrowserTest, LogsOpenerSource) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  MetricsConsentOverride metrics_consent(true);
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  std::unique_ptr<ProfileSyncServiceHarness> harness =
+      EnableSyncForProfile(profile);
+  Browser* sync_browser = CreateBrowser(profile);
+
+  const ukm::UkmSource* first_source = NavigateAndGetSource(
+      sync_browser, embedded_test_server()->GetURL("/title1.html"));
+  // This tab was not opened by another tab, so it should not have an opener
+  // id.
+  EXPECT_EQ(ukm::kInvalidSourceId,
+            first_source->navigation_data().opener_source_id);
+
+  // Open a new tab with window.open.
+  content::WebContents* opener =
+      sync_browser->tab_strip_model()->GetActiveWebContents();
+  GURL new_tab_url = embedded_test_server()->GetURL("/title2.html");
+  content::TestNavigationObserver waiter(new_tab_url);
+  waiter.StartWatchingNewWebContents();
+  EXPECT_TRUE(content::ExecuteScript(
+      opener, content::JsReplace("window.open($1)", new_tab_url)));
+  waiter.Wait();
+  EXPECT_NE(opener, sync_browser->tab_strip_model()->GetActiveWebContents());
+  ukm::SourceId new_id = ukm::GetSourceIdForWebContentsDocument(
+      sync_browser->tab_strip_model()->GetActiveWebContents());
+  ukm::UkmSource* new_tab_source = GetSource(new_id);
+  EXPECT_NE(nullptr, new_tab_source);
+  EXPECT_EQ(first_source->id(),
+            new_tab_source->navigation_data().opener_source_id);
+
+  // Subsequent navigations within the tab should not get an opener set.
+  const ukm::UkmSource* subsequent_source = NavigateAndGetSource(
+      sync_browser, embedded_test_server()->GetURL("/title3.html"));
+  EXPECT_EQ(ukm::kInvalidSourceId,
+            subsequent_source->navigation_data().opener_source_id);
+}
+
 // Make sure that UKM is disabled when an secondary passphrase is set.
 // Keep in sync with UkmTest.secondaryPassphraseCheck in
 // chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/
@@ -845,12 +976,12 @@ IN_PROC_BROWSER_TEST_P(UkmBrowserTest, HistoryDeleteCheck) {
 
   const ukm::SourceId kDummySourceId = 0x54321;
   RecordDummySource(kDummySourceId);
-  EXPECT_TRUE(HasDummySource(kDummySourceId));
+  EXPECT_TRUE(HasSource(kDummySourceId));
 
   ClearBrowsingData(profile);
   // Other sources may already have been recorded since the data was cleared,
   // but the dummy source should be gone.
-  EXPECT_FALSE(HasDummySource(kDummySourceId));
+  EXPECT_FALSE(HasSource(kDummySourceId));
   // Client ID should NOT be reset.
   EXPECT_EQ(original_client_id, client_id());
   EXPECT_TRUE(ukm_enabled());

components/ukm/content/source_url_recorder.cc

@@ -23,6 +23,11 @@ namespace ukm {
 
 namespace internal {
 
+int64_t CreateUniqueTabId() {
+  static int64_t unique_id_counter = 0;
+  return ++unique_id_counter;
+}
+
 // SourceUrlRecorderWebContentsObserver is responsible for recording UKM source
 // URLs, for all (any only) main frame navigations in a given WebContents.
 // SourceUrlRecorderWebContentsObserver records both the final URL for a
@@ -38,10 +43,19 @@ class SourceUrlRecorderWebContentsObserver
   // associated with the WebContents, this method is a no-op.
   static void CreateForWebContents(content::WebContents* web_contents);
 
+  // content::WebContentsObserver:
   void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
+  void DidOpenRequestedURL(content::WebContents* new_contents,
+                           content::RenderFrameHost* source_render_frame_host,
+                           const GURL& url,
+                           const content::Referrer& referrer,
+                           WindowOpenDisposition disposition,
+                           ui::PageTransition transition,
+                           bool started_from_context_menu,
+                           bool renderer_initiated) override;
 
   ukm::SourceId GetLastCommittedSourceId() const;
 
@@ -83,7 +97,17 @@ class SourceUrlRecorderWebContentsObserver
   };
   std::vector<PendingEvent> pending_document_created_events_;
 
-  int64_t last_committed_source_id_;
+  SourceId last_committed_source_id_;
+
+  // The source id before |last_committed_source_id_|.
+  SourceId previous_committed_source_id_;
+
+  // The source id of the last committed source in the tab that opened this tab.
+  // Will be set to kInvalidSourceId after the first navigation in this tab is
+  // finished.
+  SourceId opener_source_id_;
+
+  const int64_t tab_id_;
 
   DISALLOW_COPY_AND_ASSIGN(SourceUrlRecorderWebContentsObserver);
 };
@@ -92,7 +116,10 @@ SourceUrlRecorderWebContentsObserver::SourceUrlRecorderWebContentsObserver(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       bindings_(web_contents, this),
-      last_committed_source_id_(ukm::kInvalidSourceId) {}
+      last_committed_source_id_(ukm::kInvalidSourceId),
+      previous_committed_source_id_(ukm::kInvalidSourceId),
+      opener_source_id_(ukm::kInvalidSourceId),
+      tab_id_(CreateUniqueTabId()) {}
 
 void SourceUrlRecorderWebContentsObserver::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
@@ -127,6 +154,7 @@ void SourceUrlRecorderWebContentsObserver::DidFinishNavigation(
   DCHECK(!navigation_handle->IsSameDocument());
 
   if (navigation_handle->HasCommitted()) {
+    previous_committed_source_id_ = last_committed_source_id_;
     last_committed_source_id_ = ukm::ConvertToSourceId(
         navigation_handle->GetNavigationId(), ukm::SourceIdType::NAVIGATION_ID);
   }
@@ -141,6 +169,26 @@ void SourceUrlRecorderWebContentsObserver::DidFinishNavigation(
   MaybeRecordUrl(navigation_handle, initial_url);
 
   MaybeFlushPendingEvents();
+
+  // Reset the opener source id. Only the first source in a tab should have an
+  // opener.
+  opener_source_id_ = kInvalidSourceId;
+}
+
+void SourceUrlRecorderWebContentsObserver::DidOpenRequestedURL(
+    content::WebContents* new_contents,
+    content::RenderFrameHost* source_render_frame_host,
+    const GURL& url,
+    const content::Referrer& referrer,
+    WindowOpenDisposition disposition,
+    ui::PageTransition transition,
+    bool started_from_context_menu,
+    bool renderer_initiated) {
+  auto* new_recorder =
+      SourceUrlRecorderWebContentsObserver::FromWebContents(new_contents);
+  if (!new_recorder)
+    return;
+  new_recorder->opener_source_id_ = GetLastCommittedSourceId();
 }
 
 ukm::SourceId SourceUrlRecorderWebContentsObserver::GetLastCommittedSourceId()
@@ -195,13 +243,23 @@ void SourceUrlRecorderWebContentsObserver::MaybeRecordUrl(
   if (!ukm_recorder)
     return;
 
-  const ukm::SourceId source_id = ukm::ConvertToSourceId(
-      navigation_handle->GetNavigationId(), ukm::SourceIdType::NAVIGATION_ID);
-  ukm_recorder->UpdateNavigationURL(source_id, initial_url);
-
   const GURL& final_url = navigation_handle->GetURL();
+
+  UkmSource::NavigationData navigation_data;
+  navigation_data.url = final_url;
   if (final_url != initial_url)
-    ukm_recorder->UpdateNavigationURL(source_id, final_url);
+    navigation_data.initial_url = initial_url;
+
+  // Careful note: the current navigation may have failed.
+  navigation_data.previous_source_id = navigation_handle->HasCommitted()
+                                           ? previous_committed_source_id_
+                                           : last_committed_source_id_;
+  navigation_data.opener_source_id = opener_source_id_;
+  navigation_data.tab_id = tab_id_;
+
+  const ukm::SourceId source_id = ukm::ConvertToSourceId(
+      navigation_handle->GetNavigationId(), ukm::SourceIdType::NAVIGATION_ID);
+  ukm_recorder->RecordNavigation(source_id, navigation_data);
 }
 
 // static

components/ukm/ukm_recorder_impl.cc

@@ -139,6 +139,11 @@ GURL SanitizeURL(const GURL& url) {
   if (url.SchemeIs(url::kAboutScheme) || url.SchemeIs("chrome")) {
     remove_params.ClearQuery();
   }
+  if (url.SchemeIs(kExtensionScheme)) {
+    remove_params.ClearPath();
+    remove_params.ClearQuery();
+    remove_params.ClearRef();
+  }
   return url.ReplaceComponents(remove_params);
 }
 
@@ -391,72 +396,115 @@ bool UkmRecorderImpl::ShouldRestrictToWhitelistedEntries() const {
 void UkmRecorderImpl::UpdateSourceURL(SourceId source_id,
                                       const GURL& unsanitized_url) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  const GURL sanitized_url = SanitizeURL(unsanitized_url);
+  if (!ShouldRecordUrl(source_id, sanitized_url))
+    return;
+
+  if (base::ContainsKey(recordings_.sources, source_id))
+    return;
+
+  if (recordings_.sources.size() >= GetMaxSources()) {
+    RecordDroppedSource(DroppedDataReason::MAX_HIT);
+    return;
+  }
+  recordings_.sources.emplace(
+      source_id, std::make_unique<UkmSource>(source_id, sanitized_url));
+}
+
+void UkmRecorderImpl::UpdateAppURL(SourceId source_id, const GURL& url) {
+  if (!extensions_enabled_) {
+    RecordDroppedSource(DroppedDataReason::EXTENSION_URLS_DISABLED);
+    return;
+  }
+  UpdateSourceURL(source_id, url);
+}
+
+void UkmRecorderImpl::RecordNavigation(
+    SourceId source_id,
+    const UkmSource::NavigationData& unsanitized_navigation_data) {
+  DCHECK(GetSourceIdType(source_id) == SourceIdType::NAVIGATION_ID);
+  // TODO(csharrison): This is a bit messy because it tries not to change
+  // existing behavior from previous code, where if |final_url| should not be
+  // recorded, the entry assumes |initial_url| is the final url. This should be
+  // changed so that the Source isn't even recorded at all if the final URL
+  // should not be recorded.
+  std::vector<GURL> urls;
+  GURL sanitized_initial_url =
+      SanitizeURL(unsanitized_navigation_data.initial_url);
+  GURL sanitized_final_url = SanitizeURL(unsanitized_navigation_data.url);
+  if (!sanitized_initial_url.is_empty() &&
+      ShouldRecordUrl(source_id, sanitized_initial_url)) {
+    urls.push_back(sanitized_initial_url);
+  }
+  if (ShouldRecordUrl(source_id, sanitized_final_url)) {
+    urls.push_back(sanitized_final_url);
+  }
+
+  // Neither URLs passed the ShouldRecordUrl check, so do not create a new
+  // Source for them.
+  if (urls.empty())
+    return;
 
+  UkmSource::NavigationData sanitized_navigation_data =
+      unsanitized_navigation_data.CopyWithSanitizedUrls(urls);
+  DCHECK(!base::ContainsKey(recordings_.sources, source_id));
+  if (recordings_.sources.size() >= GetMaxSources()) {
+    RecordDroppedSource(DroppedDataReason::MAX_HIT);
+    return;
+  }
+  recordings_.sources.emplace(
+      source_id,
+      std::make_unique<UkmSource>(source_id, sanitized_navigation_data));
+}
+
+bool UkmRecorderImpl::ShouldRecordUrl(SourceId source_id,
+                                      const GURL& sanitized_url) {
   if (!recording_enabled_) {
     RecordDroppedSource(DroppedDataReason::RECORDING_DISABLED);
-    return;
+    return false;
   }
 
   if (ShouldRestrictToWhitelistedSourceIds() &&
       !IsWhitelistedSourceId(source_id)) {
     RecordDroppedSource(DroppedDataReason::NOT_WHITELISTED);
-    return;
+    return false;
   }
 
-  if (unsanitized_url.is_empty()) {
+  if (sanitized_url.is_empty()) {
     RecordDroppedSource(DroppedDataReason::EMPTY_URL);
-    return;
+    return false;
   }
 
   recordings_.source_counts.observed++;
+
+  // TODO(csharrison): Move this to RecordNavigation. It would be a behavior
+  // change though, since we would only record _once_ per navigation, and we
+  // wouldn't track e.g. unsupported schemes or other cases where we drop after
+  // this point.
   if (GetSourceIdType(source_id) == SourceIdType::NAVIGATION_ID)
     recordings_.source_counts.navigation_sources++;
 
-  GURL url = SanitizeURL(unsanitized_url);
-
-  if (!HasSupportedScheme(url)) {
+  if (!HasSupportedScheme(sanitized_url)) {
     RecordDroppedSource(DroppedDataReason::UNSUPPORTED_URL_SCHEME);
     DVLOG(2) << "Dropped Unsupported UKM URL:" << source_id << ":"
-             << url.spec();
-    return;
+             << sanitized_url.spec();
+    return false;
   }
 
   // Extension URLs need to be specifically enabled and the extension synced.
-  if (url.SchemeIs(kExtensionScheme)) {
+  if (sanitized_url.SchemeIs(kExtensionScheme)) {
+    DCHECK_EQ(sanitized_url.GetWithEmptyPath(), sanitized_url);
     if (!extensions_enabled_) {
       RecordDroppedSource(DroppedDataReason::EXTENSION_URLS_DISABLED);
-      return;
+      return false;
     }
     if (!is_webstore_extension_callback_ ||
-        !is_webstore_extension_callback_.Run(url.host_piece())) {
+        !is_webstore_extension_callback_.Run(sanitized_url.host_piece())) {
       RecordDroppedSource(DroppedDataReason::EXTENSION_NOT_SYNCED);
-      return;
-    }
-    url = url.GetWithEmptyPath();
-  }
-
-  // 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.
-  if (base::ContainsKey(recordings_.sources, source_id)) {
-    recordings_.sources[source_id]->UpdateUrl(url);
-    return;
+      return false;
     }
-
-  if (recordings_.sources.size() >= GetMaxSources()) {
-    RecordDroppedSource(DroppedDataReason::MAX_HIT);
-    return;
   }
-  recordings_.sources.emplace(source_id,
-                              std::make_unique<UkmSource>(source_id, url));
-}
-
-void UkmRecorderImpl::UpdateAppURL(SourceId source_id, const GURL& url) {
-  if (!extensions_enabled_) {
-    RecordDroppedSource(DroppedDataReason::EXTENSION_URLS_DISABLED);
-    return;
-  }
-  UpdateSourceURL(source_id, url);
+  return true;
 }
 
 void UkmRecorderImpl::AddEntry(mojom::UkmEntryPtr entry) {

components/ukm/ukm_recorder_impl.h

@@ -84,6 +84,9 @@ class UkmRecorderImpl : public UkmRecorder {
   // UkmRecorder:
   void UpdateSourceURL(SourceId source_id, const GURL& url) override;
   void UpdateAppURL(SourceId source_id, const GURL& url) override;
+  void RecordNavigation(
+      SourceId source_id,
+      const UkmSource::NavigationData& navigation_data) override;
   using UkmRecorder::RecordOtherURL;
 
   virtual bool ShouldRestrictToWhitelistedSourceIds() const;
@@ -118,6 +121,9 @@ class UkmRecorderImpl : public UkmRecorder {
 
   using MetricAggregateMap = std::map<uint64_t, MetricAggregate>;
 
+  // Returns true if |sanitized_url| should be recorded.
+  bool ShouldRecordUrl(SourceId source_id, const GURL& sanitized_url);
+
   void AddEntry(mojom::UkmEntryPtr entry) override;
 
   // Load sampling configurations from field-trial information.

components/ukm/ukm_service_unittest.cc

@@ -61,6 +61,11 @@ class TestRecordingHelper {
     recorder_->UpdateSourceURL(source_id, url);
   }
 
+  void RecordNavigation(SourceId source_id,
+                        const UkmSource::NavigationData& navigation_data) {
+    recorder_->RecordNavigation(source_id, navigation_data);
+  }
+
  private:
   UkmRecorder* recorder_;
 
@@ -253,10 +258,12 @@ TEST_F(UkmServiceTest, SourceSerialization) {
   service.EnableRecording(/*extensions=*/false);
   service.EnableReporting();
 
+  UkmSource::NavigationData navigation_data;
+  navigation_data.initial_url = GURL("https://google.com/initial");
+  navigation_data.url = GURL("https://google.com/final");
+
   ukm::SourceId id = GetWhitelistedSourceId(0);
-  recorder.UpdateSourceURL(id, GURL("https://google.com/initial"));
-  recorder.UpdateSourceURL(id, GURL("https://google.com/intermediate"));
-  recorder.UpdateSourceURL(id, GURL("https://google.com/foobar"));
+  recorder.RecordNavigation(id, navigation_data);
 
   service.Flush();
   EXPECT_EQ(GetPersistedLogCount(), 1);
@@ -267,7 +274,7 @@ TEST_F(UkmServiceTest, SourceSerialization) {
   const Source& proto_source = proto_report.sources(0);
 
   EXPECT_EQ(id, proto_source.id());
-  EXPECT_EQ(GURL("https://google.com/foobar").spec(), proto_source.url());
+  EXPECT_EQ(GURL("https://google.com/final").spec(), proto_source.url());
   EXPECT_FALSE(proto_source.has_initial_url());
 }
 
@@ -442,9 +449,10 @@ TEST_F(UkmServiceTest, RecordInitialUrl) {
     service.EnableReporting();
 
     ukm::SourceId id = GetWhitelistedSourceId(0);
-    recorder.UpdateSourceURL(id, GURL("https://google.com/initial"));
-    recorder.UpdateSourceURL(id, GURL("https://google.com/intermediate"));
-    recorder.UpdateSourceURL(id, GURL("https://google.com/foobar"));
+    UkmSource::NavigationData navigation_data;
+    navigation_data.initial_url = GURL("https://google.com/initial");
+    navigation_data.url = GURL("https://google.com/final");
+    recorder.RecordNavigation(id, navigation_data);
 
     service.Flush();
     EXPECT_EQ(GetPersistedLogCount(), 1);
@@ -454,7 +462,7 @@ TEST_F(UkmServiceTest, RecordInitialUrl) {
     const Source& proto_source = proto_report.sources(0);
 
     EXPECT_EQ(id, proto_source.id());
-    EXPECT_EQ(GURL("https://google.com/foobar").spec(), proto_source.url());
+    EXPECT_EQ(GURL("https://google.com/final").spec(), proto_source.url());
     EXPECT_EQ(should_record_initial_url, proto_source.has_initial_url());
     if (should_record_initial_url) {
       EXPECT_EQ(GURL("https://google.com/initial").spec(),

ios/chrome/browser/metrics/ukm_url_recorder.mm

@@ -134,13 +134,18 @@ void SourceUrlRecorderWebStateObserver::MaybeRecordUrl(
   if (!ukm_recorder)
     return;
 
-  const SourceId source_id = ConvertToSourceId(
-      navigation_context->GetNavigationId(), SourceIdType::NAVIGATION_ID);
-  ukm_recorder->UpdateNavigationURL(source_id, initial_url);
-
   const GURL& final_url = navigation_context->GetUrl();
+
+  UkmSource::NavigationData navigation_data;
+  navigation_data.url = final_url;
   if (final_url != initial_url)
-    ukm_recorder->UpdateNavigationURL(source_id, final_url);
+    navigation_data.initial_url = initial_url;
+
+  // TODO(crbug.com/873316): Fill out the other fields in NavigationData.
+
+  const ukm::SourceId source_id = ukm::ConvertToSourceId(
+      navigation_context->GetNavigationId(), ukm::SourceIdType::NAVIGATION_ID);
+  ukm_recorder->RecordNavigation(source_id, navigation_data);
 }
 
 }  // namespace internal

services/metrics/public/cpp/delegating_ukm_recorder.cc

@@ -44,16 +44,24 @@ void DelegatingUkmRecorder::UpdateSourceURL(SourceId source_id,
         << "UpdateSourceURL invoked for NAVIGATION_ID or APP_ID SourceId";
     return;
   }
-  UpdateSourceURLImpl(source_id, url);
+
+  base::AutoLock auto_lock(lock_);
+  for (auto& iterator : delegates_)
+    iterator.second.UpdateSourceURL(source_id, url);
 }
 
-void DelegatingUkmRecorder::UpdateNavigationURL(SourceId source_id,
-                                                const GURL& url) {
+void DelegatingUkmRecorder::RecordNavigation(
+    SourceId source_id,
+    const UkmSource::NavigationData& navigation_data) {
   if (GetSourceIdType(source_id) != SourceIdType::NAVIGATION_ID) {
     DLOG(FATAL) << "UpdateNavigationURL invoked for non-NAVIGATION_ID SourceId";
     return;
   }
-  UpdateSourceURLImpl(source_id, url);
+
+  base::AutoLock auto_lock(lock_);
+  for (auto& iterator : delegates_) {
+    iterator.second.RecordNavigation(source_id, navigation_data);
+  }
 }
 
 void DelegatingUkmRecorder::UpdateAppURL(SourceId source_id, const GURL& url) {
@@ -66,13 +74,6 @@ void DelegatingUkmRecorder::UpdateAppURL(SourceId source_id, const GURL& url) {
     iterator.second.UpdateAppURL(source_id, url);
 }
 
-void DelegatingUkmRecorder::UpdateSourceURLImpl(SourceId source_id,
-                                                const GURL& url) {
-  base::AutoLock auto_lock(lock_);
-  for (auto& iterator : delegates_)
-    iterator.second.UpdateSourceURL(source_id, url);
-}
-
 void DelegatingUkmRecorder::AddEntry(mojom::UkmEntryPtr entry) {
   base::AutoLock auto_lock(lock_);
   // If there is exactly one delegate, just forward the call.
@@ -116,6 +117,18 @@ void DelegatingUkmRecorder::Delegate::UpdateAppURL(ukm::SourceId source_id,
                                                    ptr_, source_id, url));
 }
 
+void DelegatingUkmRecorder::Delegate::RecordNavigation(
+    ukm::SourceId source_id,
+    const UkmSource::NavigationData& navigation_data) {
+  if (task_runner_->RunsTasksInCurrentSequence()) {
+    ptr_->RecordNavigation(source_id, navigation_data);
+    return;
+  }
+  task_runner_->PostTask(
+      FROM_HERE, base::BindOnce(&UkmRecorder::RecordNavigation, ptr_, source_id,
+                                navigation_data));
+}
+
 void DelegatingUkmRecorder::Delegate::AddEntry(mojom::UkmEntryPtr entry) {
   if (task_runner_->RunsTasksInCurrentSequence()) {
     ptr_->AddEntry(std::move(entry));

services/metrics/public/cpp/delegating_ukm_recorder.h

@@ -50,15 +50,11 @@ class METRICS_EXPORT DelegatingUkmRecorder : public UkmRecorder {
   // UkmRecorder:
   void UpdateSourceURL(SourceId source_id, const GURL& url) override;
   void UpdateAppURL(SourceId source_id, const GURL& url) override;
+  void RecordNavigation(
+      SourceId source_id,
+      const UkmSource::NavigationData& navigation_data) override;
   void AddEntry(mojom::UkmEntryPtr entry) override;
 
-  void UpdateSourceURLImpl(SourceId source_id, const GURL& url);
-
-  // UpdateNavigationURL provides a variation of the UpdateSourceURL API for
-  // recording NAVIGATION_ID sources. This method should only be called by
-  // SourceUrlRecorderWebContentsObserver.
-  void UpdateNavigationURL(SourceId source_id, const GURL& url);
-
   class Delegate final {
    public:
     Delegate(scoped_refptr<base::SequencedTaskRunner> task_runner,
@@ -68,6 +64,8 @@ class METRICS_EXPORT DelegatingUkmRecorder : public UkmRecorder {
 
     void UpdateSourceURL(SourceId source_id, const GURL& url);
     void UpdateAppURL(SourceId source_id, const GURL& url);
+    void RecordNavigation(SourceId source_id,
+                          const UkmSource::NavigationData& navigation_data);
     void AddEntry(mojom::UkmEntryPtr entry);
 
    private:

services/metrics/public/cpp/mojo_ukm_recorder.cc

@@ -37,6 +37,12 @@ void MojoUkmRecorder::UpdateAppURL(SourceId source_id, const GURL& url) {
   NOTREACHED();
 }
 
+void MojoUkmRecorder::RecordNavigation(
+    SourceId source_id,
+    const UkmSource::NavigationData& navigation_data) {
+  NOTREACHED();
+}
+
 void MojoUkmRecorder::AddEntry(mojom::UkmEntryPtr entry) {
   interface_->AddEntry(std::move(entry));
 }

services/metrics/public/cpp/mojo_ukm_recorder.h

@@ -45,6 +45,9 @@ class METRICS_EXPORT MojoUkmRecorder : public UkmRecorder {
   // UkmRecorder:
   void UpdateSourceURL(SourceId source_id, const GURL& url) override;
   void UpdateAppURL(SourceId source_id, const GURL& url) override;
+  void RecordNavigation(
+      SourceId source_id,
+      const UkmSource::NavigationData& navigation_data) override;
   void AddEntry(mojom::UkmEntryPtr entry) override;
 
   mojom::UkmRecorderInterfacePtr interface_;

services/metrics/public/cpp/ukm_recorder.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/threading/thread_checker.h"
 #include "services/metrics/public/cpp/metrics_export.h"
+#include "services/metrics/public/cpp/ukm_source.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
 #include "services/metrics/public/mojom/ukm_interface.mojom.h"
 #include "url/gurl.h"
@@ -132,6 +133,13 @@ class METRICS_EXPORT UkmRecorder {
   // should only be called by AppSourceUrlRecorder and DelegatingUkmRecorder.
   virtual void UpdateAppURL(SourceId source_id, const GURL& url) = 0;
 
+  // Associates navigation data with the UkmSource keyed by |source_id|. This
+  // should only be called by SourceUrlRecorderWebContentsObserver, for
+  // navigation sources.
+  virtual void RecordNavigation(
+      SourceId source_id,
+      const UkmSource::NavigationData& navigation_data) = 0;
+
   DISALLOW_COPY_AND_ASSIGN(UkmRecorder);
 };
 

services/metrics/public/cpp/ukm_source.cc

@@ -39,28 +39,49 @@ void UkmSource::SetCustomTabVisible(bool visible) {
   g_custom_tab_state = visible ? kCustomTabTrue : kCustomTabFalse;
 }
 
+UkmSource::NavigationData UkmSource::NavigationData::CopyWithSanitizedUrls(
+    std::vector<GURL> sanitized_urls) const {
+  DCHECK_LE(sanitized_urls.size(), 2u);
+  DCHECK(!sanitized_urls.empty());
+
+  NavigationData sanitized_navigation_data;
+
+  // TODO(csharrison): Migrate this class to internally keep a vector<GURL>
+  // rather than two separate members.
+  sanitized_navigation_data.url = sanitized_urls.back();
+  if (sanitized_urls.size() == 2u)
+    sanitized_navigation_data.initial_url = sanitized_urls.front();
+
+  sanitized_navigation_data.previous_source_id = previous_source_id;
+  sanitized_navigation_data.opener_source_id = opener_source_id;
+  sanitized_navigation_data.tab_id = tab_id;
+  return sanitized_navigation_data;
+}
+
 UkmSource::UkmSource(ukm::SourceId id, const GURL& url)
     : id_(id),
-      url_(url),
       custom_tab_state_(g_custom_tab_state),
       creation_time_(base::TimeTicks::Now()) {
-  DCHECK(!url_.is_empty());
+  navigation_data_.url = url;
+  DCHECK(!url.is_empty());
+}
+
+UkmSource::UkmSource(ukm::SourceId id, const NavigationData& navigation_data)
+    : id_(id),
+      navigation_data_(navigation_data),
+      custom_tab_state_(g_custom_tab_state),
+      creation_time_(base::TimeTicks::Now()) {
+  DCHECK(GetSourceIdType(id_) == SourceIdType::NAVIGATION_ID);
+  DCHECK(!navigation_data.url.is_empty());
 }
 
 UkmSource::~UkmSource() = default;
 
-void UkmSource::UpdateUrl(const GURL& url) {
-  DCHECK(!url.is_empty());
-  if (url_ == url)
+void UkmSource::UpdateUrl(const GURL& new_url) {
+  DCHECK(!new_url.is_empty());
+  if (url() == new_url)
     return;
-  // We only track multiple URLs for navigation-based Sources.  These are
-  // currently the only sources that intentionally record multiple entries,
-  // and this makes it easier to compare other source URLs against a
-  // whitelist.
-  if (initial_url_.is_empty() &&
-      GetSourceIdType(id_) == SourceIdType::NAVIGATION_ID)
-    initial_url_ = url_;
-  url_ = url;
+  navigation_data_.url = new_url;
 }
 
 void UkmSource::PopulateProto(Source* proto_source) const {
@@ -69,14 +90,16 @@ void UkmSource::PopulateProto(Source* proto_source) const {
   DCHECK(!proto_source->has_initial_url());
 
   proto_source->set_id(id_);
-  proto_source->set_url(GetShortenedURL(url_));
-  if (!initial_url_.is_empty()) {
+  proto_source->set_url(GetShortenedURL(url()));
+  if (!initial_url().is_empty()) {
     DCHECK_EQ(SourceIdType::NAVIGATION_ID, GetSourceIdType(id_));
-    proto_source->set_initial_url(GetShortenedURL(initial_url_));
+    proto_source->set_initial_url(GetShortenedURL(initial_url()));
   }
 
   if (custom_tab_state_ != kCustomTabUnset)
     proto_source->set_is_custom_tab(custom_tab_state_ == kCustomTabTrue);
+
+  // TODO(csharrison): Populate other fields from |navigation_data_|.
 }
 
 }  // namespace ukm

services/metrics/public/cpp/ukm_source.h

@@ -6,6 +6,7 @@
 #define SERVICES_METRICS_PUBLIC_CPP_UKM_SOURCE_H_
 
 #include <map>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/time/time.h"
@@ -27,13 +28,46 @@ class METRICS_EXPORT UkmSource {
     kCustomTabFalse,
   };
 
+  // Extra navigation data associated with a particular Source. Currently, all
+  // of these members except |url| are only set for navigation id sources.
+  struct METRICS_EXPORT NavigationData {
+    // Creates a copy of this struct, replacing the URL members with sanitized
+    // versions. Currently, |sanitized_urls| expects a one or two element
+    // vector. The last element in the vector will always be the final URL in
+    // the redirect chain. For two-element vectors, the first URL is assumed to
+    // be the first URL in the redirect chain.
+    NavigationData CopyWithSanitizedUrls(
+        std::vector<GURL> sanitized_urls) const;
+
+    // TODO(csharrison): Convert these URLs to vector<GURL>.
+    // The final, canonical URL for this source.
+    GURL url;
+
+    // The initial URL for this source.
+    GURL initial_url;
+
+    // The previous source id for this tab.
+    SourceId previous_source_id = kInvalidSourceId;
+
+    // The source id for the source which opened this tab. This should be set to
+    // kInvalidSourceId for all but the first navigation in the tab.
+    SourceId opener_source_id = kInvalidSourceId;
+
+    // A unique identifier for the tab the source navigated in. Tab ids should
+    // be increasing over time within a session.
+    int64_t tab_id = 0;
+  };
+
   UkmSource(SourceId id, const GURL& url);
+  UkmSource(SourceId id, const NavigationData& data);
   ~UkmSource();
 
   ukm::SourceId id() const { return id_; }
 
-  const GURL& initial_url() const { return initial_url_; }
-  const GURL& url() const { return url_; }
+  const GURL& initial_url() const { return navigation_data_.initial_url; }
+  const GURL& url() const { return navigation_data_.url; }
+
+  const NavigationData& navigation_data() const { return navigation_data_; }
 
   // The object creation time. This is for internal purposes only and is not
   // intended to be anything useful for UKM clients.
@@ -51,13 +85,7 @@ class METRICS_EXPORT UkmSource {
  private:
   const ukm::SourceId id_;
 
-  // The final, canonical URL for this source.
-  GURL url_;
-
-  // The initial URL for this source.
-  // Only set for navigation-id based sources where more than one value of URL
-  // was recorded.
-  GURL initial_url_;
+  NavigationData navigation_data_;
 
   // A flag indicating if metric was collected in a custom tab. This is set
   // automatically when the object is created and so represents the state when