From 12f9f8ee979459b0f1d0cf5ed2fd89bd9a7a41b7 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 16 Nov 2015 15:17:30 -0500 Subject: [PATCH] Bulk Reports: Fix an infinite loop in ChildTraceMessageFilter::OnSetUMACallback. Also added some unit testing for this file. BUG=555690 Review URL: https://codereview.chromium.org/1447643002 Cr-Commit-Position: refs/heads/master@{#359736} (cherry picked from commit 5ee44554d36595c779c59adef061dce09fe6d2a6) Review URL: https://codereview.chromium.org/1448093002 . Cr-Commit-Position: refs/branch-heads/2564@{#9} Cr-Branched-From: 1283eca15bd9f772387f75241576cde7bdec7f54-refs/heads/master@{#359700} --- components/components_tests.gyp | 4 + .../tracing/child_trace_message_filter.cc | 11 ++- .../tracing/child_trace_message_filter.h | 4 + .../child_trace_message_filter_unittest.cc | 87 +++++++++++++++++++ 4 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 components/tracing/child_trace_message_filter_unittest.cc diff --git a/components/components_tests.gyp b/components/components_tests.gyp index ebc91c923d137..bbea001162d8f 100644 --- a/components/components_tests.gyp +++ b/components/components_tests.gyp @@ -97,6 +97,9 @@ 'certificate_transparency_unittest_sources': [ 'certificate_transparency/log_proof_fetcher_unittest.cc', ], + 'child_trace_message_filter_unittest_sources': [ + 'tracing/child_trace_message_filter_unittest.cc', + ], 'cloud_devices_unittest_sources': [ 'cloud_devices/common/cloud_devices_urls_unittest.cc', 'cloud_devices/common/printer_description_unittest.cc', @@ -859,6 +862,7 @@ '<@(bubble_unittest_sources)', '<@(captive_portal_unittest_sources)', '<@(certificate_reporting_unittest_sources)', + '<@(child_trace_message_filter_unittest_sources)', '<@(cloud_devices_unittest_sources)', '<@(component_updater_unittest_sources)', '<@(compression_unittest_sources)', diff --git a/components/tracing/child_trace_message_filter.cc b/components/tracing/child_trace_message_filter.cc index 5d087347b176d..0694a174887b7 100644 --- a/components/tracing/child_trace_message_filter.cc +++ b/components/tracing/child_trace_message_filter.cc @@ -34,6 +34,10 @@ void ChildTraceMessageFilter::OnFilterAdded(IPC::Sender* sender) { this); } +void ChildTraceMessageFilter::SetSenderForTesting(IPC::Sender* sender) { + sender_ = sender; +} + void ChildTraceMessageFilter::OnFilterRemoved() { ChildMemoryDumpManagerDelegateImpl::GetInstance()->SetChildTraceMessageFilter( nullptr); @@ -306,10 +310,6 @@ void ChildTraceMessageFilter::OnSetUMACallback( base::HistogramBase::Sample max; base::HistogramBase::Count count; sample_iterator->Get(&min, &max, &count); - if (count == 0) { - sample_iterator->Next(); - continue; - } if (min >= histogram_lower_value && max <= histogram_upper_value) { ipc_task_runner_->PostTask( @@ -322,7 +322,10 @@ void ChildTraceMessageFilter::OnSetUMACallback( base::Bind( &ChildTraceMessageFilter::SendAbortBackgroundTracingMessage, this)); + break; } + + sample_iterator->Next(); } } diff --git a/components/tracing/child_trace_message_filter.h b/components/tracing/child_trace_message_filter.h index 9f7755ca70d26..47999762f944b 100644 --- a/components/tracing/child_trace_message_filter.h +++ b/components/tracing/child_trace_message_filter.h @@ -42,6 +42,8 @@ class TRACING_EXPORT ChildTraceMessageFilter : public IPC::MessageFilter { ~ChildTraceMessageFilter() override; private: + friend class ChildTraceMessageFilterTest; + // Message handlers. void OnBeginTracing(const std::string& trace_config_str, base::TimeTicks browser_time, @@ -84,6 +86,8 @@ class TRACING_EXPORT ChildTraceMessageFilter : public IPC::MessageFilter { void OnProcessMemoryDumpDone(uint64 dump_guid, bool success); + void SetSenderForTesting(IPC::Sender* sender); + IPC::Sender* sender_; base::SingleThreadTaskRunner* ipc_task_runner_; diff --git a/components/tracing/child_trace_message_filter_unittest.cc b/components/tracing/child_trace_message_filter_unittest.cc new file mode 100644 index 0000000000000..75d1ad811a59a --- /dev/null +++ b/components/tracing/child_trace_message_filter_unittest.cc @@ -0,0 +1,87 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "components/tracing/child_trace_message_filter.h" + +#include "base/memory/ref_counted.h" +#include "base/memory/scoped_ptr.h" +#include "base/message_loop/message_loop.h" +#include "components/tracing/tracing_messages.h" +#include "ipc/ipc_message.h" +#include "ipc/ipc_sender.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace tracing { + +class FakeSender : public IPC::Sender { + public: + FakeSender() {} + + ~FakeSender() override {} + + bool Send(IPC::Message* msg) override { + last_message_.reset(msg); + return true; + } + + scoped_ptr last_message_; +}; + +class ChildTraceMessageFilterTest : public testing::Test { + public: + ChildTraceMessageFilterTest() { + message_filter_ = + new tracing::ChildTraceMessageFilter(message_loop_.task_runner().get()); + message_filter_->SetSenderForTesting(&fake_sender_); + } + + void OnSetUMACallback(const std::string& histogram, + int low, + int high, + bool repeat) { + fake_sender_.last_message_.reset(); + message_filter_->OnSetUMACallback(histogram, low, high, repeat); + } + + base::MessageLoop message_loop_; + FakeSender fake_sender_; + scoped_refptr message_filter_; +}; + +TEST_F(ChildTraceMessageFilterTest, TestHistogramDoesNotTrigger) { + LOCAL_HISTOGRAM_COUNTS("foo1", 10); + + OnSetUMACallback("foo1", 20000, 25000, true); + + message_loop_.RunUntilIdle(); + + EXPECT_FALSE(fake_sender_.last_message_); +} + +TEST_F(ChildTraceMessageFilterTest, TestHistogramTriggers) { + LOCAL_HISTOGRAM_COUNTS("foo2", 2); + + OnSetUMACallback("foo2", 1, 3, true); + + message_loop_.RunUntilIdle(); + + EXPECT_TRUE(fake_sender_.last_message_); + EXPECT_EQ(fake_sender_.last_message_->type(), + TracingHostMsg_TriggerBackgroundTrace::ID); +} + +TEST_F(ChildTraceMessageFilterTest, TestHistogramAborts) { + LOCAL_HISTOGRAM_COUNTS("foo3", 10); + + OnSetUMACallback("foo3", 1, 3, false); + + message_loop_.RunUntilIdle(); + + EXPECT_TRUE(fake_sender_.last_message_); + EXPECT_EQ(fake_sender_.last_message_->type(), + TracingHostMsg_AbortBackgroundTrace::ID); +} + +} // namespace tracing