Wait for ukm entry in TryAutoplay

The test fails due to a race of UkmRecorder::AddEntry with DOMMessageQueue::Observe:
https://cs.chromium.org/chromium/src/services/metrics/public/cpp/delegating_ukm_recorder.cc?q=src/services/metrics/public/cpp/delegating_ukm_recorder.cc&sq=package:chromium&g=0&l=137

Bug: 880217
Change-Id: Icf099c3eb1094d8a571dc6f87b152e3c838807dc
Reviewed-on: https://chromium-review.googlesource.com/1204011Reviewed-by: Tommi <tommi@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589317}

chrome/browser/media/autoplay_metrics_browsertest.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/task/post_task.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/ukm/test_ukm_recorder.h"
@@ -18,15 +19,25 @@ namespace {
 
 class AutoplayMetricsBrowserTest : public InProcessBrowserTest {
  public:
+  using Entry = ukm::builders::Media_Autoplay_Attempt;
+  using CreatedEntry = ukm::builders::DocumentCreated;
+
   void SetUpOnMainThread() override {
     host_resolver()->AddRule("*", "127.0.0.1");
     content::SetupCrossSiteRedirector(embedded_test_server());
     ASSERT_TRUE(embedded_test_server()->Start());
   }
 
-  void TryAutoplay(const content::ToRenderFrameHost& adapter) {
+  void TryAutoplay(ukm::TestUkmRecorder& ukm_recorder,
+                   const content::ToRenderFrameHost& adapter) {
+    base::RunLoop run_loop;
+    base::PostDelayedTask(FROM_HERE, run_loop.QuitClosure(),
+                          base::TimeDelta::FromSeconds(10));
+    ukm_recorder.SetOnAddEntryCallback(Entry::kEntryName,
+                                       run_loop.QuitClosure());
     EXPECT_TRUE(ExecuteScriptWithoutUserGesture(adapter.render_frame_host(),
                                                 "tryPlayback();"));
+    run_loop.Run();
   }
 
   void NavigateFrameAndWait(content::RenderFrameHost* rfh, const GURL& url) {
@@ -70,9 +81,6 @@ class AutoplayMetricsBrowserTest : public InProcessBrowserTest {
 
 IN_PROC_BROWSER_TEST_F(AutoplayMetricsBrowserTest, RecordAutoplayAttemptUkm) {
   ukm::TestAutoSetUkmRecorder test_ukm_recorder;
-  using Entry = ukm::builders::Media_Autoplay_Attempt;
-  using CreatedEntry = ukm::builders::DocumentCreated;
-
   GURL main_url(embedded_test_server()->GetURL("example.com",
                                                "/media/autoplay_iframe.html"));
   GURL foo_url(
@@ -82,25 +90,25 @@ IN_PROC_BROWSER_TEST_F(AutoplayMetricsBrowserTest, RecordAutoplayAttemptUkm) {
 
   // Navigate main frame, try play.
   NavigateFrameAndWait(web_contents()->GetMainFrame(), main_url);
-  TryAutoplay(web_contents());
+  TryAutoplay(test_ukm_recorder, web_contents());
 
   // Check that we recorded a UKM event using the main frame URL.
   {
     auto ukm_entries = test_ukm_recorder.GetEntriesByName(Entry::kEntryName);
 
-    EXPECT_EQ(1u, ukm_entries.size());
+    ASSERT_EQ(1u, ukm_entries.size());
     test_ukm_recorder.ExpectEntrySourceHasUrl(ukm_entries[0], main_url);
   }
 
   // Navigate sub frame, try play.
   NavigateFrameAndWait(first_child(), foo_url);
-  TryAutoplay(first_child());
+  TryAutoplay(test_ukm_recorder, first_child());
 
   // Check that we recorded a UKM event that is not keyed to any URL.
   {
     auto ukm_entries = test_ukm_recorder.GetEntriesByName(Entry::kEntryName);
 
-    EXPECT_EQ(2u, ukm_entries.size());
+    ASSERT_EQ(2u, ukm_entries.size());
     EXPECT_FALSE(
         test_ukm_recorder.GetSourceForSourceId(ukm_entries[1]->source_id));
 
@@ -122,13 +130,13 @@ IN_PROC_BROWSER_TEST_F(AutoplayMetricsBrowserTest, RecordAutoplayAttemptUkm) {
 
   // Navigate sub sub frame, try play.
   NavigateFrameAndWait(second_child(), bar_url);
-  TryAutoplay(second_child());
+  TryAutoplay(test_ukm_recorder, second_child());
 
   // Check that we recorded a UKM event that is not keyed to any url.
   {
     auto ukm_entries = test_ukm_recorder.GetEntriesByName(Entry::kEntryName);
 
-    EXPECT_EQ(3u, ukm_entries.size());
+    ASSERT_EQ(3u, ukm_entries.size());
     EXPECT_FALSE(
         test_ukm_recorder.GetSourceForSourceId(ukm_entries[2]->source_id));
 
@@ -150,13 +158,13 @@ IN_PROC_BROWSER_TEST_F(AutoplayMetricsBrowserTest, RecordAutoplayAttemptUkm) {
 
   // Navigate top frame, try play.
   NavigateFrameAndWait(web_contents()->GetMainFrame(), foo_url);
-  TryAutoplay(web_contents());
+  TryAutoplay(test_ukm_recorder, web_contents());
 
   // Check that we recorded a UKM event using the main frame URL.
   {
     auto ukm_entries = test_ukm_recorder.GetEntriesByName(Entry::kEntryName);
 
-    EXPECT_EQ(4u, ukm_entries.size());
+    ASSERT_EQ(4u, ukm_entries.size());
     test_ukm_recorder.ExpectEntrySourceHasUrl(ukm_entries[3], foo_url);
   }
 }

components/ukm/test_ukm_recorder.cc

@@ -56,6 +56,14 @@ bool TestUkmRecorder::ShouldRestrictToWhitelistedEntries() const {
   return false;
 }
 
+void TestUkmRecorder::AddEntry(mojom::UkmEntryPtr entry) {
+  const bool should_run_callback =
+      entry && entry_hash_to_wait_for_ == entry->event_hash;
+  UkmRecorderImpl::AddEntry(std::move(entry));
+  if (should_run_callback && on_add_entry_)
+    std::move(on_add_entry_).Run();
+}
+
 const UkmSource* TestUkmRecorder::GetSourceForSourceId(
     SourceId source_id) const {
   const UkmSource* source = nullptr;
@@ -68,6 +76,12 @@ const UkmSource* TestUkmRecorder::GetSourceForSourceId(
   return source;
 }
 
+void TestUkmRecorder::SetOnAddEntryCallback(base::StringPiece entry_name,
+                                            base::OnceClosure on_add_entry) {
+  on_add_entry_ = std::move(on_add_entry);
+  entry_hash_to_wait_for_ = base::HashMetricName(entry_name);
+}
+
 std::vector<const mojom::UkmEntry*> TestUkmRecorder::GetEntriesByName(
     base::StringPiece entry_name) const {
   uint64_t hash = base::HashMetricName(entry_name);

components/ukm/test_ukm_recorder.h

@@ -31,6 +31,8 @@ class TestUkmRecorder : public UkmRecorderImpl {
   bool ShouldRestrictToWhitelistedSourceIds() const override;
   bool ShouldRestrictToWhitelistedEntries() const override;
 
+  void AddEntry(mojom::UkmEntryPtr entry) override;
+
   size_t sources_count() const { return sources().size(); }
 
   size_t entries_count() const { return entries().size(); }
@@ -47,6 +49,10 @@ class TestUkmRecorder : public UkmRecorderImpl {
   // Gets UkmSource data for a single SourceId.
   const UkmSource* GetSourceForSourceId(ukm::SourceId source_id) const;
 
+  // Sets a callback that will be called when recording an entry for entry name.
+  void SetOnAddEntryCallback(base::StringPiece entry_name,
+                             base::OnceClosure on_add_entry);
+
   // Gets all of the entries recorded for entry name.
   std::vector<const mojom::UkmEntry*> GetEntriesByName(
       base::StringPiece entry_name) const;
@@ -75,6 +81,9 @@ class TestUkmRecorder : public UkmRecorderImpl {
                                        base::StringPiece metric_name);
 
  private:
+  uint64_t entry_hash_to_wait_for_;
+  base::OnceClosure on_add_entry_;
+
   DISALLOW_COPY_AND_ASSIGN(TestUkmRecorder);
 };
 

components/ukm/ukm_recorder_impl.h

@@ -82,6 +82,7 @@ class UkmRecorderImpl : public UkmRecorder {
   }
 
   // UkmRecorder:
+  void AddEntry(mojom::UkmEntryPtr entry) override;
   void UpdateSourceURL(SourceId source_id, const GURL& url) override;
   void UpdateAppURL(SourceId source_id, const GURL& url) override;
   void RecordNavigation(
@@ -126,8 +127,6 @@ class UkmRecorderImpl : public UkmRecorder {
 
   void RecordSource(std::unique_ptr<UkmSource> source);
 
-  void AddEntry(mojom::UkmEntryPtr entry) override;
-
   // Load sampling configurations from field-trial information.
   void LoadExperimentSamplingInfo();