From b1640eee939813efd80e3ead1cd0e10d5a3d28d5 Mon Sep 17 00:00:00 2001 From: Yunlian Jiang Date: Wed, 27 Aug 2014 16:22:19 -0700 Subject: [PATCH] metrics: fix memory leaks in unittest. This fixes the memory leaks exposed in unittest. BUG=chromium:408309 TEST=The memory leak related to |sender_| is gone. Change-Id: I92970d0560f6ccd1ccd7f5022ece8cc5eba4a674 Reviewed-on: https://chromium-review.googlesource.com/214578 Reviewed-by: Yunlian Jiang Tested-by: Yunlian Jiang Commit-Queue: Yunlian Jiang --- metrics/uploader/curl_sender.h | 2 +- metrics/uploader/sender.h | 1 + metrics/uploader/upload_service.h | 2 +- metrics/uploader/upload_service_test.cc | 42 +++++++++++++------------ 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/metrics/uploader/curl_sender.h b/metrics/uploader/curl_sender.h index fc5b0f481..b7e958524 100644 --- a/metrics/uploader/curl_sender.h +++ b/metrics/uploader/curl_sender.h @@ -14,7 +14,7 @@ class CurlSender : public Sender { public: explicit CurlSender(std::string server_url); - + virtual ~CurlSender() {} // Sends |content| whose SHA1 hash is |hash| to server_url with a synchronous // POST request to server_url. bool Send(const std::string& content, const std::string& hash) override; diff --git a/metrics/uploader/sender.h b/metrics/uploader/sender.h index 70d29cb7e..521183407 100644 --- a/metrics/uploader/sender.h +++ b/metrics/uploader/sender.h @@ -10,6 +10,7 @@ // Abstract class for a Sender that uploads a metrics message. class Sender { public: + virtual ~Sender() {} // Sends a message |content| with its sha1 hash |hash| virtual bool Send(const std::string& content, const std::string& hash) = 0; }; diff --git a/metrics/uploader/upload_service.h b/metrics/uploader/upload_service.h index ed5ab8526..80b1bf371 100644 --- a/metrics/uploader/upload_service.h +++ b/metrics/uploader/upload_service.h @@ -128,7 +128,7 @@ class UploadService : public base::HistogramFlattener { SystemProfileSetter* system_profile_setter_; base::HistogramSnapshotManager histogram_snapshot_manager_; - Sender* sender_; + scoped_ptr sender_; int failed_upload_count_; scoped_ptr current_log_; scoped_ptr staged_log_; diff --git a/metrics/uploader/upload_service_test.cc b/metrics/uploader/upload_service_test.cc index 9e5bd8d17..7a82624fa 100644 --- a/metrics/uploader/upload_service_test.cc +++ b/metrics/uploader/upload_service_test.cc @@ -23,7 +23,8 @@ class UploadServiceTest : public testing::Test { protected: UploadServiceTest() : upload_service_(), exit_manager_(new base::AtExitManager()) { - upload_service_.sender_ = &sender_; + sender_ = new SenderMock; + upload_service_.sender_.reset(sender_); upload_service_.system_profile_setter_ = new MockSystemProfileSetter(); upload_service_.Init(); } @@ -32,7 +33,7 @@ class UploadServiceTest : public testing::Test { CHECK(dir_.CreateUniqueTempDir()); upload_service_.GatherHistograms(); upload_service_.Reset(); - sender_.Reset(); + sender_->Reset(); cache_.is_testing_ = true; chromeos_metrics::PersistentInteger::SetTestingMode(true); @@ -45,7 +46,7 @@ class UploadServiceTest : public testing::Test { } base::ScopedTempDir dir_; - SenderMock sender_; + SenderMock *sender_; SystemProfileCache cache_; UploadService upload_service_; @@ -90,20 +91,20 @@ TEST_F(UploadServiceTest, UnknownCrashIgnored) { } TEST_F(UploadServiceTest, FailedSendAreRetried) { - sender_.set_should_succeed(false); + sender_->set_should_succeed(false); upload_service_.AddSample(*Crash("user")); upload_service_.UploadEvent(); - EXPECT_EQ(1, sender_.send_call_count()); - std::string sent_string = sender_.last_message(); + EXPECT_EQ(1, sender_->send_call_count()); + std::string sent_string = sender_->last_message(); upload_service_.UploadEvent(); - EXPECT_EQ(2, sender_.send_call_count()); - EXPECT_EQ(sent_string, sender_.last_message()); + EXPECT_EQ(2, sender_->send_call_count()); + EXPECT_EQ(sent_string, sender_->last_message()); } TEST_F(UploadServiceTest, DiscardLogsAfterTooManyFailedUpload) { - sender_.set_should_succeed(false); + sender_->set_should_succeed(false); upload_service_.AddSample(*Crash("user")); for (int i = 0; i < UploadService::kMaxFailedUpload; i++) { @@ -118,7 +119,7 @@ TEST_F(UploadServiceTest, DiscardLogsAfterTooManyFailedUpload) { TEST_F(UploadServiceTest, EmptyLogsAreNotSent) { upload_service_.UploadEvent(); EXPECT_FALSE(upload_service_.current_log_); - EXPECT_EQ(0, sender_.send_call_count()); + EXPECT_EQ(0, sender_->send_call_count()); } TEST_F(UploadServiceTest, LogEmptyByDefault) { @@ -133,12 +134,12 @@ TEST_F(UploadServiceTest, CanSendMultipleTimes) { upload_service_.AddSample(*Crash("user")); upload_service_.UploadEvent(); - std::string first_message = sender_.last_message(); + std::string first_message = sender_->last_message(); upload_service_.AddSample(*Crash("kernel")); upload_service_.UploadEvent(); - EXPECT_NE(first_message, sender_.last_message()); + EXPECT_NE(first_message, sender_->last_message()); } TEST_F(UploadServiceTest, LogEmptyAfterUpload) { @@ -197,16 +198,17 @@ TEST_F(UploadServiceTest, ValuesInConfigFileAreSent) { upload_service_.AddSample(*histogram.get()); upload_service_.UploadEvent(); - EXPECT_EQ(1, sender_.send_call_count()); - EXPECT_TRUE(sender_.is_good_proto()); - EXPECT_EQ(1, sender_.last_message_proto().histogram_event().size()); + EXPECT_EQ(1, sender_->send_call_count()); + EXPECT_TRUE(sender_->is_good_proto()); + EXPECT_EQ(1, sender_->last_message_proto().histogram_event().size()); - EXPECT_EQ(name, sender_.last_message_proto().system_profile().os().name()); + EXPECT_EQ(name, sender_->last_message_proto().system_profile().os().name()); EXPECT_EQ(metrics::SystemProfileProto::CHANNEL_BETA, - sender_.last_message_proto().system_profile().channel()); - EXPECT_NE(0, sender_.last_message_proto().client_id()); - EXPECT_NE(0, sender_.last_message_proto().system_profile().build_timestamp()); - EXPECT_NE(0, sender_.last_message_proto().session_id()); + sender_->last_message_proto().system_profile().channel()); + EXPECT_NE(0, sender_->last_message_proto().client_id()); + EXPECT_NE(0, + sender_->last_message_proto().system_profile().build_timestamp()); + EXPECT_NE(0, sender_->last_message_proto().session_id()); } TEST_F(UploadServiceTest, PersistentGUID) {