Bug 1415980 - make hash keys movable and not copyable; r=erahm

Everything that goes in a PLDHashtable (and its derivatives, like
nsTHashtable) needs to inherit from PLDHashEntryHdr. But through a lack
of enforcement, copy constructors for these derived classes didn't
explicitly invoke the copy constructor for PLDHashEntryHdr (and the
compiler didn't invoke the copy constructor for us). Instead,
PLDHashTable explicitly copied around the bits that the copy constructor
would have.

The current setup has two problems:

1) Derived classes should be using move construction, not copy
   construction, since anything that's shuffling hash table keys/entries
   around will be using move construction.

2) Derived classes should take responsibility for transferring bits of
   superclass state around, and not rely on something else to handle that.

The second point is not a huge problem for PLDHashTable (PLDHashTable
only has to copy PLDHashEntryHdr's bits in a single place), but future
hash table implementations that might move entries around more
aggressively would have to insert compensation code all over the
place. Additionally, if moving entries is implemented via memcpy (which
is quite common), PLDHashTable copying around bits *again* is
inefficient.

Let's fix all these problems in one go, by:

1) Explicitly declaring the set of constructors that PLDHashEntryHdr
   implements (and does not implement). In particular, the copy
   constructor is deleted, so any derived classes that attempt to make
   themselves copyable will be detected at compile time: the compiler
   will complain that the superclass type is not copyable.

This change on its own will result in many compiler errors, so...

2) Change any derived classes to implement move constructors instead of
   copy constructors. Note that some of these move constructors are,
   strictly speaking, unnecessary, since the relevant classes are moved
   via memcpy in nsTHashtable and its derivatives.

Author: froydnj

accessible/base/NotificationController.h

@@ -367,7 +367,10 @@ class NotificationController final : public EventQueue,
     typedef const T* KeyTypePointer;
 
     explicit nsCOMPtrHashKey(const T* aKey) : mKey(const_cast<T*>(aKey)) {}
-    explicit nsCOMPtrHashKey(const nsPtrHashKey<T> &aToCopy) : mKey(aToCopy.mKey) {}
+    nsCOMPtrHashKey(nsCOMPtrHashKey<T>&& aOther)
+      : PLDHashEntryHdr(std::move(aOther))
+      , mKey(std::move(aOther.mKey))
+    {}
     ~nsCOMPtrHashKey() { }
 
     KeyType GetKey() const { return mKey; }

dom/animation/PseudoElementHashEntry.h

@@ -24,7 +24,7 @@ class PseudoElementHashEntry : public PLDHashEntryHdr
   explicit PseudoElementHashEntry(KeyTypePointer aKey)
     : mElement(aKey->mElement)
     , mPseudoType(aKey->mPseudoType) { }
-  explicit PseudoElementHashEntry(const PseudoElementHashEntry& aCopy)=default;
+  PseudoElementHashEntry(PseudoElementHashEntry&& aOther) = default;
 
   ~PseudoElementHashEntry() = default;
 

dom/base/nsDocument.cpp

@@ -392,7 +392,8 @@ nsIdentifierMapEntry::~nsIdentifierMapEntry()
 {}
 
 nsIdentifierMapEntry::nsIdentifierMapEntry(nsIdentifierMapEntry&& aOther)
-  : mKey(std::move(aOther.mKey))
+  : PLDHashEntryHdr(std::move(aOther))
+  , mKey(std::move(aOther.mKey))
   , mIdContentList(std::move(aOther.mIdContentList))
   , mNameContentList(std::move(aOther.mNameContentList))
   , mChangeCallbacks(std::move(aOther.mChangeCallbacks))

dom/base/nsIdentifierMapEntry.h

@@ -182,8 +182,9 @@ class nsIdentifierMapEntry : public PLDHashEntryHdr
 
     explicit ChangeCallbackEntry(const ChangeCallback* aKey) :
       mKey(*aKey) { }
