VM: Fix memory leak during shutdown

The way runtime/platform/hashmap.h:HashMap was implemented so far did not allow
deleting elements while iterating over the map. If one iterated like this

  HashMap::Entry* cursor = map.Start();
  while (cursor != NULL) {
    if (cond) {
      map.Remove(cursor->key, cursor->hash);
    }
    cursor = map.Next(cursor);
  }

Then the iteration `cursor` will skip elements. This is due to the fact that
`HashMap::Remove()` is left-rotating elements in certain cases and
`HashMap::Next()` will unconditionally advance to the next position in the
backing store.

PROBLEM IS: There was existing code which did remove elements while iterating
over a HashMap.

R=fschneider@google.com

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

runtime/bin/hashmap_test.cc

@@ -171,7 +171,7 @@ void TestSet(IntKeyHash hash, int size) {
 }
 
 
-UNIT_TEST_CASE(Set) {
+UNIT_TEST_CASE(HashMap_Basic) {
   TestSet(WordHash, 100);
   TestSet(Hash, 100);
   TestSet(CollisionHash1, 50);
@@ -180,4 +180,47 @@ UNIT_TEST_CASE(Set) {
   TestSet(CollisionHash4, 50);
 }
 
+
+UNIT_TEST_CASE(HashMap_RemoveDuringIteration) {
+  class Utils {
+   public:
+    static bool MatchFun(void* key1, void* key2) { return key1 == key2; }
+    static void* Key(intptr_t i) { return reinterpret_cast<void*>(i); }
+    static void* Value(intptr_t i) { return reinterpret_cast<void*>(i); }
+    static uint32_t HashCode(intptr_t key) { return 1; }
+  };
+
+  HashMap map(Utils::MatchFun, 8);
+
+  // Add 6 (1, 1), ..., (6, 60) entries to the map all with a hashcode of 1
+  // (i.e. have all keys have collinding hashcode).
+  //
+  // This causes the probing position in the hashmap to be 1 and open-addressing
+  // with linear probing will fill in the slots towards the right
+  // (i.e. from 1..6).
+  for (intptr_t i = 1; i <= 6 /* avoid rehash at 7 */; i++) {
+    HashMap::Entry* entry = map.Lookup(Utils::Key(i), Utils::HashCode(i), true);
+    entry->value = Utils::Value(10 * i);
+  }
+
+  // Now we iterate over all elements and delete the current element. Since all
+  // our entries have a colliding hashcode of 1, each deletion will cause all
+  // following elements to be left-rotated by 1.
+  intptr_t i = 0;
+  HashMap::Entry* current = map.Start();
+  while (current != NULL) {
+    i++;
+    EXPECT_EQ(Utils::Key(i), current->key);
+    EXPECT_EQ(Utils::Value(10 * i), current->value);
+
+    // Every 2nd element we keep to hit the left-rotation case only sometimes.
+    if (i % 2 == 0) {
+      current = map.Remove(current);
+    } else {
+      current = map.Next(current);
+    }
+  }
+  EXPECT_EQ(6, i);
+}
+
 }  // namespace dart

runtime/bin/socket.cc

@@ -196,7 +196,8 @@ Dart_Handle ListeningSocketRegistry::CreateBindListen(Dart_Handle socket_object,
 }
 
 
-bool ListeningSocketRegistry::CloseOneSafe(OSSocket* os_socket) {
+bool ListeningSocketRegistry::CloseOneSafe(OSSocket* os_socket,
+                                           bool update_hash_maps) {
   ASSERT(!mutex_->TryLock());
   ASSERT(os_socket != NULL);
   ASSERT(os_socket->ref_count > 0);
@@ -204,26 +205,28 @@ bool ListeningSocketRegistry::CloseOneSafe(OSSocket* os_socket) {
   if (os_socket->ref_count > 0) {
     return false;
   }
-  // We free the OS socket by removing it from two datastructures.
-  RemoveByFd(os_socket->socketfd);
+  if (update_hash_maps) {
+    // We free the OS socket by removing it from two datastructures.
+    RemoveByFd(os_socket->socketfd);
 
-  OSSocket* prev = NULL;
-  OSSocket* current = LookupByPort(os_socket->port);
-  while (current != os_socket) {
-    ASSERT(current != NULL);
-    prev = current;
-    current = current->next;
-  }
+    OSSocket* prev = NULL;
+    OSSocket* current = LookupByPort(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.
-    RemoveByPort(os_socket->port);
-  } else if (prev == NULL) {
-    // Remove first element of the list.
-    InsertByPort(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.
+      RemoveByPort(os_socket->port);
+    } else if (prev == NULL) {
+      // Remove first element of the list.
+      InsertByPort(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);
@@ -234,9 +237,10 @@ bool ListeningSocketRegistry::CloseOneSafe(OSSocket* os_socket) {
 
 void ListeningSocketRegistry::CloseAllSafe() {
   MutexLocker ml(mutex_);
-  for (HashMap::Entry* p = sockets_by_fd_.Start(); p != NULL;
-       p = sockets_by_fd_.Next(p)) {
-    CloseOneSafe(reinterpret_cast<OSSocket*>(p->value));
+
+  for (HashMap::Entry* cursor = sockets_by_fd_.Start(); cursor != NULL;
+       cursor = sockets_by_fd_.Next(cursor)) {
+    CloseOneSafe(reinterpret_cast<OSSocket*>(cursor->value), false);
   }
 }
 
@@ -245,7 +249,7 @@ bool ListeningSocketRegistry::CloseSafe(intptr_t socketfd) {
   ASSERT(!mutex_->TryLock());
   OSSocket* os_socket = LookupByFd(socketfd);
   if (os_socket != NULL) {
-    return CloseOneSafe(os_socket);
+    return CloseOneSafe(os_socket, true);
   } else {
     // It should be impossible for the event handler to close something that
     // hasn't been created before.

runtime/bin/socket.h

@@ -469,7 +469,7 @@ class ListeningSocketRegistry {
   void InsertByFd(intptr_t fd, OSSocket* socket);
   void RemoveByFd(intptr_t fd);
 
-  bool CloseOneSafe(OSSocket* os_socket);
+  bool CloseOneSafe(OSSocket* os_socket, bool update_hash_maps);
   void CloseAllSafe();
 
   HashMap sockets_by_port_;

runtime/platform/hashmap.cc

@@ -106,11 +106,27 @@ 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_--;
 }
 
 
+HashMap::Entry* HashMap::Remove(Entry* entry) {
+  Remove(entry->key, entry->hash);
+
+  // A key can only exist once in the map and we just removed `key`. This means
+  // that either a left-rotation has happened (in which case `entry` points
+  // already to the next element (in terms of iteration order)) or alternatively
+  // we can use the normal `Next()` call to move in iteration order.
+  if (entry->key != NULL) {
+    // A left-rotation happened. `entry` points already to the next element in
+    // iteration order.
+    return entry;
+  } else {
+    return Next(entry);
+  }
+}
+
+
 void HashMap::Clear(ClearFun clear) {
   // Mark all entries as empty.
   const Entry* end = map_end();
@@ -118,7 +134,6 @@ void HashMap::Clear(ClearFun clear) {
     if ((clear != NULL) && (p->key != NULL)) {
       clear(p->value);
     }
-    p->value = NULL;
     p->key = NULL;
   }
   occupancy_ = 0;

runtime/platform/hashmap.h

@@ -61,8 +61,28 @@ class HashMap {
   Entry* Lookup(void* key, uint32_t hash, bool insert);
 
   // Removes the entry with matching key.
+  //
+  // WARNING: This method cannot be called while iterating a `HashMap`
+  // otherwise the iteration might step over elements!
   void Remove(void* key, uint32_t hash);
 
+  // Removes the entry and returns the next entry (in iteration order) after
+  // this one.
+  //
+  // Usage instructions for removing elements during iteration:
+  //
+  //    HashMap::Entry* current = map.Start();
+  //    while (current != NULL) {
+  //       ...
+  //      if (condition) {
+  //        current = map.Remove(current);
+  //      } else {
+  //        current = map.Next(current);
+  //      }
+  //    }
+  //
+  Entry* Remove(Entry* entry);
+
   // Empties the hash map (occupancy() == 0), and calls the function 'clear' on
   // each of the values if given.
   void Clear(ClearFun clear = NULL);