Fixing memory leak in TimedMap

BUG=402118

Review URL: https://codereview.chromium.org/453203002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288950 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 35 insertions, 72 deletions

diff --git a/components/copresence/handlers/audio/audio_directive_handler_unittest.cc b/components/copresence/handlers/audio/audio_directive_handler_unittest.cc
index 4faa697..6f90e8f 100644
--- a/components/copresence/handlers/audio/audio_directive_handler_unittest.cc
+++ b/components/copresence/handlers/audio/audio_directive_handler_unittest.cc
@@ -77,10 +77,8 @@ class AudioDirectiveHandlerTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(AudioDirectiveHandlerTest);
 };
 
-// TODO(rkc): Find and fix the memory leak here.
-#define MAYBE_Basic DISABLED_Basic
-
-TEST_F(AudioDirectiveHandlerTest, MAYBE_Basic) {
+// TODO(rkc): This test is broken, possibly due to the changes for audible.
+TEST_F(AudioDirectiveHandlerTest, DISABLED_Basic) {
   const base::TimeDelta kSmallTtl = base::TimeDelta::FromMilliseconds(0x1337);
   const base::TimeDelta kLargeTtl = base::TimeDelta::FromSeconds(0x7331);
 
diff --git a/components/copresence/handlers/audio/audio_directive_list_unittest.cc b/components/copresence/handlers/audio/audio_directive_list_unittest.cc
index f746b0d..ba7ff66 100644
--- a/components/copresence/handlers/audio/audio_directive_list_unittest.cc
+++ b/components/copresence/handlers/audio/audio_directive_list_unittest.cc
@@ -36,8 +36,9 @@ class AudioDirectiveListTest : public testing::Test {
   scoped_ptr<AudioDirectiveList> directive_list_;
 };
 
-// TODO(rkc): Find and fix the memory leak here.
+// TODO(rkc): Fix errors in these tests. See crbug/402578.
 #define MAYBE_Basic DISABLED_Basic
+#define MAYBE_OutOfOrderAndMultiple DISABLED_OutOfOrderAndMultiple
 
 TEST_F(AudioDirectiveListTest, MAYBE_Basic) {
   const base::TimeDelta kZeroTtl = base::TimeDelta::FromMilliseconds(0);
@@ -57,9 +58,6 @@ TEST_F(AudioDirectiveListTest, MAYBE_Basic) {
   EXPECT_EQ("op_id3", directive_list_->GetNextReceive()->op_id);
 }
 
-// TODO(rkc): Find out why this is breaking on bots and fix it.
-#define MAYBE_OutOfOrderAndMultiple DISABLED_OutOfOrderAndMultiple
-
 TEST_F(AudioDirectiveListTest, MAYBE_OutOfOrderAndMultiple) {
   const base::TimeDelta kZeroTtl = base::TimeDelta::FromMilliseconds(0);
   const base::TimeDelta kLargeTtl = base::TimeDelta::FromSeconds(0x7331);
diff --git a/components/copresence/mediums/audio/audio_recorder_unittest.cc b/components/copresence/mediums/audio/audio_recorder_unittest.cc
index 900dffb..69c856a 100644
--- a/components/copresence/mediums/audio/audio_recorder_unittest.cc
+++ b/components/copresence/mediums/audio/audio_recorder_unittest.cc
@@ -189,18 +189,14 @@ class AudioRecorderTest : public testing::Test {
   content::TestBrowserThreadBundle thread_bundle_;
 };
 
-#if defined(OS_WIN) || defined(OS_MACOSX)
-// Windows does not let us use non-OS params. The tests need to be rewritten to
-// use the params provided to us by the audio manager rather than setting our
-// own params.
+// TODO(rkc): These tests are broken on all platforms.
+// On Windows and Mac, we cannot use non-OS params. The tests need to be
+// rewritten to use the params provided to us by the audio manager
+// rather than setting our own params.
+// On Linux, there is a memory leak in the audio code during initialization.
 #define MAYBE_BasicRecordAndStop DISABLED_BasicRecordAndStop
 #define MAYBE_OutOfOrderRecordAndStopMultiple DISABLED_OutOfOrderRecordAndStopMultiple
 #define MAYBE_RecordingEndToEnd DISABLED_RecordingEndToEnd
-#else
-#define MAYBE_BasicRecordAndStop BasicRecordAndStop
-#define MAYBE_OutOfOrderRecordAndStopMultiple OutOfOrderRecordAndStopMultiple
-#define MAYBE_RecordingEndToEnd RecordingEndToEnd
-#endif
 
 TEST_F(AudioRecorderTest, MAYBE_BasicRecordAndStop) {
   CreateSimpleRecorder();
diff --git a/components/copresence/rpc/rpc_handler_unittest.cc b/components/copresence/rpc/rpc_handler_unittest.cc
index b7907907..61eb39c 100644
--- a/components/copresence/rpc/rpc_handler_unittest.cc
+++ b/components/copresence/rpc/rpc_handler_unittest.cc
@@ -175,11 +175,7 @@ class RpcHandlerTest : public testing::Test, public CopresenceClientDelegate {
   std::map<std::string, std::vector<Message> > messages_by_subscription_;
 };
 
-// TODO(ckehoe): Renable these after https://codereview.chromium.org/453203002/
-// lands.
-#define MAYBE_Initialize DISABLED_Initialize
-
-TEST_F(RpcHandlerTest, MAYBE_Initialize) {
+TEST_F(RpcHandlerTest, Initialize) {
   SetDeviceId("");
   rpc_handler_.Initialize(RpcHandler::SuccessCallback());
   RegisterDeviceRequest* registration =
@@ -192,11 +188,7 @@ TEST_F(RpcHandlerTest, MAYBE_Initialize) {
 // TODO(ckehoe): Fix this on Windows. See rpc_handler.cc.
 #ifndef OS_WIN
 
-// TODO(ckehoe): Renable these after https://codereview.chromium.org/453203002/
-// lands.
-#define MAYBE_GetDeviceCapabilities DISABLED_GetDeviceCapabilities
-
-TEST_F(RpcHandlerTest, MAYBE_GetDeviceCapabilities) {
+TEST_F(RpcHandlerTest, GetDeviceCapabilities) {
   // Empty request.
   rpc_handler_.SendReportRequest(make_scoped_ptr(new ReportRequest));
   EXPECT_EQ(RpcHandler::kReportRequestRpcName, rpc_name_);
@@ -243,11 +235,7 @@ TEST_F(RpcHandlerTest, MAYBE_GetDeviceCapabilities) {
 }
 #endif
 
-// TODO(ckehoe): Renable these after https://codereview.chromium.org/453203002/
-// lands.
-#define MAYBE_CreateRequestHeader DISABLED_CreateRequestHeader
-
-TEST_F(RpcHandlerTest, MAYBE_CreateRequestHeader) {
+TEST_F(RpcHandlerTest, CreateRequestHeader) {
   SetDeviceId("CreateRequestHeader Device ID");
   rpc_handler_.SendReportRequest(make_scoped_ptr(new ReportRequest),
                                  "CreateRequestHeader App ID",
@@ -261,11 +249,7 @@ TEST_F(RpcHandlerTest, MAYBE_CreateRequestHeader) {
             report->header().registered_device_id());
 }
 
-// TODO(ckehoe): Renable these after https://codereview.chromium.org/453203002/
-// lands.
-#define MAYBE_ReportTokens DISABLED_ReportTokens
-
-TEST_F(RpcHandlerTest, MAYBE_ReportTokens) {
+TEST_F(RpcHandlerTest, ReportTokens) {
   std::vector<FullToken> test_tokens;
   test_tokens.push_back(FullToken("token 1", false));
   test_tokens.push_back(FullToken("token 2", true));
@@ -282,11 +266,7 @@ TEST_F(RpcHandlerTest, MAYBE_ReportTokens) {
   EXPECT_EQ("token 3", tokens_sent.Get(1).token_id());
 }
 
-// TODO(ckehoe): Renable these after https://codereview.chromium.org/453203002/
-// lands.
-#define MAYBE_ReportResponseHandler DISABLED_ReportResponseHandler
-
-TEST_F(RpcHandlerTest, MAYBE_ReportResponseHandler) {
+TEST_F(RpcHandlerTest, ReportResponseHandler) {
   // Fail on HTTP status != 200.
   ReportResponse empty_response;
   empty_response.mutable_header()->mutable_status()->set_code(OK);
diff --git a/components/copresence/timed_map.h b/components/copresence/timed_map.h
index 9097752..4367e59 100644
--- a/components/copresence/timed_map.h
+++ b/components/copresence/timed_map.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_COPRESENCE_TIMED_MAP_
-#define COMPONENTS_COPRESENCE_TIMED_MAP_
+#ifndef COMPONENTS_COPRESENCE_TIMED_MAP_H_
+#define COMPONENTS_COPRESENCE_TIMED_MAP_H_
 
 #include <map>
 #include <queue>
@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/time/default_tick_clock.h"
 #include "base/time/tick_clock.h"
 #include "base/time/time.h"
@@ -51,7 +52,9 @@ class TimedMap {
     return elt == map_.end() ? kEmptyValue : elt->second;
   }
 
-  void set_clock_for_testing(base::TickClock* clock) { clock_ = clock; }
+  void set_clock_for_testing(scoped_ptr<base::TickClock> clock) {
+    clock_ = clock.Pass();
+  }
 
  private:
   void ClearExpiredTokens() {
@@ -80,7 +83,7 @@ class TimedMap {
 
   const ValueType kEmptyValue;
 
-  base::TickClock* clock_;
+  scoped_ptr<base::TickClock> clock_;
   base::RepeatingTimer<TimedMap> timer_;
   const base::TimeDelta lifetime_;
   const size_t max_elements_;
@@ -94,4 +97,4 @@ class TimedMap {
 
 }  // namespace copresence
 
-#endif  // COMPONENTS_COPRESENCE_TIMED_MAP_
+#endif  // COMPONENTS_COPRESENCE_TIMED_MAP_H_
diff --git a/components/copresence/timed_map_unittest.cc b/components/copresence/timed_map_unittest.cc
index 355f304..dcf2266 100644
--- a/components/copresence/timed_map_unittest.cc
+++ b/components/copresence/timed_map_unittest.cc
@@ -22,6 +22,8 @@ struct Value {
 
 class TimedMapTest : public testing::Test {
  public:
+  typedef copresence::TimedMap<int, Value> Map;
+
   TimedMapTest() {}
 
  private:
@@ -31,11 +33,7 @@ class TimedMapTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(TimedMapTest);
 };
 
-// TODO(rkc): Find and fix the memory leak here.
-#define MAYBE_Basic DISABLED_Basic
-
-TEST_F(TimedMapTest, MAYBE_Basic) {
-  typedef copresence::TimedMap<int, Value> Map;
+TEST_F(TimedMapTest, Basic) {
   Map map(base::TimeDelta::FromSeconds(9999), 3);
 
   EXPECT_FALSE(map.HasKey(0));
@@ -60,11 +58,7 @@ TEST_F(TimedMapTest, MAYBE_Basic) {
   EXPECT_EQ(0x7331, map.GetValue(0x1337).value);
 }
 
-// TODO(rkc): Find and fix the memory leak here.
-#define MAYBE_ValueReplacement DISABLED_ValueReplacement
-
-TEST_F(TimedMapTest, MAYBE_ValueReplacement) {
-  typedef copresence::TimedMap<int, Value> Map;
+TEST_F(TimedMapTest, ValueReplacement) {
   Map map(base::TimeDelta::FromSeconds(9999), 10);
 
   map.Add(0x1337, Value(0x7331));
@@ -80,11 +74,7 @@ TEST_F(TimedMapTest, MAYBE_ValueReplacement) {
   EXPECT_EQ(0xd00d, map.GetValue(0x1337).value);
 }
 
-// TODO(rkc): Find and fix the memory leak here.
-#define MAYBE_SizeEvict DISABLED_SizeEvict
-
-TEST_F(TimedMapTest, MAYBE_SizeEvict) {
-  typedef copresence::TimedMap<int, Value> Map;
+TEST_F(TimedMapTest, SizeEvict) {
   Map two_element_map(base::TimeDelta::FromSeconds(9999), 2);
 
   two_element_map.Add(0x1337, Value(0x7331));
@@ -103,15 +93,13 @@ TEST_F(TimedMapTest, MAYBE_SizeEvict) {
   EXPECT_EQ(0, two_element_map.GetValue(0x1337).value);
 }
 
-// TODO(rkc): Find and fix the memory leak here.
-#define MAYBE_TimedEvict DISABLED_TimedEvict
-
-TEST_F(TimedMapTest, MAYBE_TimedEvict) {
+TEST_F(TimedMapTest, TimedEvict) {
   const int kLargeTimeValueSeconds = 9999;
-  base::SimpleTestTickClock clock;
-  typedef copresence::TimedMap<int, Value> Map;
   Map map(base::TimeDelta::FromSeconds(kLargeTimeValueSeconds), 2);
-  map.set_clock_for_testing(&clock);
+
+  // The map takes ownership of the clock, but we retain a pointer.
+  base::SimpleTestTickClock* clock = new base::SimpleTestTickClock;
+  map.set_clock_for_testing(make_scoped_ptr<base::TickClock>(clock));
 
   // Add value at T=0.
   map.Add(0x1337, Value(0x7331));
@@ -119,7 +107,7 @@ TEST_F(TimedMapTest, MAYBE_TimedEvict) {
   EXPECT_EQ(0x7331, map.GetValue(0x1337).value);
 
   // Add value at T=kLargeTimeValueSeconds-1.
-  clock.Advance(base::TimeDelta::FromSeconds(kLargeTimeValueSeconds - 1));
+  clock->Advance(base::TimeDelta::FromSeconds(kLargeTimeValueSeconds - 1));
   map.Add(0xbaad, Value(0xf00d));
 
   // Check values at T=kLargeTimeValueSeconds-1.
@@ -129,14 +117,14 @@ TEST_F(TimedMapTest, MAYBE_TimedEvict) {
   EXPECT_EQ(0x7331, map.GetValue(0x1337).value);
 
   // Check values at T=kLargeTimeValueSeconds.
-  clock.Advance(base::TimeDelta::FromSeconds(1));
+  clock->Advance(base::TimeDelta::FromSeconds(1));
   EXPECT_FALSE(map.HasKey(0x1337));
   EXPECT_EQ(0, map.GetValue(0x1337).value);
   EXPECT_TRUE(map.HasKey(0xbaad));
   EXPECT_EQ(0xf00d, map.GetValue(0xbaad).value);
 
   // Check values at T=2*kLargeTimeValueSeconds
-  clock.Advance(base::TimeDelta::FromSeconds(kLargeTimeValueSeconds));
+  clock->Advance(base::TimeDelta::FromSeconds(kLargeTimeValueSeconds));
   EXPECT_FALSE(map.HasKey(0xbaad));
   EXPECT_EQ(0, map.GetValue(0xbaad).value);
 }