Fixes memory leaks in the eventhandler

These probably only happen when the VM is going down after an unhandled
exception, but I want to fix them anyway.

[email protected]

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

Author: zanderso

runtime/bin/eventhandler.h

@@ -132,6 +132,8 @@ class CircularLinkedList {
  public:
   CircularLinkedList() : head_(NULL) {}
 
+  typedef void (*ClearFun) (void* value);
+
   // Returns true if the list was empty.
   bool Add(T t) {
     Entry* e = new Entry(t);
@@ -151,7 +153,7 @@ class CircularLinkedList {
     }
   }
 
-  void RemoveHead() {
+  void RemoveHead(ClearFun clear = NULL) {
     ASSERT(head_ != NULL);
 
     Entry* e = head_;
@@ -162,6 +164,9 @@ class CircularLinkedList {
       e->next_->prev_ = e->prev_;
       head_ = e->next_;
     }
+    if (clear != NULL) {
+      clear(reinterpret_cast<void*>(e->t));
+    }
     delete e;
   }
 
@@ -195,9 +200,9 @@ class CircularLinkedList {
     }
   }
 
-  void RemoveAll() {
+  void RemoveAll(ClearFun clear = NULL) {
     while (HasHead()) {
-      RemoveHead();
+      RemoveHead(clear);
     }
   }
 
@@ -413,7 +418,9 @@ class DescriptorInfoMultipleMixin : public DI {
       : DI(fd), tokens_map_(&SamePortValue, kTokenCount),
         disable_tokens_(disable_tokens) {}
 
-  virtual ~DescriptorInfoMultipleMixin() {}
+  virtual ~DescriptorInfoMultipleMixin() {
+    RemoveAllPorts();
+  }
 
   virtual bool IsListeningSocket() const { return true; }
 
@@ -497,14 +504,16 @@ class DescriptorInfoMultipleMixin : public DI {
   }
 
   virtual void RemoveAllPorts() {
-    active_readers_.RemoveAll();
     for (HashMap::Entry *entry = tokens_map_.Start();
          entry != NULL;
          entry = tokens_map_.Next(entry)) {
       PortEntry* pentry = reinterpret_cast<PortEntry*>(entry->value);
+      entry->value = NULL;
+      active_readers_.Remove(pentry);
       delete pentry;
     }
     tokens_map_.Clear();
+    active_readers_.RemoveAll(DeletePortEntry);
   }
 
   virtual Dart_Port NextNotifyDartPort(intptr_t events_ready) {
@@ -585,6 +594,11 @@ class DescriptorInfoMultipleMixin : public DI {
   }
 
  private:
+  static void DeletePortEntry(void* data) {
+    PortEntry* entry = reinterpret_cast<PortEntry*>(data);
+    delete entry;
+  }
+
   // The [Dart_Port]s which are not paused (i.e. are interested in read events,
   // i.e. `mask == (1 << kInEvent)`) and we have enough tokens to communicate
   // with them.

runtime/bin/eventhandler_android.cc

@@ -118,7 +118,15 @@ EventHandlerImplementation::EventHandlerImplementation()
 }
 
 
+static void DeleteDescriptorInfo(void* info) {
+  DescriptorInfo* di = reinterpret_cast<DescriptorInfo*>(info);
+  di->Close();
+  delete di;
+}
+
+
 EventHandlerImplementation::~EventHandlerImplementation() {
+  socket_map_.Clear(DeleteDescriptorInfo);
   VOID_TEMP_FAILURE_RETRY(close(epoll_fd_));
   VOID_TEMP_FAILURE_RETRY(close(interrupt_fds_[0]));
   VOID_TEMP_FAILURE_RETRY(close(interrupt_fds_[1]));

runtime/bin/eventhandler_linux.cc

@@ -127,7 +127,15 @@ EventHandlerImplementation::EventHandlerImplementation()
 }
 
 
+static void DeleteDescriptorInfo(void* info) {
+  DescriptorInfo* di = reinterpret_cast<DescriptorInfo*>(info);
+  di->Close();
+  delete di;
+}
+
+
 EventHandlerImplementation::~EventHandlerImplementation() {
+  socket_map_.Clear(DeleteDescriptorInfo);
   VOID_TEMP_FAILURE_RETRY(close(epoll_fd_));
   VOID_TEMP_FAILURE_RETRY(close(timer_fd_));
   VOID_TEMP_FAILURE_RETRY(close(interrupt_fds_[0]));

runtime/bin/eventhandler_macos.cc

@@ -140,7 +140,15 @@ EventHandlerImplementation::EventHandlerImplementation()
 }
 
 
+static void DeleteDescriptorInfo(void* info) {
+  DescriptorInfo* di = reinterpret_cast<DescriptorInfo*>(info);
+  di->Close();
+  delete di;
+}
+
+
 EventHandlerImplementation::~EventHandlerImplementation() {
+  socket_map_.Clear(DeleteDescriptorInfo);
   VOID_TEMP_FAILURE_RETRY(close(kqueue_fd_));
   VOID_TEMP_FAILURE_RETRY(close(interrupt_fds_[0]));
   VOID_TEMP_FAILURE_RETRY(close(interrupt_fds_[1]));

runtime/bin/socket.cc

@@ -127,42 +127,57 @@ Dart_Handle ListeningSocketRegistry::CreateBindListen(Dart_Handle socket_object,
 }
 
 
-bool ListeningSocketRegistry::CloseSafe(intptr_t socketfd) {
+bool ListeningSocketRegistry::CloseOneSafe(OSSocket* os_socket) {
   ASSERT(!mutex_->TryLock());
+  ASSERT(os_socket != NULL);
+  ASSERT(os_socket->ref_count > 0);
+  os_socket->ref_count--;
+  if (os_socket->ref_count > 0) {
+    return false;
+  }
+  // We free the OS socket by removing it from two datastructures.
+  sockets_by_fd_.erase(os_socket->socketfd);
 
-  SocketsIterator it = sockets_by_fd_.find(socketfd);
-  if (it != sockets_by_fd_.end()) {
-    OSSocket *os_socket = it->second;
+  OSSocket *prev = NULL;
+  OSSocket *current = sockets_by_port_[os_socket->port];
+  while (current != os_socket) {
+    ASSERT(current != NULL);
+    prev = current;
+    current = current->next;
+  }
 
-    ASSERT(os_socket->ref_count > 0);
-    os_socket->ref_count--;
-    if (os_socket->ref_count == 0) {
-      // We free the OS socket by removing it from two datastructures.
-      sockets_by_fd_.erase(socketfd);
-
-      OSSocket *prev = NULL;
-      OSSocket *current = sockets_by_port_[os_socket->port];
-      while (current != os_socket) {
-        ASSERT(current != NULL);
-        prev = current;
-        current = current->next;
-      }
+  if ((prev == NULL) && (current->next == NULL)) {
+    // Remove last element from the list.
+    sockets_by_port_.erase(os_socket->port);
+  } else if (prev == NULL) {
+    // Remove first element of the list.
+    sockets_by_port_[os_socket->port] = current->next;
+  } else {
+    // Remove element from the list which is not the first one.
+    prev->next = os_socket->next;
+  }
 
-      if ((prev == NULL) && (current->next == NULL)) {
-        // Remove last element from the list.
-        sockets_by_port_.erase(os_socket->port);
-      } else if (prev == NULL) {
-        // Remove first element of the list.
-        sockets_by_port_[os_socket->port] = current->next;
-      } else {
-        // Remove element from the list which is not the first one.
-        prev->next = os_socket->next;
-      }
+  ASSERT(os_socket->ref_count == 0);
+  delete os_socket;
+  return true;
+}
 
-      delete os_socket;
-      return true;
-    }
-    return false;
+
+void ListeningSocketRegistry::CloseAllSafe() {
+  MutexLocker ml(mutex_);
+  SocketsIterator it = sockets_by_fd_.begin();
+  while (it != sockets_by_fd_.end()) {
+    CloseOneSafe(it->second);
+    it++;
+  }
+}
+
+
+bool ListeningSocketRegistry::CloseSafe(intptr_t socketfd) {
+  ASSERT(!mutex_->TryLock());
+  SocketsIterator it = sockets_by_fd_.find(socketfd);
+  if (it != sockets_by_fd_.end()) {
+    return CloseOneSafe(it->second);
   } else {
     // It should be impossible for the event handler to close something that
     // hasn't been created before.

runtime/bin/socket.h

@@ -402,6 +402,7 @@ class ListeningSocketRegistry {
   ListeningSocketRegistry() : mutex_(new Mutex()) {}
 
   ~ListeningSocketRegistry() {
+    CloseAllSafe();
     delete mutex_;
     mutex_ = NULL;
   }
@@ -437,6 +438,10 @@ class ListeningSocketRegistry {
     return NULL;
   }
 
+  bool CloseOneSafe(OSSocket* os_socket);
+  void CloseAllSafe();
+
+  // TODO(zra): Replace std::map with the HashMap in platform/hashmap.h.
   std::map<intptr_t, OSSocket*> sockets_by_port_;
   std::map<intptr_t, OSSocket*> sockets_by_fd_;
   Mutex *mutex_;

runtime/platform/hashmap.cc

@@ -15,7 +15,7 @@ HashMap::HashMap(MatchFun match, uint32_t initial_capacity) {
 
 
 HashMap::~HashMap() {
-  free(map_);
+  delete[] map_;
 }
 
 
@@ -106,14 +106,19 @@ void HashMap::Remove(void* key, uint32_t hash) {
 
   // Clear the candidate which will not break searching the hash table.
   candidate->key = NULL;
+  candidate->value = NULL;
   occupancy_--;
 }
 
 
-void HashMap::Clear() {
+void HashMap::Clear(ClearFun clear) {
   // Mark all entries as empty.
   const Entry* end = map_end();
   for (Entry* p = map_; p < end; p++) {
+    if ((clear != NULL) && (p->key != NULL)) {
+      clear(p->value);
+    }
+    p->value = NULL;
     p->key = NULL;
   }
   occupancy_ = 0;
@@ -159,14 +164,14 @@ HashMap::Entry* HashMap::Probe(void* key, uint32_t hash) {
 
 void HashMap::Initialize(uint32_t capacity) {
   ASSERT(dart::Utils::IsPowerOfTwo(capacity));
-  map_ = reinterpret_cast<Entry*>(malloc(capacity * sizeof(Entry)));
+  map_ = new Entry[capacity];
   if (map_ == NULL) {
     // TODO(sgjesse): Handle out of memory.
     FATAL("Cannot allocate memory for hashmap");
     return;
   }
   capacity_ = capacity;
-  Clear();
+  occupancy_ = 0;
 }
 
 
@@ -186,7 +191,7 @@ void HashMap::Resize() {
   }
 
   // Delete old map.
-  free(map);
+  delete[] map;
 }
 
 }  // namespace dart

runtime/platform/hashmap.h

@@ -13,6 +13,8 @@ class HashMap {
  public:
   typedef bool (*MatchFun) (void* key1, void* key2);
 
+  typedef void (*ClearFun) (void* value);
+
   static bool SamePointerValue(void* key1, void* key2) {
     return key1 == key2;
   }
@@ -48,6 +50,7 @@ class HashMap {
   // Some clients may not need to use the value slot
   // (e.g. implementers of sets, where the key is the value).
   struct Entry {
+    Entry() : key(NULL), value(NULL), hash(0) {}
     void* key;
     void* value;
     uint32_t hash;  // The full hash value for key.
@@ -63,8 +66,12 @@ class HashMap {
   // Removes the entry with matching key.
   void Remove(void* key, uint32_t hash);
 
-  // Empties the hash map (occupancy() == 0).
-  void Clear();
+  // Empties the hash map (occupancy() == 0), and calls the function 'clear' on
+  // each of the values if given.
+  void Clear(ClearFun clear = NULL);
+
+  // The number of entries stored in the table.
+  intptr_t size() const { return occupancy_; }
 
   // The capacity of the table. The implementation
   // makes sure that occupancy is at most 80% of