Skip to content

GitLab

T
tangled
Commit 1783bb87 authored by Yuki Shiino's avatar Yuki Shiino Committed by Commit Bot
Browse files
Options

Revert "Wait for ukm entry in TryAutoplay" 

This reverts commit ebe80ad0.

Reason for revert: Suspicious to break MSan Tests.
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20MSan%20Tests/11452

Original change's description:
> 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/1204011
> Reviewed-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}

TBR=tommi@chromium.org,holte@chromium.org,jochen@chromium.org,rkuksin@yandex-team.ru

Change-Id: I2c930e1d9ac74bd63ab876f06e494824fd4524d2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 880217
Reviewed-on: https://chromium-review.googlesource.com/1212245Reviewed-by: default avatarYuki Shiino <yukishiino@chromium.org>
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589423}
parent 8677e6fb
Hide whitespace changes
Inline Side-by-side
Showing with 14 additions and 44 deletions
chrome/browser/media/autoplay_metrics_browsertest.cc
View file @ 1783bb87
......@@ -2,7 +2,6 @@
// 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"
......@@ -19,25 +18,15 @@ 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(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());
void TryAutoplay(const content::ToRenderFrameHost& adapter) {
EXPECT_TRUE(ExecuteScriptWithoutUserGesture(adapter.render_frame_host(),
"tryPlayback();"));
run_loop.Run();
}
void NavigateFrameAndWait(content::RenderFrameHost* rfh, const GURL& url) {
......@@ -81,6 +70,9 @@ 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(
......@@ -90,25 +82,25 @@ IN_PROC_BROWSER_TEST_F(AutoplayMetricsBrowserTest, RecordAutoplayAttemptUkm) {
// Navigate main frame, try play.
NavigateFrameAndWait(web_contents()->GetMainFrame(), main_url);
TryAutoplay(test_ukm_recorder, web_contents());
TryAutoplay(web_contents());
// Check that we recorded a UKM event using the main frame URL.
{
auto ukm_entries = test_ukm_recorder.GetEntriesByName(Entry::kEntryName);
ASSERT_EQ(1u, ukm_entries.size());
EXPECT_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(test_ukm_recorder, first_child());
TryAutoplay(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);
ASSERT_EQ(2u, ukm_entries.size());
EXPECT_EQ(2u, ukm_entries.size());
EXPECT_FALSE(
test_ukm_recorder.GetSourceForSourceId(ukm_entries[1]->source_id));
......@@ -130,13 +122,13 @@ IN_PROC_BROWSER_TEST_F(AutoplayMetricsBrowserTest, RecordAutoplayAttemptUkm) {
// Navigate sub sub frame, try play.
NavigateFrameAndWait(second_child(), bar_url);
TryAutoplay(test_ukm_recorder, second_child());
TryAutoplay(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);
ASSERT_EQ(3u, ukm_entries.size());
EXPECT_EQ(3u, ukm_entries.size());
EXPECT_FALSE(
test_ukm_recorder.GetSourceForSourceId(ukm_entries[2]->source_id));
......@@ -158,13 +150,13 @@ IN_PROC_BROWSER_TEST_F(AutoplayMetricsBrowserTest, RecordAutoplayAttemptUkm) {
// Navigate top frame, try play.
NavigateFrameAndWait(web_contents()->GetMainFrame(), foo_url);
TryAutoplay(test_ukm_recorder, web_contents());
TryAutoplay(web_contents());
// Check that we recorded a UKM event using the main frame URL.
{
auto ukm_entries = test_ukm_recorder.GetEntriesByName(Entry::kEntryName);
ASSERT_EQ(4u, ukm_entries.size());
EXPECT_EQ(4u, ukm_entries.size());
test_ukm_recorder.ExpectEntrySourceHasUrl(ukm_entries[3], foo_url);
}
}
......
components/ukm/test_ukm_recorder.cc
View file @ 1783bb87
......@@ -56,14 +56,6 @@ 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;
......@@ -76,12 +68,6 @@ 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
View file @ 1783bb87
......@@ -31,8 +31,6 @@ 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(); }
......@@ -49,10 +47,6 @@ 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;
......@@ -81,9 +75,6 @@ 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
View file @ 1783bb87
......@@ -82,7 +82,6 @@ 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(
......@@ -127,6 +126,8 @@ 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();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment