Bug 1160436 - Fix PLDHashTable::operator=. r=froydnj.

This fixes the following problems with PLDHashTable::operator=:

- It doesn't handle self-assigments.

- It leaks the memory used by the assigned-to table.

- It doesn't leave the assigned-from table in a safely destructable state.

xpcom/glue/pldhash.cpp

@@ -266,6 +266,41 @@ PL_DHashTableInit(PLDHashTable* aTable, const PLDHashTableOps* aOps,
   aTable->Init(aOps, aEntrySize, aLength);
 }
 
+PLDHashTable& PLDHashTable::operator=(PLDHashTable&& aOther)
+{
+  if (this == &aOther) {
+    return *this;
+  }
+
+  // Destruct |this|.
+  Finish();
+
+  // Move pieces over.
+  mOps = Move(aOther.mOps);
+  mHashShift = Move(aOther.mHashShift);
+  mEntrySize = Move(aOther.mEntrySize);
+  mEntryCount = Move(aOther.mEntryCount);
+  mRemovedCount = Move(aOther.mRemovedCount);
+  mGeneration = Move(aOther.mGeneration);
+  mEntryStore = Move(aOther.mEntryStore);
+#ifdef PL_DHASHMETER
+  mStats = Move(aOther.mStats);
+#endif
+#ifdef DEBUG
+  // Atomic<> doesn't have an |operator=(Atomic<>&&)|.
+  mRecursionLevel = uint32_t(aOther.mRecursionLevel);
+#endif
+
+  // Clear up |aOther| so its destruction will be a no-op.
+  aOther.mOps = nullptr;
+  aOther.mEntryStore = nullptr;
+#ifdef DEBUG
+  aOther.mRecursionLevel = 0;
+#endif
+
+  return *this;
+}
+
 /*
  * Double hashing needs the second hash code to be relatively prime to table
  * size, so we simply make hash2 odd.
@@ -305,7 +340,10 @@ PLDHashTable::EntryIsFree(PLDHashEntryHdr* aEntry)
 MOZ_ALWAYS_INLINE void
 PLDHashTable::Finish()
 {
-  MOZ_ASSERT(IsInitialized());
+  if (!IsInitialized()) {
+    MOZ_ASSERT(!mEntryStore);
+    return;
+  }
 
   INCREMENT_RECURSION_LEVEL(this);
 

xpcom/glue/pldhash.h

@@ -223,32 +223,18 @@ public:
 #endif
   {}
 
-  PLDHashTable(PLDHashTable&& aOther) { *this = mozilla::Move(aOther); }
+  PLDHashTable(PLDHashTable&& aOther)
-
+    : mOps(nullptr)
-  PLDHashTable& operator=(PLDHashTable&& aOther)
+    , mEntryStore(nullptr)
-  {
-    using mozilla::Move;
-
-    mOps = Move(aOther.mOps);
-    mHashShift = Move(aOther.mHashShift);
-    mEntrySize = Move(aOther.mEntrySize);
-    mEntryCount = Move(aOther.mEntryCount);
-    mRemovedCount = Move(aOther.mRemovedCount);
-    mGeneration = Move(aOther.mGeneration);
-    mEntryStore = Move(aOther.mEntryStore);
-
-#ifdef PL_DHASHMETER
-    mStats = Move(aOther.mStats);
-#endif
-
 #ifdef DEBUG
-    // Atomic<> doesn't have an |operator=(Atomic<>&&)|.
+    , mRecursionLevel(0)
-    mRecursionLevel = uint32_t(aOther.mRecursionLevel);
 #endif
-
+  {
-    return *this;
+    *this = mozilla::Move(aOther);
   }
 
+  PLDHashTable& operator=(PLDHashTable&& aOther);
+
   bool IsInitialized() const { return !!mOps; }
 
   // These should be used rarely.

xpcom/tests/TestPLDHash.cpp

@@ -112,21 +112,73 @@ static bool test_pldhash_lazy_storage()
   return true;
 }
 
-// See bug 931062, we skip this test on Android due to OOM.
+// We insert the integers 0.., so this hash function is (a) as simple as
-#ifndef MOZ_WIDGET_ANDROID
-// We insert the integers 0.., so this is has function is (a) as simple as
 // possible, and (b) collision-free.  Both of which are good, because we want
 // this test to be as fast as possible.
 static PLDHashNumber
-hash(PLDHashTable *table, const void *key)
+trivial_hash(PLDHashTable *table, const void *key)
 {
   return (PLDHashNumber)(size_t)key;
 }
 
+static bool test_pldhash_move_semantics()
+{
+  static const PLDHashTableOps ops = {
+    trivial_hash,
+    PL_DHashMatchEntryStub,
+    PL_DHashMoveEntryStub,
+    PL_DHashClearEntryStub,
+    nullptr
+  };
+
+  PLDHashTable t1, t2;
+  PL_DHashTableInit(&t1, &ops, sizeof(PLDHashEntryStub));
+  PL_DHashTableAdd(&t1, (const void*)88);
+  PL_DHashTableInit(&t2, &ops, sizeof(PLDHashEntryStub));
+  PL_DHashTableAdd(&t2, (const void*)99);
+
+  t1 = mozilla::Move(t1);   // self-move
+
+  t1 = mozilla::Move(t2);   // inited overwritten with inited
+
+  PL_DHashTableFinish(&t1);
+  PL_DHashTableFinish(&t2);
+
+  PLDHashTable t3, t4;
+  PL_DHashTableInit(&t3, &ops, sizeof(PLDHashEntryStub));
+  PL_DHashTableAdd(&t3, (const void*)88);
+
+  t3 = mozilla::Move(t4);   // inited overwritten with uninited
+
+  PL_DHashTableFinish(&t3);
+  PL_DHashTableFinish(&t4);
+
+  PLDHashTable t5, t6;
+  PL_DHashTableInit(&t6, &ops, sizeof(PLDHashEntryStub));
+  PL_DHashTableAdd(&t6, (const void*)88);
+
+  t5 = mozilla::Move(t6);   // uninited overwritten with inited
+
+  PL_DHashTableFinish(&t5);
+  PL_DHashTableFinish(&t6);
+
+  PLDHashTable t7;
+  PLDHashTable t8(mozilla::Move(t7));   // new table constructed with uninited
+
+  PLDHashTable t9;
+  PL_DHashTableInit(&t9, &ops, sizeof(PLDHashEntryStub));
+  PL_DHashTableAdd(&t9, (const void*)88);
+  PLDHashTable t10(mozilla::Move(t9));  // new table constructed with inited
+
+  return true;
+}
+
+// See bug 931062, we skip this test on Android due to OOM.
+#ifndef MOZ_WIDGET_ANDROID
 static bool test_pldhash_grow_to_max_capacity()
 {
   static const PLDHashTableOps ops = {
-    hash,
+    trivial_hash,
     PL_DHashMatchEntryStub,
     PL_DHashMoveEntryStub,
     PL_DHashClearEntryStub,
@@ -174,6 +226,7 @@ static const struct Test {
 } tests[] = {
   DECL_TEST(test_pldhash_Init_capacity_ok),
   DECL_TEST(test_pldhash_lazy_storage),
+  DECL_TEST(test_pldhash_move_semantics),
 // See bug 931062, we skip this test on Android due to OOM.
 #ifndef MOZ_WIDGET_ANDROID
   DECL_TEST(test_pldhash_grow_to_max_capacity),