-    ChangeCallbackEntry(const ChangeCallbackEntry& toCopy) :
-      mKey(toCopy.mKey) { }
+    ChangeCallbackEntry(ChangeCallbackEntry&& aOther) :
+      PLDHashEntryHdr(std::move(aOther)),
+      mKey(std::move(aOther.mKey)) { }
 
     KeyType GetKey() const { return mKey; }
     bool KeyEquals(KeyTypePointer aKey) const {

dom/base/nsNodeInfoManager.h

@@ -148,6 +148,7 @@ class nsNodeInfoManager final
   {
   public:
     explicit NodeInfoInnerKey(KeyTypePointer aKey) : nsPtrHashKey(aKey) {}
+    NodeInfoInnerKey(NodeInfoInnerKey&&) = default;
     ~NodeInfoInnerKey() = default;
     bool KeyEquals(KeyTypePointer aKey) const { return *mKey == *aKey; }
     static PLDHashNumber HashKey(KeyTypePointer aKey) { return aKey->Hash(); }

dom/commandhandler/nsCommandParams.cpp

@@ -336,10 +336,10 @@ nsCommandParams::HashMoveEntry(PLDHashTable* aTable,
                                const PLDHashEntryHdr* aFrom,
                                PLDHashEntryHdr* aTo)
 {
-  const HashEntry* fromEntry = static_cast<const HashEntry*>(aFrom);
+  auto* fromEntry = const_cast<HashEntry*>(static_cast<const HashEntry*>(aFrom));
   HashEntry* toEntry = static_cast<HashEntry*>(aTo);
 
-  new (toEntry) HashEntry(*fromEntry);
+  new (KnownNotNull, toEntry) HashEntry(std::move(*fromEntry));
 
   fromEntry->~HashEntry();
 }

dom/commandhandler/nsCommandParams.h

@@ -86,7 +86,7 @@ class nsCommandParams : public nsICommandParams
       Reset(mEntryType);
     }
 
-    HashEntry(const HashEntry& aRHS)
+    explicit HashEntry(const HashEntry& aRHS)
       : mEntryType(aRHS.mEntryType)
     {
       Reset(mEntryType);

dom/html/HTMLMediaElement.cpp

@@ -3689,15 +3689,10 @@ HTMLMediaElement::MozCaptureStreamUntilEnded(ErrorResult& aRv)
 class MediaElementSetForURI : public nsURIHashKey
 {
 public:
-  explicit MediaElementSetForURI(const nsIURI* aKey)
-    : nsURIHashKey(aKey)
-  {
-  }
-  MediaElementSetForURI(const MediaElementSetForURI& toCopy)
-    : nsURIHashKey(toCopy)
-    , mElements(toCopy.mElements)
-  {
-  }
+  explicit MediaElementSetForURI(const nsIURI* aKey) : nsURIHashKey(aKey) {}
+  MediaElementSetForURI(MediaElementSetForURI&& aOther)
+    : nsURIHashKey(std::move(aOther))
+    , mElements(std::move(aOther.mElements)) {}
   nsTArray<HTMLMediaElement*> mElements;
 };
 

dom/smil/nsSMILCompositor.h

@@ -35,7 +35,8 @@ class nsSMILCompositor : public PLDHashEntryHdr
      mForceCompositing(false)
   { }
   nsSMILCompositor(nsSMILCompositor&& toMove)
-    : mKey(std::move(toMove.mKey)),
+    : PLDHashEntryHdr(std::move(toMove)),
+      mKey(std::move(toMove.mKey)),
       mAnimationFunctions(std::move(toMove.mAnimationFunctions)),
       mForceCompositing(false)
   { }

dom/storage/LocalStorageManager.h

@@ -66,8 +66,10 @@ class LocalStorageManager final : public nsIDOMStorageManager
       , mCache(new LocalStorageCache(aKey))
     {}
 
-    LocalStorageCacheHashKey(const LocalStorageCacheHashKey& aOther)
-      : nsCStringHashKey(aOther)
+    LocalStorageCacheHashKey(LocalStorageCacheHashKey&& aOther)
+      : nsCStringHashKey(std::move(aOther))
+      , mCache(std::move(aOther.mCache))
+      , mCacheRef(std::move(aOther.mCacheRef))
     {
       NS_ERROR("Shouldn't be called");
     }

dom/xslt/xslt/txExecutionState.h

@@ -29,8 +29,10 @@ class txLoadedDocumentEntry : public nsStringHashKey
                                                           mLoadResult(NS_OK)
     {
     }
-    txLoadedDocumentEntry(const txLoadedDocumentEntry& aToCopy)
-        : nsStringHashKey(aToCopy)
+    txLoadedDocumentEntry(txLoadedDocumentEntry&& aOther)
+        : nsStringHashKey(std::move(aOther))
+        , mDocument(std::move(aOther.mDocument))
+        , mLoadResult(std::move(aOther.mLoadResult))
     {
         NS_ERROR("We're horked.");
     }

gfx/thebes/gfxFontFeatures.h

@@ -112,7 +112,10 @@ class gfxFontFeatureValueSet final {
         typedef const FeatureValueHashKey *KeyTypePointer;
 
         explicit FeatureValueHashEntry(KeyTypePointer aKey) { }
-        FeatureValueHashEntry(const FeatureValueHashEntry& toCopy)
+        FeatureValueHashEntry(FeatureValueHashEntry&& other)
+            : PLDHashEntryHdr(std::move(other))
+            , mKey(std::move(other.mKey))
+            , mValues(std::move(other.mValues))
         {
             NS_ERROR("Should not be called");
         }

gfx/thebes/gfxGlyphExtents.h

@@ -94,12 +94,12 @@ class gfxGlyphExtents {
           , y(0.0)
           , width(0.0)
           , height(0.0) {}
-        HashEntry(const HashEntry& toCopy)
-          : nsUint32HashKey(toCopy) {
-            x = toCopy.x;
-            y = toCopy.y;
-            width = toCopy.width;
-            height = toCopy.height;
+        HashEntry(HashEntry&& aOther)
+            : nsUint32HashKey(std::move(aOther))
+            , x(aOther.x)
+            , y(aOther.y)
+            , width(aOther.width)
+            , height(aOther.height) {
         }
 
         float x, y, width, height;

gfx/thebes/gfxUserFontSet.h

@@ -394,7 +394,8 @@ class gfxUserFontSet {
             { }
 
             Entry(Entry&& aOther)
-                : mURI(std::move(aOther.mURI))
+                : PLDHashEntryHdr(std::move(aOther))
+                , mURI(std::move(aOther.mURI))
                 , mPrincipal(std::move(aOther.mPrincipal))
                 , mFontEntry(std::move(aOther.mFontEntry))
                 , mPrivate(std::move(aOther.mPrivate))

layout/painting/RetainedDisplayListHelpers.h

@@ -30,7 +30,7 @@ class DisplayItemHashEntry : public PLDHashEntryHdr
     : mKey(*aKey)
   {
   }
-  explicit DisplayItemHashEntry(const DisplayItemHashEntry& aCopy) = default;
+  DisplayItemHashEntry(DisplayItemHashEntry&&) = default;
 
   ~DisplayItemHashEntry() = default;
 

layout/style/Loader.h

@@ -24,6 +24,7 @@
 #include "mozilla/StyleSheetInlines.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/MemoryReporting.h"
+#include "mozilla/Move.h"
 #include "mozilla/StyleSheet.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/net/ReferrerPolicy.h"
@@ -70,11 +71,11 @@ class URIPrincipalReferrerPolicyAndCORSModeHashKey : public nsURIHashKey
     MOZ_COUNT_CTOR(URIPrincipalReferrerPolicyAndCORSModeHashKey);
   }
 
-  URIPrincipalReferrerPolicyAndCORSModeHashKey(const URIPrincipalReferrerPolicyAndCORSModeHashKey& toCopy)
-    : nsURIHashKey(toCopy),
-      mPrincipal(toCopy.mPrincipal),
-      mCORSMode(toCopy.mCORSMode),
-      mReferrerPolicy(toCopy.mReferrerPolicy)
+  URIPrincipalReferrerPolicyAndCORSModeHashKey(URIPrincipalReferrerPolicyAndCORSModeHashKey&& toMove)
+    : nsURIHashKey(std::move(toMove)),
+      mPrincipal(std::move(toMove.mPrincipal)),
+      mCORSMode(std::move(toMove.mCORSMode)),
+      mReferrerPolicy(std::move(toMove.mReferrerPolicy))
   {
     MOZ_COUNT_CTOR(URIPrincipalReferrerPolicyAndCORSModeHashKey);
   }

modules/libpref/Preferences.cpp

@@ -2052,7 +2052,7 @@ class PrefCallback : public PLDHashEntryHdr
     mCanonical = canonical;
   }
 
-  // Copy constructor needs to be explicit or the linker complains.
+  // This is explicitly not a copy constructor.
   explicit PrefCallback(const PrefCallback*& aCopy)
     : mDomain(aCopy->mDomain)
     , mBranch(aCopy->mBranch)
@@ -2063,6 +2063,9 @@ class PrefCallback : public PLDHashEntryHdr
     MOZ_COUNT_CTOR(PrefCallback);
   }
 
+  PrefCallback(const PrefCallback&) = delete;
+  PrefCallback(PrefCallback&&) = default;
+
   ~PrefCallback() { MOZ_COUNT_DTOR(PrefCallback); }
 
   bool KeyEquals(const PrefCallback* aKey) const

netwerk/base/nsURIHashKey.h

@@ -9,6 +9,7 @@
 #include "nsCOMPtr.h"
 #include "nsIURI.h"
 #include "nsHashKeys.h"
+#include "mozilla/Move.h"
 #include "mozilla/Unused.h"
 
 /**
@@ -22,8 +23,12 @@ class nsURIHashKey : public PLDHashEntryHdr
 
     explicit nsURIHashKey(const nsIURI* aKey) :
         mKey(const_cast<nsIURI*>(aKey)) { MOZ_COUNT_CTOR(nsURIHashKey); }
-    nsURIHashKey(const nsURIHashKey& toCopy) :
-        mKey(toCopy.mKey) { MOZ_COUNT_CTOR(nsURIHashKey); }
+    nsURIHashKey(nsURIHashKey&& toMove)
+        : PLDHashEntryHdr(std::move(toMove))
+        , mKey(std::move(toMove.mKey))
+    {
+        MOZ_COUNT_CTOR(nsURIHashKey);
+    }
     ~nsURIHashKey() { MOZ_COUNT_DTOR(nsURIHashKey); }
 
     nsIURI* GetKey() const { return mKey; }

netwerk/cache/nsCacheEntry.cpp

@@ -502,8 +502,8 @@ nsCacheEntryHashTable::MoveEntry(PLDHashTable * /* table */,
                                  const PLDHashEntryHdr *from,
                                  PLDHashEntryHdr       *to)
 {
-    ((nsCacheEntryHashTableEntry *)to)->cacheEntry =
-        ((nsCacheEntryHashTableEntry *)from)->cacheEntry;
+    new (KnownNotNull, to) nsCacheEntryHashTableEntry(std::move(*((nsCacheEntryHashTableEntry *)from)));
+    // No need to destroy `from`.
 }
 
 

netwerk/cache/nsDiskCacheBinding.cpp

@@ -43,7 +43,8 @@ MoveEntry(PLDHashTable *           /* table */,
           const PLDHashEntryHdr *     src,
           PLDHashEntryHdr       *     dst)
 {
-    ((HashTableEntry *)dst)->mBinding = ((HashTableEntry *)src)->mBinding;
+    new (KnownNotNull, dst) HashTableEntry(std::move(*(HashTableEntry*)src));
+    // No need to delete `src`.
 }
 
 

netwerk/cookie/nsCookieKey.h

@@ -28,10 +28,8 @@ class nsCookieKey : public PLDHashEntryHdr
     , mOriginAttributes(other->mOriginAttributes)
   {}
 
-  nsCookieKey(KeyType other)
-    : mBaseDomain(other.mBaseDomain)
-    , mOriginAttributes(other.mOriginAttributes)
-  {}
+  nsCookieKey(nsCookieKey&& other) = default;
+  nsCookieKey& operator=(nsCookieKey&&) = default;
 
   bool KeyEquals(KeyTypePointer other) const
   {

netwerk/cookie/nsCookieService.cpp

@@ -2958,7 +2958,7 @@ nsCookieService::Read()
 
     nsCookieKey key(baseDomain, attrs);
     CookieDomainTuple* tuple = mReadArray.AppendElement();
-    tuple->key = key;
+    tuple->key = std::move(key);
     tuple->cookie = GetCookieFromRow(stmt, attrs);
   }
 

parser/html/nsHtml5AtomTable.cpp

@@ -11,8 +11,8 @@ nsHtml5AtomEntry::nsHtml5AtomEntry(KeyTypePointer aStr)
 {
 }
 
-nsHtml5AtomEntry::nsHtml5AtomEntry(const nsHtml5AtomEntry& aOther)
-  : nsStringHashKey(aOther)
+nsHtml5AtomEntry::nsHtml5AtomEntry(nsHtml5AtomEntry&& aOther)
+  : nsStringHashKey(std::move(aOther))
   , mAtom(nullptr)
 {
   MOZ_ASSERT_UNREACHABLE("nsHtml5AtomTable is broken; tried to copy an entry");

parser/html/nsHtml5AtomTable.h

@@ -16,7 +16,7 @@ class nsHtml5AtomEntry : public nsStringHashKey
 {
 public:
   explicit nsHtml5AtomEntry(KeyTypePointer aStr);
-  nsHtml5AtomEntry(const nsHtml5AtomEntry& aOther);
+  nsHtml5AtomEntry(nsHtml5AtomEntry&& aOther);
   ~nsHtml5AtomEntry();
   inline nsAtom* GetAtom() { return mAtom; }
 

security/manager/ssl/nsCertOverrideService.h

@@ -81,7 +81,8 @@ class nsCertOverrideEntry final : public PLDHashEntryHdr
     }
 
     nsCertOverrideEntry(nsCertOverrideEntry&& toMove)
-      : mSettings(std::move(toMove.mSettings))
+      : PLDHashEntryHdr(std::move(toMove))
+      , mSettings(std::move(toMove.mSettings))
       , mHostWithPort(std::move(toMove.mHostWithPort))
     {
     }

security/manager/ssl/nsClientAuthRemember.h

@@ -65,7 +65,8 @@ class nsClientAuthRememberEntry final : public PLDHashEntryHdr
     }
 
     nsClientAuthRememberEntry(nsClientAuthRememberEntry&& aToMove)
-      : mSettings(std::move(aToMove.mSettings))
+      : PLDHashEntryHdr(std::move(aToMove))
+      , mSettings(std::move(aToMove.mSettings))
       , mEntryKey(std::move(aToMove.mEntryKey))
     {
     }

toolkit/components/places/History.h

@@ -9,6 +9,7 @@
 
 #include "mozilla/IHistory.h"
 #include "mozilla/MemoryReporting.h"
+#include "mozilla/Move.h"
 #include "mozilla/Mutex.h"
 #include "mozIAsyncHistory.h"
 #include "Database.h"
@@ -209,8 +210,10 @@ class History final : public IHistory
     : nsURIHashKey(aURI)
     {
     }
-    KeyClass(const KeyClass& aOther)
-    : nsURIHashKey(aOther)
+    KeyClass(KeyClass&& aOther)
+      : nsURIHashKey(std::move(aOther))
+      , array(std::move(aOther.array))
+      , mVisited(std::move(aOther.mVisited))
     {
       MOZ_ASSERT_UNREACHABLE("Do not call me!");
     }
@@ -234,7 +237,7 @@ class History final : public IHistory
     explicit RecentURIKey(const nsIURI* aURI) : nsURIHashKey(aURI)
     {
     }
-    RecentURIKey(const RecentURIKey& aOther) : nsURIHashKey(aOther)
+    RecentURIKey(RecentURIKey&& aOther) : nsURIHashKey(std::move(aOther))
     {
       MOZ_ASSERT_UNREACHABLE("Do not call me!");
     }

toolkit/components/places/nsFaviconService.h

@@ -21,6 +21,7 @@
 #include "imgITools.h"
 #include "mozilla/storage.h"
 #include "mozilla/Attributes.h"
+#include "mozilla/Move.h"
 
 #include "FaviconHelpers.h"
 
@@ -37,11 +38,13 @@ class UnassociatedIconHashKey : public nsURIHashKey
 {
 public:
   explicit UnassociatedIconHashKey(const nsIURI* aURI)
-  : nsURIHashKey(aURI)
+    : nsURIHashKey(aURI)
   {
   }
-  UnassociatedIconHashKey(const UnassociatedIconHashKey& aOther)
-  : nsURIHashKey(aOther)
+  UnassociatedIconHashKey(UnassociatedIconHashKey&& aOther)
+    : nsURIHashKey(std::move(aOther))
+    , iconData(std::move(aOther.iconData))
+    , created(std::move(aOther.created))
   {
     MOZ_ASSERT_UNREACHABLE("Do not call me!");
   }

toolkit/components/places/nsNavHistory.h

@@ -532,8 +532,8 @@ class nsNavHistory final : public nsSupportsWeakReference
     : nsURIHashKey(aURI)
     {
     }
-    VisitHashKey(const VisitHashKey& aOther)
-    : nsURIHashKey(aOther)
+    VisitHashKey(VisitHashKey&& aOther)
+      : nsURIHashKey(std::move(aOther))
     {
       MOZ_ASSERT_UNREACHABLE("Do not call me!");
     }