Delay binding of SourceId to URL in PasswordFormMetricsRecorder

This CL delays the binding of a ukm::SourceID to a URL in the
PasswordFormMetricsRecorder until the recorder is destroyed. The reason for
this is that long-lived tabs/WebContents would bind the URL at creation. But by
destruction (i.e. reporting time) the binding is purged and the data is
discarded.

Bug: 732846
Change-Id: I0a7c654ddc1a866e4abbe51206f62949500b5ba9
Reviewed-on: https://chromium-review.googlesource.com/577887
Commit-Queue: Dominic Battré <battre@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488214}

chrome/browser/password_manager/save_password_infobar_delegate_android_unittest.cc

@@ -185,13 +185,9 @@ TEST_P(SavePasswordInfoBarDelegateTestForUKMs, VerifyUKMRecording) {
   ukm::TestUkmRecorder test_ukm_recorder;
   {
     // Setup metrics recorder
-    ukm::SourceId source_id = test_ukm_recorder.GetNewSourceID();
-    static_cast<ukm::UkmRecorder*>(&test_ukm_recorder)
-        ->UpdateSourceURL(source_id, GURL("https://www.example.com/"));
     auto recorder = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-        true /*is_main_frame_secure*/,
-        PasswordFormMetricsRecorder::CreateUkmEntryBuilder(&test_ukm_recorder,
-                                                           source_id));
+        true /*is_main_frame_secure*/, &test_ukm_recorder,
+        test_ukm_recorder.GetNewSourceID(), GURL("https://www.example.com/"));
 
     // Exercise delegate.
     std::unique_ptr<MockPasswordFormManager> password_form_manager(

chrome/browser/ui/passwords/manage_passwords_bubble_model_unittest.cc

@@ -334,13 +334,10 @@ TEST_F(ManagePasswordsBubbleModelTest, Edit) {
     {
       // Setup metrics recorder
       ukm::SourceId source_id = test_ukm_recorder.GetNewSourceID();
-      static_cast<ukm::UkmRecorder*>(&test_ukm_recorder)
-          ->UpdateSourceURL(source_id, GURL("https://www.example.com/"));
-      auto recorder = base::MakeRefCounted<
-          password_manager::PasswordFormMetricsRecorder>(
-          true /*is_main_frame_secure*/,
-          password_manager::PasswordFormMetricsRecorder::CreateUkmEntryBuilder(
-              &test_ukm_recorder, source_id));
+      auto recorder =
+          base::MakeRefCounted<password_manager::PasswordFormMetricsRecorder>(
+              true /*is_main_frame_secure*/, &test_ukm_recorder, source_id,
+              GURL("https://www.example.com/"));
 
       // Exercise bubble.
       ON_CALL(*controller(), GetPasswordFormMetricsRecorder())
@@ -571,14 +568,11 @@ TEST_F(ManagePasswordsBubbleModelTest, RecordUKMs) {
         ukm::TestUkmRecorder test_ukm_recorder;
         {
           // Setup metrics recorder
-          ukm::SourceId source_id = test_ukm_recorder.GetNewSourceID();
-          static_cast<ukm::UkmRecorder*>(&test_ukm_recorder)
-              ->UpdateSourceURL(source_id, GURL("https://www.example.com/"));
           auto recorder = base::MakeRefCounted<
               password_manager::PasswordFormMetricsRecorder>(
-              true /*is_main_frame_secure*/,
-              password_manager::PasswordFormMetricsRecorder::
-                  CreateUkmEntryBuilder(&test_ukm_recorder, source_id));
+              true /*is_main_frame_secure*/, &test_ukm_recorder,
+              test_ukm_recorder.GetNewSourceID(),
+              GURL("https://www.example.com/"));
 
           // Exercise bubble.
           ON_CALL(*controller(), GetPasswordFormMetricsRecorder())

components/password_manager/core/browser/password_form_manager.cc

@@ -255,9 +255,8 @@ void PasswordFormManager::Init(
   metrics_recorder_ = std::move(metrics_recorder);
   if (!metrics_recorder_) {
     metrics_recorder_ = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-        client_->IsMainFrameSecure(),
-        PasswordFormMetricsRecorder::CreateUkmEntryBuilder(
-            client_->GetUkmRecorder(), client_->GetUkmSourceId()));
+        client_->IsMainFrameSecure(), client_->GetUkmRecorder(),
+        client_->GetUkmSourceId(), client_->GetMainFrameURL());
   }
 
   if (owned_form_fetcher_)

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -3843,21 +3843,22 @@ TEST_F(PasswordFormManagerTest, TestUkmForFilling) {
       fetched_forms.push_back(test.fetched_form);
 
     ukm::TestUkmRecorder test_ukm_recorder;
-    client()->GetUkmRecorder()->UpdateSourceURL(client()->GetUkmSourceId(),
-                                                form_to_fill.origin);
-
     {
+      auto metrics_recorder = base::MakeRefCounted<PasswordFormMetricsRecorder>(
+          form_to_fill.origin.SchemeIsCryptographic(), &test_ukm_recorder,
+          test_ukm_recorder.GetNewSourceID(), form_to_fill.origin);
       FakeFormFetcher fetcher;
       PasswordFormManager form_manager(
           password_manager(), client(),
           test.is_http_basic_auth ? nullptr : client()->driver(), form_to_fill,
           base::MakeUnique<NiceMock<MockFormSaver>>(), &fetcher);
-      form_manager.Init(nullptr);
+      form_manager.Init(metrics_recorder);
       fetcher.SetNonFederated(fetched_forms, 0u);
     }
 
     const auto* source =
         test_ukm_recorder.GetSourceForUrl(form_to_fill.origin.spec().c_str());
+    ASSERT_TRUE(source);
     test_ukm_recorder.ExpectMetric(*source, "PasswordForm",
                                    kUkmManagerFillEvent, test.expected_event);
   }

components/password_manager/core/browser/password_form_metrics_recorder.cc

@@ -32,9 +32,17 @@ const char kUkmUserAction[] = "User.Action";
 
 PasswordFormMetricsRecorder::PasswordFormMetricsRecorder(
     bool is_main_frame_secure,
-    std::unique_ptr<ukm::UkmEntryBuilder> ukm_entry_builder)
+    ukm::UkmRecorder* ukm_recorder,
+    ukm::SourceId source_id,
+    const GURL& main_frame_url)
     : is_main_frame_secure_(is_main_frame_secure),
-      ukm_entry_builder_(std::move(ukm_entry_builder)) {}
+      ukm_recorder_(ukm_recorder),
+      source_id_(source_id),
+      main_frame_url_(main_frame_url),
+      ukm_entry_builder_(
+          ukm_recorder
+              ? ukm_recorder->GetEntryBuilder(source_id, "PasswordForm")
+              : nullptr) {}
 
 PasswordFormMetricsRecorder::~PasswordFormMetricsRecorder() {
   UMA_HISTOGRAM_ENUMERATION("PasswordManager.ActionsTakenV3", GetActionsTaken(),
@@ -76,16 +84,11 @@ PasswordFormMetricsRecorder::~PasswordFormMetricsRecorder() {
 
   for (const DetailedUserAction& action : one_time_report_user_actions_)
     RecordUkmMetric(kUkmUserAction, static_cast<int64_t>(action));
-}
 
-// static
-std::unique_ptr<ukm::UkmEntryBuilder>
-PasswordFormMetricsRecorder::CreateUkmEntryBuilder(
-    ukm::UkmRecorder* ukm_recorder,
-    ukm::SourceId source_id) {
-  if (!ukm_recorder)
-    return nullptr;
-  return ukm_recorder->GetEntryBuilder(source_id, "PasswordForm");
+  // Bind |main_frame_url_| to |source_id_| directly before sending the content
+  // of |ukm_recorder_| to ensure that the binding has not been purged already.
+  if (ukm_recorder_)
+    ukm_recorder_->UpdateSourceURL(source_id_, main_frame_url_);
 }
 
 void PasswordFormMetricsRecorder::MarkGenerationAvailable() {

components/password_manager/core/browser/password_form_metrics_recorder.h

@@ -17,6 +17,7 @@
 #include "components/password_manager/core/browser/password_form_user_action.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
+#include "url/gurl.h"
 
 namespace password_manager {
 
@@ -96,19 +97,18 @@ class FormFetcher;
 class PasswordFormMetricsRecorder
     : public base::RefCounted<PasswordFormMetricsRecorder> {
  public:
-  // |ukm_entry_builder| is the destination into which UKM metrics are recorded.
-  // It may be nullptr, in which case no UKM metrics are recorded. This should
-  // be created via the static CreateUkmEntryBuilder() method of this class.
-  PasswordFormMetricsRecorder(
-      bool is_main_frame_secure,
-      std::unique_ptr<ukm::UkmEntryBuilder> ukm_entry_builder);
-
-  // Creates a UkmEntryBuilder that can be used to record metrics into the event
-  // "PasswordForm". |source_id| should be bound the the correct URL in the
-  // |ukm_recorder| when this function is called.
-  static std::unique_ptr<ukm::UkmEntryBuilder> CreateUkmEntryBuilder(
-      ukm::UkmRecorder* ukm_recorder,
-      ukm::SourceId source_id);
+  // Records UKM metrics and reports them on destruction. The |source_id| is
+  // (re-)bound to |main_frame_url| shortly before reporting. As such it is
+  // crucial that the |source_id| is never bound to a different URL by another
+  // consumer. The reason for this late binding is that metrics can be
+  // collected for a WebContents for a long period of time and by the time the
+  // reporting happens, the binding of |source_id| to |main_frame_url| is
+  // already purged. |ukm_recorder| may be a nullptr, in which case no UKM
+  // metrics are recorded.
+  PasswordFormMetricsRecorder(bool is_main_frame_secure,
+                              ukm::UkmRecorder* ukm_recorder,
+                              ukm::SourceId source_id,
+                              const GURL& main_frame_url);
 
   // ManagerAction - What does the PasswordFormManager do with this form? Either
   // it fills it, or it doesn't. If it doesn't fill it, that's either
@@ -367,6 +367,18 @@ class PasswordFormMetricsRecorder
   // data the user has entered.
   SubmittedFormType submitted_form_type_ = kSubmittedFormTypeUnspecified;
 
+  // Recorder to which metrics are sent. Has to outlive this
+  // PasswordFormMetricsRecorder.
+  ukm::UkmRecorder* ukm_recorder_;
+
+  // A SourceId of |ukm_recorder_|. This id gets bound to |main_frame_url_| on
+  // destruction. It can be shared across multiple metrics recorders as long as
+  // they all bind it to the same URL.
+  ukm::SourceId source_id_;
+
+  // URL for which UKMs are reported.
+  GURL main_frame_url_;
+
   // Records URL keyed metrics (UKMs) and submits them on its destruction. May
   // be a nullptr in which case no recording is expected.
   std::unique_ptr<ukm::UkmEntryBuilder> ukm_entry_builder_;

components/password_manager/core/browser/password_form_metrics_recorder_unittest.cc

@@ -22,13 +22,12 @@ namespace {
 constexpr char kTestUrl[] = "https://www.example.com/";
 
 // Create a UkmEntryBuilder with a SourceId that is initialized for kTestUrl.
-std::unique_ptr<ukm::UkmEntryBuilder> CreateUkmEntryBuilder(
+scoped_refptr<PasswordFormMetricsRecorder> CreatePasswordFormMetricsRecorder(
+    bool is_main_frame_secure,
     ukm::TestUkmRecorder* test_ukm_recorder) {
-  ukm::SourceId source_id = test_ukm_recorder->GetNewSourceID();
-  static_cast<ukm::UkmRecorder*>(test_ukm_recorder)
-      ->UpdateSourceURL(source_id, GURL(kTestUrl));
-  return PasswordFormMetricsRecorder::CreateUkmEntryBuilder(test_ukm_recorder,
-                                                            source_id);
+  return base::MakeRefCounted<PasswordFormMetricsRecorder>(
+      is_main_frame_secure, test_ukm_recorder,
+      test_ukm_recorder->GetNewSourceID(), GURL(kTestUrl));
 }
 
 // TODO(crbug.com/738921) Replace this with generalized infrastructure.
@@ -87,9 +86,8 @@ TEST(PasswordFormMetricsRecorder, Generation) {
     // Use a scoped PasswordFromMetricsRecorder because some metrics are recored
     // on destruction.
     {
-      auto recorder = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-          /*is_main_frame_secure*/ true,
-          CreateUkmEntryBuilder(&test_ukm_recorder));
+      auto recorder = CreatePasswordFormMetricsRecorder(
+          /*is_main_frame_secure*/ true, &test_ukm_recorder);
       if (test.generation_available)
         recorder->MarkGenerationAvailable();
       recorder->SetHasGeneratedPassword(test.has_generated_password);
@@ -247,8 +245,8 @@ TEST(PasswordFormMetricsRecorder, Actions) {
     // Use a scoped PasswordFromMetricsRecorder because some metrics are recored
     // on destruction.
     {
-      auto recorder = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-          test.is_main_frame_secure, CreateUkmEntryBuilder(&test_ukm_recorder));
+      auto recorder = CreatePasswordFormMetricsRecorder(
+          test.is_main_frame_secure, &test_ukm_recorder);
 
       recorder->SetManagerAction(test.manager_action);
       if (test.user_action != UserAction::kNone)
@@ -314,9 +312,8 @@ TEST(PasswordFormMetricsRecorder, ActionSequence) {
   // Use a scoped PasswordFromMetricsRecorder because some metrics are recored
   // on destruction.
   {
-    auto recorder = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-        /*is_main_frame_secure*/ true,
-        CreateUkmEntryBuilder(&test_ukm_recorder));
+    auto recorder = CreatePasswordFormMetricsRecorder(
+        true /*is_main_frame_secure*/, &test_ukm_recorder);
     recorder->SetManagerAction(
         PasswordFormMetricsRecorder::kManagerActionAutofilled);
     recorder->SetUserAction(UserAction::kChoosePslMatch);
@@ -360,8 +357,8 @@ TEST(PasswordFormMetricsRecorder, SubmittedFormType) {
     // Use a scoped PasswordFromMetricsRecorder because some metrics are recored
     // on destruction.
     {
-      auto recorder = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-          test.is_main_frame_secure, CreateUkmEntryBuilder(&test_ukm_recorder));
+      auto recorder = CreatePasswordFormMetricsRecorder(
+          test.is_main_frame_secure, &test_ukm_recorder);
       recorder->SetSubmittedFormType(test.form_type);
     }
 
@@ -447,9 +444,8 @@ TEST(PasswordFormMetricsRecorder, RecordPasswordBubbleShown) {
                  << ", display_disposition = " << test.display_disposition);
     ukm::TestUkmRecorder test_ukm_recorder;
     {
-      auto recorder = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-          true /*is_main_frame_secure*/,
-          CreateUkmEntryBuilder(&test_ukm_recorder));
+      auto recorder = CreatePasswordFormMetricsRecorder(
+          true /*is_main_frame_secure*/, &test_ukm_recorder);
       recorder->RecordPasswordBubbleShown(test.credential_source_type,
                                           test.display_disposition);
     }
@@ -505,9 +501,8 @@ TEST(PasswordFormMetricsRecorder, RecordUIDismissalReason) {
                  << ", dismissal_reason = " << test.dismissal_reason);
     ukm::TestUkmRecorder test_ukm_recorder;
     {
-      auto recorder = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-          true /*is_main_frame_secure*/,
-          CreateUkmEntryBuilder(&test_ukm_recorder));
+      auto recorder = CreatePasswordFormMetricsRecorder(
+          true /*is_main_frame_secure*/, &test_ukm_recorder);
       recorder->RecordPasswordBubbleShown(
           metrics_util::CredentialSourceType::kPasswordManager,
           test.display_disposition);
@@ -530,9 +525,8 @@ TEST(PasswordFormMetricsRecorder, SequencesOfBubbles) {
   using BubbleTrigger = PasswordFormMetricsRecorder::BubbleTrigger;
   ukm::TestUkmRecorder test_ukm_recorder;
   {
-    auto recorder = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-        true /*is_main_frame_secure*/,
-        CreateUkmEntryBuilder(&test_ukm_recorder));
+    auto recorder = CreatePasswordFormMetricsRecorder(
+        true /*is_main_frame_secure*/, &test_ukm_recorder);
     // Open and confirm an automatically triggered saving prompt.
     recorder->RecordPasswordBubbleShown(
         metrics_util::CredentialSourceType::kPasswordManager,
@@ -574,9 +568,8 @@ TEST(PasswordFormMetricsRecorder, RecordDetailedUserAction) {
   const DetailedUserAction kRepeatedAction = DetailedUserAction::kUnknown;
   ukm::TestUkmRecorder test_ukm_recorder;
   {
-    auto recorder = base::MakeRefCounted<PasswordFormMetricsRecorder>(
-        true /*is_main_frame_secure*/,
-        CreateUkmEntryBuilder(&test_ukm_recorder));
+    auto recorder = CreatePasswordFormMetricsRecorder(
+        true /*is_main_frame_secure*/, &test_ukm_recorder);
     recorder->RecordDetailedUserAction(kOneTimeAction);
     recorder->RecordDetailedUserAction(kOneTimeAction);
     recorder->RecordDetailedUserAction(kRepeatedAction);