AutofillManager reset shouldn't cause the metrics to be dropped

Bug: 904953
Change-Id: I763d48881143645102fdaf5d85cb2ab812e8b8c0
Reviewed-on: https://chromium-review.googlesource.com/c/1334327Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Commit-Queue: Nik Bhagat <nikunjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607856}

components/autofill/core/browser/autofill_manager.cc

@@ -1102,7 +1102,7 @@ void AutofillManager::Reset() {
   AutofillHandler::Reset();
   form_interactions_ukm_logger_.reset(
       new AutofillMetrics::FormInteractionsUkmLogger(
-          client_->GetUkmRecorder()));
+          client_->GetUkmRecorder(), client_->GetUkmSourceId()));
   address_form_event_logger_.reset(new AutofillMetrics::FormEventLogger(
       /*is_for_credit_card=*/false, driver()->IsInMainFrame(),
       form_interactions_ukm_logger_.get()));
@@ -1143,7 +1143,8 @@ AutofillManager::AutofillManager(
           std::make_unique<AutocompleteHistoryManager>(driver, client)),
       form_interactions_ukm_logger_(
           std::make_unique<AutofillMetrics::FormInteractionsUkmLogger>(
-              client->GetUkmRecorder())),
+              client->GetUkmRecorder(),
+              client->GetUkmSourceId())),
       address_form_event_logger_(
           std::make_unique<AutofillMetrics::FormEventLogger>(
               /*is_for_credit_card=*/false,
@@ -1489,9 +1490,7 @@ void AutofillManager::OnFormsParsed(
   DCHECK(!form_structures.empty());
 
   // Setup the url for metrics that we will collect for this form.
-  form_interactions_ukm_logger_->OnFormsParsed(
-      form_structures[0]->ToFormData().main_frame_origin.GetURL(),
-      client_->GetUkmSourceId());
+  form_interactions_ukm_logger_->OnFormsParsed(client_->GetUkmSourceId());
 
   std::vector<FormStructure*> non_queryable_forms;
   std::vector<FormStructure*> queryable_forms;

components/autofill/core/browser/autofill_metrics.cc

@@ -1912,16 +1912,15 @@ void AutofillMetrics::FormEventLogger::Log(
 }
 
 AutofillMetrics::FormInteractionsUkmLogger::FormInteractionsUkmLogger(
-    ukm::UkmRecorder* ukm_recorder)
-    : ukm_recorder_(ukm_recorder) {}
+    ukm::UkmRecorder* ukm_recorder,
+    const ukm::SourceId source_id)
+    : ukm_recorder_(ukm_recorder), source_id_(source_id) {}
 
 void AutofillMetrics::FormInteractionsUkmLogger::OnFormsParsed(
-    const GURL& url,
     const ukm::SourceId source_id) {
   if (ukm_recorder_ == nullptr)
     return;
 
-  url_ = url;
   source_id_ = source_id;
 }
 
@@ -2130,7 +2129,7 @@ void AutofillMetrics::FormInteractionsUkmLogger::LogFormSubmitted(
 }
 
 bool AutofillMetrics::FormInteractionsUkmLogger::CanLog() const {
-  return ukm_recorder_ && url_.is_valid();
+  return ukm_recorder_ != nullptr;
 }
 
 int64_t AutofillMetrics::FormInteractionsUkmLogger::MillisecondsSinceFormParsed(

components/autofill/core/browser/autofill_metrics.h

@@ -786,16 +786,22 @@ class AutofillMetrics {
   // Utility to log URL keyed form interaction events.
   class FormInteractionsUkmLogger {
    public:
-    explicit FormInteractionsUkmLogger(ukm::UkmRecorder* ukm_recorder);
+    FormInteractionsUkmLogger(ukm::UkmRecorder* ukm_recorder,
+                              const ukm::SourceId source_id);
 
     bool has_pinned_timestamp() const { return !pinned_timestamp_.is_null(); }
     void set_pinned_timestamp(base::TimeTicks t) { pinned_timestamp_ = t; }
 
-    const GURL& url() const { return url_; }
-
-    // Initializes this logger with a valid url and source_id.
-    // Unless forms is parsed no autofill UKM can be recorded.
-    void OnFormsParsed(const GURL& url, const ukm::SourceId source_id);
+    // Initializes this logger with a source_id. Unless forms is parsed no
+    // autofill UKM is recorded. However due to autofill_manager resets,
+    // it is possible to have the UKM being recorded after the forms were
+    // parsed. So, rely on autofill_client to pass correct source_id
+    // However during some cases there is a race for setting AutofillClient
+    // and generation of new source_id (by UKM) as they are both observing tab
+    // navigation. Ideally we need to refactor ownership of this logger
+    // so as not to rely on OnFormsParsed to record the metrics correctly.
+    // TODO(nikunjb): Refactor the logger to be owned by AutofillClient.
+    void OnFormsParsed(const ukm::SourceId source_id);
     void LogInteractedWithForm(bool is_for_credit_card,
                                size_t local_record_type_count,
                                size_t server_record_type_count,
@@ -846,8 +852,7 @@ class AutofillMetrics {
         const base::TimeTicks& form_parsed_timestamp) const;
 
     ukm::UkmRecorder* ukm_recorder_;  // Weak reference.
-    ukm::SourceId source_id_ = -1;
-    GURL url_;
+    ukm::SourceId source_id_;
     base::TimeTicks pinned_timestamp_;
   };
 

components/autofill/core/browser/test_autofill_manager.cc

@@ -110,8 +110,7 @@ void TestAutofillManager::AddSeenForm(
   form_structure->SetFieldTypes(heuristic_types, server_types);
   AddSeenFormStructure(std::move(form_structure));
 
-  form_interactions_ukm_logger()->OnFormsParsed(form.main_frame_origin.GetURL(),
-                                                client()->GetUkmSourceId());
+  form_interactions_ukm_logger()->OnFormsParsed(client()->GetUkmSourceId());
 }
 
 void TestAutofillManager::AddSeenFormStructure(