commit dfb96674011cea66a51196e110a4b78f70351a8d
author Yi Gu <yigu@chromium.org> Fri Nov 27 05:19:31 2020
committer Commit Bot <commit-bot@chromium.org> Fri Nov 27 05:19:31 2020
tree 259f2e348e6d3b5db4cfef74274bc1cb8a05406d
parent 6e9d22d3bb7a657d416e7fbe7179f5e309d5df72

[WebOTP] Fix flaky test SmsBrowserTest.RecordUserCancelledAsOutcome

The test is flaky because sometimes the histogram "does not exist" even
though the code is executed. This is because we record UKM first and the
RunLoop is only guarding UKM. i.e. FetchHistogramsFromChildProcesses()
may reach the child processes before the UMA is recorded.

Bug: 1141862
Change-Id: Ibd8f70c0b78963798d9faeb6c94fbf18ee15ac7e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2513564
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Commit-Queue: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#831478}

content/browser/sms/sms_browsertest.cc

diff --git a/content/browser/sms/sms_browsertest.cc b/content/browser/sms/sms_browsertest.cc
index 7e61367..271b9699 100644
--- a/content/browser/sms/sms_browsertest.cc
+++ b/content/browser/sms/sms_browsertest.cc
@@ -898,8 +898,7 @@
   ExpectNoOutcomeUKM();
 }
 
-// Disabled test: crbug.com/1146218
-IN_PROC_BROWSER_TEST_F(SmsBrowserTest, DISABLED_RecordUserCancelledAsOutcome) {
+IN_PROC_BROWSER_TEST_F(SmsBrowserTest, RecordUserCancelledAsOutcome) {
   base::HistogramTester histogram_tester;
   base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
       switches::kWebOtpBackend, switches::kWebOtpBackendUserConsent);

third_party/blink/renderer/modules/credentialmanager/credential_metrics.cc

diff --git a/third_party/blink/renderer/modules/credentialmanager/credential_metrics.cc b/third_party/blink/renderer/modules/credentialmanager/credential_metrics.cc
index 0dd32f13..6aefd4c1 100644
--- a/third_party/blink/renderer/modules/credentialmanager/credential_metrics.cc
+++ b/third_party/blink/renderer/modules/credentialmanager/credential_metrics.cc
@@ -13,19 +13,25 @@
 void RecordSmsOutcome(WebOTPServiceOutcome outcome,
                       ukm::SourceId source_id,
                       ukm::UkmRecorder* ukm_recorder) {
+  // In |SmsBrowserTest| we wait for UKM to be recorded to avoid race condition
+  // between outcome capture and evaluation. Recording UMA before UKM makes sure
+  // that |FetchHistogramsFromChildProcesses| reaches the child processes after
+  // UMA is recorded.
+  UMA_HISTOGRAM_ENUMERATION("Blink.Sms.Receive.Outcome", outcome);
+
   DCHECK_NE(source_id, ukm::kInvalidSourceId);
   DCHECK(ukm_recorder);
 
   ukm::builders::SMSReceiver builder(source_id);
   builder.SetOutcome(static_cast<int>(outcome));
   builder.Record(ukm_recorder);
-
-  UMA_HISTOGRAM_ENUMERATION("Blink.Sms.Receive.Outcome", outcome);
 }
 
 void RecordSmsSuccessTime(base::TimeDelta duration,
                           ukm::SourceId source_id,
                           ukm::UkmRecorder* ukm_recorder) {
+  UMA_HISTOGRAM_MEDIUM_TIMES("Blink.Sms.Receive.TimeSuccess", duration);
+
   DCHECK_NE(source_id, ukm::kInvalidSourceId);
   DCHECK(ukm_recorder);
 
@@ -34,13 +40,13 @@
   builder.SetTimeSuccessMs(
       ukm::GetExponentialBucketMinForUserTiming(duration.InMilliseconds()));
   builder.Record(ukm_recorder);
-
-  UMA_HISTOGRAM_MEDIUM_TIMES("Blink.Sms.Receive.TimeSuccess", duration);
 }
 
 void RecordSmsUserCancelTime(base::TimeDelta duration,
                              ukm::SourceId source_id,
                              ukm::UkmRecorder* ukm_recorder) {
+  UMA_HISTOGRAM_MEDIUM_TIMES("Blink.Sms.Receive.TimeUserCancel", duration);
+
   DCHECK_NE(source_id, ukm::kInvalidSourceId);
   DCHECK(ukm_recorder);
 
@@ -49,8 +55,6 @@
   builder.SetTimeUserCancelMs(
       ukm::GetExponentialBucketMinForUserTiming(duration.InMilliseconds()));
   builder.Record(ukm_recorder);
-
-  UMA_HISTOGRAM_MEDIUM_TIMES("Blink.Sms.Receive.TimeUserCancel", duration);
 }
 
 void RecordSmsCancelTime(base::TimeDelta duration) {