Fixing memory leak in TimedMap

components/copresence/timed_map.h

 // Copyright 2014 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.
 
-#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>
 #include <utility>
 #include <vector>
 
 #include "base/macros.h"
 #include "base/time/default_tick_clock.h"
 #include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 
 namespace copresence {
 
 // TimedMap is a map with the added functionality of clearing any
 // key/value pair after its specified lifetime is over.
 template <typename KeyType, typename ValueType>
 class TimedMap {
  public:
   TimedMap(const base::TimeDelta& lifetime, size_t max_elements)
       : kEmptyValue(ValueType()),
         clock_(new base::DefaultTickClock()),
         lifetime_(lifetime),
         max_elements_(max_elements) {
     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;
     expiry_queue_.push(KeyTimeTuple(key, clock_->NowTicks() + lifetime_));
     while (map_.size() > max_elements_)
       ClearOldestToken();
   }
 
   bool HasKey(const KeyType& key) {
     ClearExpiredTokens();
     return map_.find(key) != map_.end();
   }
 
   const ValueType& GetValue(const KeyType& key) {
     ClearExpiredTokens();
     typename std::map<KeyType, ValueType>::const_iterator elt = map_.find(key);
     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() {
     while (!expiry_queue_.empty() &&
            expiry_queue_.top().second <= clock_->NowTicks())
       ClearOldestToken();
   }
 
   void ClearOldestToken() {
     map_.erase(expiry_queue_.top().first);
@@ skipping 22 matching lines @@
   std::map<KeyType, ValueType> map_;
   // Priority queue with our element keys ordered by the earliest expiring keys
   // first.
   ExpiryQueue expiry_queue_;
 
   DISALLOW_COPY_AND_ASSIGN(TimedMap);
 };
 
 }  // namespace copresence
 
-#endif  // COMPONENTS_COPRESENCE_TIMED_MAP_
+#endif  // COMPONENTS_COPRESENCE_TIMED_MAP_H_