Fixing memory leak in TimedMap

components/copresence/timed_map.h

Index: components/copresence/timed_map.h
diff --git a/components/copresence/timed_map.h b/components/copresence/timed_map.h
index 909775201ce7c5e4619cda89ab3dd02824efa8fe..e89afa1620d9205341ba3787d2261b2389d730bd 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>
@@ -31,7 +31,9 @@ class TimedMap {
     timer_.Start(FROM_HERE, lifetime_, this, &TimedMap::ClearExpiredTokens);
   }
 
-  ~TimedMap() {}
+  ~TimedMap() {
+    delete clock_;

[rkc, 2014/08/09 06:41:42]

No reason for this clock to not be a scoped_ptr. Let's do that instead.

[Charlie, 2014/08/09 07:10:40]

Done.

+  }
 
   void Add(const KeyType& key, const ValueType& value) {
     map_[key] = value;
@@ -51,7 +53,12 @@ class TimedMap {
     return elt == map_.end() ? kEmptyValue : elt->second;
   }
 
-  void set_clock_for_testing(base::TickClock* clock) { clock_ = clock; }
+  // TimedMap takes ownership of this clock.
+  void set_clock_for_testing(base::TickClock* clock) {

[rkc, 2014/08/09 06:41:41]

Change the clock to a scoped_ptr, pass in a scoped_ptr<base::TickClock> here,
remove the (then unnecessary) comment.

[Charlie, 2014/08/09 06:48:23]

That's what I wrote first. But then how do you call Advance() on the fake clock
in the test? You can't use this clock anymore, because it's now a TickClock, not
a SimpleTestTickClock. It doesn't have an Advance() method. So you are left with
two hacky options:

1. Retain a bare pointer to the test clock in the test, after you give up
ownership to the TimedMap.
2. Get the clock back from the TimedMap and cast it back to SimpleTestTickClock.

Neither of those seemed better to me than this. But I'm sort of fine with all
three options.

On 2014/08/09 06:41:41, Rahul Chaturvedi wrote:

[rkc, 2014/08/09 06:53:57]

Retain a bare pointer in the test. It is the cleanest solution. I don't want to
not use a scoped_ptr to make test code, at best, cleaner.

[Charlie, 2014/08/09 07:10:40]

Done.

+    DCHECK(clock);
+    delete clock_;
+    clock_ = clock;
+  }
 
  private:
   void ClearExpiredTokens() {
@@ -94,4 +101,4 @@ class TimedMap {
 
 }  // namespace copresence
 
-#endif  // COMPONENTS_COPRESENCE_TIMED_MAP_
+#endif  // COMPONENTS_COPRESENCE_TIMED_MAP_H_