1415980 - fix some issues with how we do hash keys

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nathan Froyd [:froydnj]]

Not super important for PLDHashTable, but for QMEHashTable, this is definitely
useful.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - make hash keys movable and not copyable

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.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - remove compensation code from PLDHashTable

Now that all hash keys always take responsibility for moving their hash
code around when they are moved, we can remove some unnecessary code
from PLDHashTable.

Comment 3

[Eric Rahm [:erahm]]

Comment on attachment 8926994 [details] [diff] [review]
part 1 - make hash keys movable and not copyable

Review of attachment 8926994 [details] [diff] [review]:
-----------------------------------------------------------------

I like this idea! A few things:

1) I think we should use default for anything that's doing the default operation. That makes things that *are not* default clearer, which I'm definitely more interested.
2) There are a bunch of copy ctor assertions that we can remove. I think we should also delete the copy ctor just for clarity.
3) I'm not sure how count ctor works, but either we need to remove it from the move ctors or add it to some.
4) I think we might run into weirdness where we used to memcpy things and then zero them out, but now we `Move` then instead which in some cases isn't going to zero out the moved value, ie Move(char*). That's probably okay sometimes and not okay other times.
5) You removed some constness in member vars, is that unavoidable?
6) I guess it's possible some of these classes really do want to use copy ctors when they're not being used as a key, we might want to check with owners on that.

::: accessible/base/NotificationController.h
@@ +347,5 @@
>      typedef T* KeyType;
>      typedef const T* KeyTypePointer;
>  
>      explicit nsCOMPtrHashKey(const T* aKey) : mKey(const_cast<T*>(aKey)) {}
> +    nsCOMPtrHashKey(nsCOMPtrHashKey<T>&& aOther)

can this be `= default`?

::: dom/base/nsIdentifierMapEntry.h
@@ +82,5 @@
>      mKey(mozilla::Move(aOther.mKey)),
>      mIdContentList(mozilla::Move(aOther.mIdContentList)),
>      mNameContentList(aOther.mNameContentList.forget()),
>      mChangeCallbacks(aOther.mChangeCallbacks.forget()),
>      mImageElement(aOther.mImageElement.forget())

These 3 can now be moves, this could probably `= default`

::: dom/html/HTMLMediaElement.cpp
@@ +3687,5 @@
>  
>  class MediaElementSetForURI : public nsURIHashKey {
>  public:
>    explicit MediaElementSetForURI(const nsIURI* aKey) : nsURIHashKey(aKey) {}
> +  MediaElementSetForURI(MediaElementSetForURI&& aOther)

Again, `= default`?

::: dom/smil/nsSMILCompositor.h
@@ +40,2 @@
>        mAnimationFunctions(mozilla::Move(toMove.mAnimationFunctions)),
>        mForceCompositing(false)

I'm like 99% sure this is a bug, we should almost certainly be copying toMove's value. Also it ignores `mCachedBaseValue` which seems wrong. Not sure if that gets a default value or is junk.

::: dom/storage/LocalStorageManager.h
@@ +65,5 @@
>        : nsCStringHashKey(aKey)
>        , mCache(new LocalStorageCache(aKey))
>      {}
>  
> +    LocalStorageCacheHashKey(LocalStorageCacheHashKey&& aOther)

`= default`?

::: dom/xslt/xslt/txExecutionState.h
@@ +36,2 @@
>      {
>          NS_ERROR("We're horked.");

uhhhhh. uhhhhhhhhhh. we probably need to ni peterv or smaug on this? I'm concerned what's going to happen in release builds if we're moving the refs.

On second thought maybe we can remove it? `nsTStringHashKey` has `ALLOW_MEMMOVE = true` so maybe it's okay.

For something like this it might make sense to explicitly delete the copy ctor as a bit of documentation.

::: gfx/thebes/gfxFontFeatures.h
@@ +111,5 @@
>          typedef const FeatureValueHashKey &KeyType;
>          typedef const FeatureValueHashKey *KeyTypePointer;
>  
>          explicit FeatureValueHashEntry(KeyTypePointer aKey) { }
> +        FeatureValueHashEntry(FeatureValueHashEntry&& other)

`= default`?

@@ +119,2 @@
>          {
>              NS_ERROR("Should not be called");

This needs to go I think. Below we have `ALLOW_MEMMOVE = true` so move construction is okay and copy construction is now verboten.

::: layout/style/Loader.h
@@ +70,5 @@
>        mReferrerPolicy(aReferrerPolicy)
>    {
>      MOZ_COUNT_CTOR(URIPrincipalReferrerPolicyAndCORSModeHashKey);
>    }
> +  URIPrincipalReferrerPolicyAndCORSModeHashKey(URIPrincipalReferrerPolicyAndCORSModeHashKey&& toMove)

`= default`? (depending on below)

@@ +79,2 @@
>    {
>      MOZ_COUNT_CTOR(URIPrincipalReferrerPolicyAndCORSModeHashKey);

Not sure what the rules are, but I don't think we want to count a move right? Or maybe we do because the moved object still hits it's dtor?

::: modules/libpref/Preferences.cpp
@@ +1955,5 @@
>      nsCOMPtr<nsISupports> canonical = do_QueryInterface(aObserver);
>      mCanonical = canonical;
>    }
>  
> +  // This is explicitly not a copy constructor.

Eesh.

@@ +1967,5 @@
>      MOZ_COUNT_CTOR(PrefCallback);
>    }
>  
> +  PrefCallback(const PrefCallback&) = delete;
> +  PrefCallback(PrefCallback&&) = default;

Maybe can't be default if we need to count ctor for move constructors too?

::: netwerk/cookie/nsCookieKey.h
@@ +29,5 @@
>      , mOriginAttributes(other->mOriginAttributes)
>    {}
>  
> +  nsCookieKey(nsCookieKey&& other) = default;
> +  nsCookieKey& operator=(nsCookieKey&&) = default;

Whey do we have to add assignment?

Ah I see below.

::: netwerk/cookie/nsCookieService.cpp
@@ +2841,5 @@
>      Unused << attrs.PopulateFromSuffix(suffix);
>  
>      nsCookieKey key(baseDomain, attrs);
>      CookieDomainTuple* tuple = mReadArray.AppendElement();
> +    tuple->key = Move(key);

Huh, seems like this whole thing could be something like:

> mReadArray.AppendElement({ nsCookieKey(baseDomain, attrs),
>                            GetCookieFromRow(stmt, attrs) });

But maybe not worth thinking about.

::: parser/html/nsHtml5AtomTable.cpp
@@ +10,5 @@
>    , mAtom(new nsAtom(nsAtom::AtomKind::HTML5Atom, *aStr, 0))
>  {
>  }
>  
> +nsHtml5AtomEntry::nsHtml5AtomEntry(nsHtml5AtomEntry&& aOther)

`= default`?

@@ +16,3 @@
>    , mAtom(nullptr)
>  {
>    NS_NOTREACHED("nsHtml5AtomTable is broken and tried to copy an entry");

But move is okay because nsStringHashKey can be moved right?

::: parser/html/nsHtml5AtomTable.h
@@ +15,5 @@
>  class nsHtml5AtomEntry : public nsStringHashKey
>  {
>    public:
>      explicit nsHtml5AtomEntry(KeyTypePointer aStr);
> +    nsHtml5AtomEntry(nsHtml5AtomEntry&& aOther);

Perhaps explicitly delete the copy ctor.

::: security/manager/ssl/nsCertOverrideService.h
@@ +79,5 @@
>      explicit nsCertOverrideEntry(KeyTypePointer aHostWithPortUTF8)
>      {
>      }
>  
>      nsCertOverrideEntry(nsCertOverrideEntry&& toMove)

` = default`?

::: security/manager/ssl/nsClientAuthRemember.h
@@ +62,5 @@
>      explicit nsClientAuthRememberEntry(KeyTypePointer aHostWithCertUTF8)
>      {
>      }
>  
>      nsClientAuthRememberEntry(nsClientAuthRememberEntry&& aToMove)

`= default`?

So this one says it's not memmovable, but it does have a move ctor. I guess these concepts are disjointed...I should probably just go look at what the point is. Oh so this is all kinda busted (in this case we would have called the default copy ctor I think and this move one would have been ignored).

::: toolkit/components/places/History.h
@@ +207,5 @@
>      explicit KeyClass(const nsIURI* aURI)
>      : nsURIHashKey(aURI)
>      {
>      }
> +    KeyClass(KeyClass&& aOther)

`= default`?

@@ +215,2 @@
>      {
>        NS_NOTREACHED("Do not call me!");

We can remove this. Maybe still delete the copy ctor to be clear?

@@ +234,5 @@
>    public:
>      explicit RecentURIKey(const nsIURI* aURI) : nsURIHashKey(aURI)
>      {
>      }
> +    RecentURIKey(RecentURIKey&& aOther) : nsURIHashKey(mozilla::Move(aOther))

`= default`

@@ +239,2 @@
>      {
>        NS_NOTREACHED("Do not call me!");

remove

::: toolkit/components/places/nsFaviconService.h
@@ +42,5 @@
>    {
>    }
> +  UnassociatedIconHashKey(UnassociatedIconHashKey&& aOther)
> +    : nsURIHashKey(mozilla::Move(aOther))
> +    , iconData(mozilla::Move(aOther.iconData))

Interesting tidbit: This would have ended up being copy previously, but now that we've added a move ctor to our strings it's okay b/c `IconData` now has an implicit move ctor.

Anyhow my point is we might want to verify the things we're moving have move ctors.

@@ +48,2 @@
>    {
>      NS_NOTREACHED("Do not call me!");

remove, delete copy ctor.

::: toolkit/components/places/nsNavHistory.h
@@ +579,5 @@
>      explicit VisitHashKey(const nsIURI* aURI)
>      : nsURIHashKey(aURI)
>      {
>      }
> +    VisitHashKey(VisitHashKey&& aOther)

= default

@@ +585,2 @@
>      {
>        NS_NOTREACHED("Do not call me!");

remove, delete copy ctor.

::: xpcom/ds/PLDHashTable.h
@@ +40,5 @@
>  struct PLDHashEntryHdr
>  {
> +  PLDHashEntryHdr() = default;
> +  PLDHashEntryHdr(const PLDHashEntryHdr&) = delete;
> +  PLDHashEntryHdr& operator=(const PLDHashEntryHdr&) = delete;

Do we need to delete the non-const versions, or is this enough?

::: xpcom/ds/nsHashKeys.h
@@ +79,5 @@
>    typedef const nsAString* KeyTypePointer;
>  
>    explicit nsStringHashKey(KeyTypePointer aStr) : mStr(*aStr) {}
> +  nsStringHashKey(const nsStringHashKey&) = delete;
> +  nsStringHashKey(nsStringHashKey&& aToMove)

= default

@@ +108,5 @@
>  
>    enum { ALLOW_MEMMOVE = true };
>  
>  private:
> +  nsString mStr;

Why remove const? Does our string class not have a const move? Should we add it?

@@ +134,5 @@
>      // take it easy just deal HashKey
>    }
> +
> +  nsStringCaseInsensitiveHashKey(const nsStringCaseInsensitiveHashKey&) = delete;
> +  nsStringCaseInsensitiveHashKey(nsStringCaseInsensitiveHashKey&& aToMove)

= default

@@ +180,5 @@
>    typedef const nsACString& KeyType;
>    typedef const nsACString* KeyTypePointer;
>  
>    explicit nsCStringHashKey(const nsACString* aStr) : mStr(*aStr) {}
> +  nsCStringHashKey(nsCStringHashKey&& aOther)

= default

@@ +221,5 @@
>    typedef const uint32_t& KeyType;
>    typedef const uint32_t* KeyTypePointer;
>  
>    explicit nsUint32HashKey(KeyTypePointer aKey) : mValue(*aKey) {}
> +  nsUint32HashKey(nsUint32HashKey&& aOther)

= default

@@ +250,5 @@
>    typedef const uint64_t& KeyType;
>    typedef const uint64_t* KeyTypePointer;
>  
>    explicit nsUint64HashKey(KeyTypePointer aKey) : mValue(*aKey) {}
> +  nsUint64HashKey(nsUint64HashKey&& aOther)

= default

@@ +282,5 @@
>    typedef const float& KeyType;
>    typedef const float* KeyTypePointer;
>  
>    explicit nsFloatHashKey(KeyTypePointer aKey) : mValue(*aKey) {}
> +  nsFloatHashKey(nsFloatHashKey&& aOther)

= default

@@ +317,5 @@
>    explicit nsISupportsHashKey(const nsISupports* aKey)
>      : mSupports(const_cast<nsISupports*>(aKey))
>    {
>    }
> +  nsISupportsHashKey(nsISupportsHashKey&& aOther)

= default

@@ +351,5 @@
>    typedef T* KeyType;
>    typedef const T* KeyTypePointer;
>  
>    explicit nsRefPtrHashKey(const T* aKey) : mKey(const_cast<T*>(aKey)) {}
> +  nsRefPtrHashKey(nsRefPtrHashKey&& aOther)

= default

@@ +394,5 @@
>  class nsClearingPtrHashKey : public nsPtrHashKey<T>
>  {
>  public:
>    explicit nsClearingPtrHashKey(const T* aKey) : nsPtrHashKey<T>(aKey) {}
> +  nsClearingPtrHashKey(nsClearingPtrHashKey&& aToMove)

= default

@@ +445,5 @@
>    typedef const nsID& KeyType;
>    typedef const nsID* KeyTypePointer;
>  
>    explicit nsIDHashKey(const nsID* aInID) : mID(*aInID) {}
> +  nsIDHashKey(nsIDHashKey&& aOther)

= default

@@ +484,5 @@
>    typedef const char* KeyType;
>    typedef const char* KeyTypePointer;
>  
>    explicit nsDepCharHashKey(const char* aKey) : mKey(aKey) {}
> +  nsDepCharHashKey(nsDepCharHashKey&& aOther)

= default

@@ +611,5 @@
>    explicit nsHashableHashKey(const nsIHashable* aKey)
>      : mKey(const_cast<nsIHashable*>(aKey))
>    {
>    }
> +  nsHashableHashKey(nsHashableHashKey&& aOther)

= default

@@ +672,5 @@
>    explicit nsGenericHashKey(KeyTypePointer aKey) : mKey(*aKey) {}
> +  nsGenericHashKey(const nsGenericHashKey&) = delete;
> +  nsGenericHashKey(nsGenericHashKey&& aOther)
> +    : PLDHashEntryHdr(mozilla::Move(aOther))
> +    , mKey(mozilla::Move(aOther.mKey)) {}

I wonder if there's a way to enforce that `T` has a move ctor? I know currently we just assume memmove and zeroing out works, but if we stop zeroing out that becomes an issue.

::: xpcom/ds/nsObserverList.h
@@ +56,5 @@
>    {
>      MOZ_COUNT_CTOR(nsObserverList);
>    }
>  
> +  nsObserverList(nsObserverList&& aOther)

= default

::: xpcom/ds/nsPointerHashKeys.h
@@ +26,5 @@
>    typedef T* KeyType;
>    typedef const T* KeyTypePointer;
>  
>    explicit nsPtrHashKey(const T* aKey) : mKey(const_cast<T*>(aKey)) {}
> +  nsPtrHashKey(nsPtrHashKey<T>&& aToMove)

= default

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #3)
> 1) I think we should use default for anything that's doing the default
> operation. That makes things that *are not* default clearer, which I'm
> definitely more interested.

I think that is a reasonable position to take, but I think mixing up "convert things to move ctors" and "change ctors to use `= default` where appropriate" is a bad idea.

> 2) There are a bunch of copy ctor assertions that we can remove. I think we
> should also delete the copy ctor just for clarity.

Do you mean deleting the (now) move ctors that contain these assertions?  We could do that, but the assertions are there to ensure that the hash table those things are contained in uses memcpy to move elements around and not C++ constructors.  Therefore, I think it's reasonable to have them.  (It would be nice to change things so people didn't have to define them if they didn't want to--the way nsTHashtable is written requires you to define the move ctor even if ALLOW_MEMMOVE is true--but fixing that is outside the scope of this bug.)

> 3) I'm not sure how count ctor works, but either we need to remove it from
> the move ctors or add it to some.

This is reasonable, I think...but if the copy ctors didn't have them before, it clearly wasn't a problem...

> 4) I think we might run into weirdness where we used to memcpy things and
> then zero them out, but now we `Move` then instead which in some cases isn't
> going to zero out the moved value, ie Move(char*). That's probably okay
> sometimes and not okay other times.

Do you have specific examples of this happening in the patch?

> 5) You removed some constness in member vars, is that unavoidable?

Yes.  (I mean, I guess you could use `const_cast`, but that's not very nice.)

> 6) I guess it's possible some of these classes really do want to use copy
> ctors when they're not being used as a key, we might want to check with
> owners on that.

This seems really unlikely, and since the compiler doesn't automatically generate a copy ctor for these classes (as they have an explicitly-defined move ctor), the compiler would complain if we ever attempted to invoke the copy ctor.  So I think we're clear on that.

> ::: dom/smil/nsSMILCompositor.h
> @@ +40,2 @@
> >        mAnimationFunctions(mozilla::Move(toMove.mAnimationFunctions)),
> >        mForceCompositing(false)
> 
> I'm like 99% sure this is a bug, we should almost certainly be copying
> toMove's value. Also it ignores `mCachedBaseValue` which seems wrong. Not
> sure if that gets a default value or is junk.

mCachedBaseValue would just get a null nsSMILValue, since it would be default-constructed.

> @@ +79,2 @@
> >    {
> >      MOZ_COUNT_CTOR(URIPrincipalReferrerPolicyAndCORSModeHashKey);
> 
> Not sure what the rules are, but I don't think we want to count a move
> right? Or maybe we do because the moved object still hits it's dtor?

The latter is correct.

> @@ +1967,5 @@
> >      MOZ_COUNT_CTOR(PrefCallback);
> >    }
> >  
> > +  PrefCallback(const PrefCallback&) = delete;
> > +  PrefCallback(PrefCallback&&) = default;
> 
> Maybe can't be default if we need to count ctor for move constructors too?

Reasonable point, I can change (though I don't think this caused any problems on try, sooo...)

> ::: netwerk/cookie/nsCookieService.cpp
> @@ +2841,5 @@
> >      Unused << attrs.PopulateFromSuffix(suffix);
> >  
> >      nsCookieKey key(baseDomain, attrs);
> >      CookieDomainTuple* tuple = mReadArray.AppendElement();
> > +    tuple->key = Move(key);
> 
> Huh, seems like this whole thing could be something like:
> 
> > mReadArray.AppendElement({ nsCookieKey(baseDomain, attrs),
> >                            GetCookieFromRow(stmt, attrs) });
> 
> But maybe not worth thinking about.

This would be worth doing in a followup, perhaps.

> ::: parser/html/nsHtml5AtomTable.h
> @@ +15,5 @@
> >  class nsHtml5AtomEntry : public nsStringHashKey
> >  {
> >    public:
> >      explicit nsHtml5AtomEntry(KeyTypePointer aStr);
> > +    nsHtml5AtomEntry(nsHtml5AtomEntry&& aOther);
> 
> Perhaps explicitly delete the copy ctor.

The compiler does that for us, but I guess we could?

> ::: security/manager/ssl/nsClientAuthRemember.h
> @@ +62,5 @@
> >      explicit nsClientAuthRememberEntry(KeyTypePointer aHostWithCertUTF8)
> >      {
> >      }
> >  
> >      nsClientAuthRememberEntry(nsClientAuthRememberEntry&& aToMove)
> 
> `= default`?
> 
> So this one says it's not memmovable, but it does have a move ctor. I guess
> these concepts are disjointed...I should probably just go look at what the
> point is. Oh so this is all kinda busted (in this case we would have called
> the default copy ctor I think and this move one would have been ignored).

I do not understand this objection.

> ::: toolkit/components/places/nsFaviconService.h
> @@ +42,5 @@
> >    {
> >    }
> > +  UnassociatedIconHashKey(UnassociatedIconHashKey&& aOther)
> > +    : nsURIHashKey(mozilla::Move(aOther))
> > +    , iconData(mozilla::Move(aOther.iconData))
> 
> Interesting tidbit: This would have ended up being copy previously, but now
> that we've added a move ctor to our strings it's okay b/c `IconData` now has
> an implicit move ctor.
> 
> Anyhow my point is we might want to verify the things we're moving have move
> ctors.

C++ deliberately designed things so you can call Move() on objects even if they don't have move ctors; the copy ctors are used as a fallback.  This policy has problems (you can Move() something expecting the efficient thing to happen and it might not), but you also don't have to carefully consider in templated code whether something is actually movable or not: you can always Move() and the compiler will sort things out for you.  In non-templated code, you can use Move() in the same way...and if the thing you're moving *didn't* have a move ctor, that's OK.  If, later, you add a move ctor, then everywhere that tried Move()'ing the object magically starts working: you don't have to carefully audit all your code to try and make sure you're doing the efficient thing everywhere.

I don't think we need to do this verification.

> ::: xpcom/ds/PLDHashTable.h
> @@ +40,5 @@
> >  struct PLDHashEntryHdr
> >  {
> > +  PLDHashEntryHdr() = default;
> > +  PLDHashEntryHdr(const PLDHashEntryHdr&) = delete;
> > +  PLDHashEntryHdr& operator=(const PLDHashEntryHdr&) = delete;
> 
> Do we need to delete the non-const versions, or is this enough?

This is sufficient.  The non-const versions will not be implicitly generated by the compiler, so if somebody wanted to write non-const versions in a subclass...they're on their own.

> @@ +108,5 @@
> >  
> >    enum { ALLOW_MEMMOVE = true };
> >  
> >  private:
> > +  nsString mStr;
> 
> Why remove const? Does our string class not have a const move? Should we add
> it?

Foo(const Foo&&) doesn't make much sense, as you can't modify the object you're moving from (since it's const).

> @@ +672,5 @@
> >    explicit nsGenericHashKey(KeyTypePointer aKey) : mKey(*aKey) {}
> > +  nsGenericHashKey(const nsGenericHashKey&) = delete;
> > +  nsGenericHashKey(nsGenericHashKey&& aOther)
> > +    : PLDHashEntryHdr(mozilla::Move(aOther))
> > +    , mKey(mozilla::Move(aOther.mKey)) {}
> 
> I wonder if there's a way to enforce that `T` has a move ctor? I know
> currently we just assume memmove and zeroing out works, but if we stop
> zeroing out that becomes an issue.

We could static_assert it, but as explained above, I don't think we should.

Comment 5

[Eric Rahm [:erahm]]

Finally coming back to this with fresh eyes. I think I was conflating |ALLOW_MEMMOVE| with |mozilla::Move|, so for 

> 2) There are a bunch of copy ctor assertions that we can remove. I think we
> should also delete the copy ctor just for clarity.

We can just ignore that, although it would be nice to update the assertions to say something like: this class should only ever by memmoved, this move ctor is only defined to make PLDHashTable happy and should never actually be called.

(In reply to Nathan Froyd [:froydnj] (doing reviews while in Austin) from comment #4)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #3)
> > 1) I think we should use default for anything that's doing the default
> > operation. That makes things that *are not* default clearer, which I'm
> > definitely more interested.
> 
> I think that is a reasonable position to take, but I think mixing up
> "convert things to move ctors" and "change ctors to use `= default` where
> appropriate" is a bad idea.
> 

Okay, I agree analyzing the correctness and converting to ` = default` is scope creep for this bug. Can you file a follow up? 

> C++ constructors.  Therefore, I think it's reasonable to have them.  (It
> would be nice to change things so people didn't have to define them if they
> didn't want to--the way nsTHashtable is written requires you to define the
> move ctor even if ALLOW_MEMMOVE is true--but fixing that is outside the
> scope of this bug.)

Yeah being able to delete them would be much nicer, but I agree that's out of scope. I'm not sure it's worth following up, but just adding some clear comments might be nice at some point.

> > 3) I'm not sure how count ctor works, but either we need to remove it from
> > the move ctors or add it to some.
> 
> This is reasonable, I think...but if the copy ctors didn't have them before,
> it clearly wasn't a problem...

I guess that's because we just memmove them. I suppose it could become an issue if anyone does a `Move` directly, but that can be left to a followup.

> > 4) I think we might run into weirdness where we used to memcpy things and
> > then zero them out, but now we `Move` then instead which in some cases isn't
> > going to zero out the moved value, ie Move(char*). That's probably okay
> > sometimes and not okay other times.
> 
> Do you have specific examples of this happening in the patch?

Sorry again I was confusing memmove with Move.

> > 5) You removed some constness in member vars, is that unavoidable?
> 
> Yes.  (I mean, I guess you could use `const_cast`, but that's not very nice.)
> 
> > 6) I guess it's possible some of these classes really do want to use copy
> > ctors when they're not being used as a key, we might want to check with
> > owners on that.
> 
> This seems really unlikely, and since the compiler doesn't automatically
> generate a copy ctor for these classes (as they have an explicitly-defined
> move ctor), the compiler would complain if we ever attempted to invoke the
> copy ctor.  So I think we're clear on that.

Good point.

> > ::: dom/smil/nsSMILCompositor.h
> > @@ +40,2 @@
> > >        mAnimationFunctions(mozilla::Move(toMove.mAnimationFunctions)),
> > >        mForceCompositing(false)
> > 
> > I'm like 99% sure this is a bug, we should almost certainly be copying
> > toMove's value. Also it ignores `mCachedBaseValue` which seems wrong. Not
> > sure if that gets a default value or is junk.
> 
> mCachedBaseValue would just get a null nsSMILValue, since it would be
> default-constructed.

Okay so it just loses it's cached value which may or may not be intended but it's harmless. Regardless, it's using |ALLOW_MEMMOVE| so it's a moot point.

> > @@ +79,2 @@
> > >    {
> > >      MOZ_COUNT_CTOR(URIPrincipalReferrerPolicyAndCORSModeHashKey);
> > 
> > Not sure what the rules are, but I don't think we want to count a move
> > right? Or maybe we do because the moved object still hits it's dtor?
> 
> The latter is correct.
> 
> > @@ +1967,5 @@
> > >      MOZ_COUNT_CTOR(PrefCallback);
> > >    }
> > >  
> > > +  PrefCallback(const PrefCallback&) = delete;
> > > +  PrefCallback(PrefCallback&&) = default;
> > 
> > Maybe can't be default if we need to count ctor for move constructors too?
> 
> Reasonable point, I can change (though I don't think this caused any
> problems on try, sooo...)

Yeah I suppose it's never actually hits the move ctor due to |ALLOW_MEMMOVE|.

> > ::: netwerk/cookie/nsCookieService.cpp
> > @@ +2841,5 @@
> > >      Unused << attrs.PopulateFromSuffix(suffix);
> > >  
> > >      nsCookieKey key(baseDomain, attrs);
> > >      CookieDomainTuple* tuple = mReadArray.AppendElement();
> > > +    tuple->key = Move(key);
> > 
> > Huh, seems like this whole thing could be something like:
> > 
> > > mReadArray.AppendElement({ nsCookieKey(baseDomain, attrs),
> > >                            GetCookieFromRow(stmt, attrs) });
> > 
> > But maybe not worth thinking about.
> 
> This would be worth doing in a followup, perhaps.

Yeah I don't feel to strongly about this. I guess if it's a hot path or something it could be worth looking at.

> > ::: parser/html/nsHtml5AtomTable.h
> > @@ +15,5 @@
> > >  class nsHtml5AtomEntry : public nsStringHashKey
> > >  {
> > >    public:
> > >      explicit nsHtml5AtomEntry(KeyTypePointer aStr);
> > > +    nsHtml5AtomEntry(nsHtml5AtomEntry&& aOther);
> > 
> > Perhaps explicitly delete the copy ctor.
> 
> The compiler does that for us, but I guess we could?

Yeah that's probably fine, I was thinking along the lines of documenting "you should only move this thing" but we're not actually intendeding to use the move ctor anyhow.

> > ::: security/manager/ssl/nsClientAuthRemember.h
> > @@ +62,5 @@
> > >      explicit nsClientAuthRememberEntry(KeyTypePointer aHostWithCertUTF8)
> > >      {
> > >      }
> > >  
> > >      nsClientAuthRememberEntry(nsClientAuthRememberEntry&& aToMove)
> > 
> > `= default`?
> > 
> > So this one says it's not memmovable, but it does have a move ctor. I guess
> > these concepts are disjointed...I should probably just go look at what the
> > point is. Oh so this is all kinda busted (in this case we would have called
> > the default copy ctor I think and this move one would have been ignored).
> 
> I do not understand this objection.

Neither do I :) But it looks like you already took care of this in bug 1406486.

> > ::: toolkit/components/places/nsFaviconService.h
> > @@ +42,5 @@
> > >    {
> > >    }
> > > +  UnassociatedIconHashKey(UnassociatedIconHashKey&& aOther)
> > > +    : nsURIHashKey(mozilla::Move(aOther))
> > > +    , iconData(mozilla::Move(aOther.iconData))
> > 
> > Interesting tidbit: This would have ended up being copy previously, but now
> > that we've added a move ctor to our strings it's okay b/c `IconData` now has
> > an implicit move ctor.
> > 
> > Anyhow my point is we might want to verify the things we're moving have move
> > ctors.
> 
> C++ deliberately designed things so you can call Move() on objects even if
> they don't have move ctors; the copy ctors are used as a fallback.  This
> policy has problems (you can Move() something expecting the efficient thing
> to happen and it might not), but you also don't have to carefully consider
> in templated code whether something is actually movable or not: you can
> always Move() and the compiler will sort things out for you.  In
> non-templated code, you can use Move() in the same way...and if the thing
> you're moving *didn't* have a move ctor, that's OK.  If, later, you add a
> move ctor, then everywhere that tried Move()'ing the object magically starts
> working: you don't have to carefully audit all your code to try and make
> sure you're doing the efficient thing everywhere.
> 
> I don't think we need to do this verification.

Yeah I just think it gives the false impression this is a low cost operation when in fact it is not guaranteed. I guess they were already potentially slow with the copy ctors so we're not making anything worse. And again for this case it's a moot point because we shouldn't be calling the ctor anyhow.

It *might* be worth auditing the |ALLOW_MEMMOVE = false| cases from a perf angle in a follow up but that's clearly out of scope here.

> > ::: xpcom/ds/PLDHashTable.h
> > @@ +40,5 @@
> > >  struct PLDHashEntryHdr
> > >  {
> > > +  PLDHashEntryHdr() = default;
> > > +  PLDHashEntryHdr(const PLDHashEntryHdr&) = delete;
> > > +  PLDHashEntryHdr& operator=(const PLDHashEntryHdr&) = delete;
> > 
> > Do we need to delete the non-const versions, or is this enough?
> 
> This is sufficient.  The non-const versions will not be implicitly generated
> by the compiler, so if somebody wanted to write non-const versions in a
> subclass...they're on their own.
> 
> > @@ +108,5 @@
> > >  
> > >    enum { ALLOW_MEMMOVE = true };
> > >  
> > >  private:
> > > +  nsString mStr;
> > 
> > Why remove const? Does our string class not have a const move? Should we add
> > it?
> 
> Foo(const Foo&&) doesn't make much sense, as you can't modify the object
> you're moving from (since it's const).
> 
> > @@ +672,5 @@
> > >    explicit nsGenericHashKey(KeyTypePointer aKey) : mKey(*aKey) {}
> > > +  nsGenericHashKey(const nsGenericHashKey&) = delete;
> > > +  nsGenericHashKey(nsGenericHashKey&& aOther)
> > > +    : PLDHashEntryHdr(mozilla::Move(aOther))
> > > +    , mKey(mozilla::Move(aOther.mKey)) {}
> > 
> > I wonder if there's a way to enforce that `T` has a move ctor? I know
> > currently we just assume memmove and zeroing out works, but if we stop
> > zeroing out that becomes an issue.
> 
> We could static_assert it, but as explained above, I don't think we should.

My object doesn't really make sense anyhow, I think I missed that we are hitting the dtors [1], not just memsetting [2].

[1] https://searchfox.org/mozilla-central/rev/b1e0ae2573e88391a7ed92407568be77994c9317/xpcom/ds/nsTHashtable.h#401,444
[2] https://searchfox.org/mozilla-central/rev/b1e0ae2573e88391a7ed92407568be77994c9317/xpcom/ds/PLDHashTable.cpp#109,116

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8926994 [details] [diff] [review]
part 1 - make hash keys movable and not copyable

Review of attachment 8926994 [details] [diff] [review]:
-----------------------------------------------------------------

Re-review looks good.

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8926995 [details] [diff] [review]
part 2 - remove compensation code from PLDHashTable

Review of attachment 8926995 [details] [diff] [review]:
-----------------------------------------------------------------

Can you reword the commit message? "remove compensation code" doesn't make much sense to me. Also the messsage seems to imply there's more code that was remove, maybe just state what was removed and why. In this case I guess because we're now calling PLDHashEntry's ctor from the derived classes' ctors we don't need to manually copy the hash.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9035ff3757ac
make hash keys movable and not copyable; r=erahm

Comment 9

[Jorg K (CEST = GMT+2)]

As I wrote over in bug 1415982 comment #3, that gives compile errors:
PLDHashTable.h(52): error C2580: 'PLDHashEntryHdr::PLDHashEntryHdr(PLDHashEntryHdr &&)': multiple versions of a defaulted special member functions are not allowed

Doing a bit of research, this might well be an error in VS2017, this page claims it's fixed at 15.8 (which we don't use):
https://developercommunity.visualstudio.com/content/problem/270293/internal-compiler-error-compiler-file-msc1cpp-line.html

Maybe I'm completely wrong ;-) and TB code is at fault.

Anyone compiling FF with VS2017 15.6.x?

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Jorg K (GMT+2) from comment #9)
> Anyone compiling FF with VS2017 15.6.x?

Firefox automation is.  And so is TB automation, I suspect, since we only allow specific VS2017 versions...

Comment 11

[Jorg K (CEST = GMT+2)]

They switched to clang-cl on July 10th, 2018. I know since I fixed a bunch of things that didn't compile in clang-cl:
https://mail.mozilla.org/pipermail/firefox-dev/2018-July/006629.html

Or have they switched back?

Comment 12

[Nathan Froyd [:froydnj]]

Sigh, I didn't think about try *not* testing things on MSVC.  Apparently we only test MSVC builds on mozilla-central, so now a try push can guarantee that your code will be OK up until it merges to central.  At least the MSVC builds are tier 2, for even more fun.

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f43ac4d5a7
Backed out changeset 9035ff3757ac at request from froydnj on the suspicion that it's going to break MSVC builds when it gets merged to central.

Comment 14

[Masatoshi Kimura [:emk]]

I imported 9035ff3757ac with some unbitrotting. VS 2017 15.8.4 no longer breaks. Could you consider re-landing?

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Masatoshi Kimura [:emk] from comment #14)
> I imported 9035ff3757ac with some unbitrotting. VS 2017 15.8.4 no longer
> breaks. Could you consider re-landing?

This is not the case on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61a9b41175079eab1c2355037a436370e7680870&selectedJob=200261044

Checking the compilation log says:

20:34:18     INFO -   0:03.75 Downloading vs2017_15.8.4.zip
20:34:18     INFO -   0:03.75 attempt 1/5

So I am not sure how your build succeeded and mine did not.

Comment 16

[Nathan Froyd [:froydnj]]

Trying a version without the `const&&` constructor (which is admittedly nonsensical):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cabfb9b241cb2b699e57454de9af9d37aa2df16

Comment 17

[Masatoshi Kimura [:emk]]

My bad. I have no idea why I have missed the build error. Sorry for the confusion.

14:28.28 e:\m\mozilla-unified\obj-x86_64-pc-msvc\dist\include\PLDHashTable.h(57): error C2580: 'PLDHashEntryHdr::PLDHashEntryHdr(PLDHashEntryHdr &&)': 既定値にされた特殊なメンバー関数の複数のバージョンは使用できません
14:28.31 mozmake.EXE[4]: *** [e:/m/mozilla-unified/config/rules.mk:1123: sandboxBroker.obj] Error 2
...
21:28.09 mozmake.EXE: *** [client.mk:150: build] Error 2

Comment 18

[Masatoshi Kimura [:emk]]

Removing dependency because bug 1490654 did not help.

Comment 19

[Nathan Froyd [:froydnj]]

Thank you for checking.

The version without `PLDHashEntryHdr(const PLDHashEntryHdr&&)` seems to work; maybe that was all we needed to do before.

I'll check the patch in without the offending constructor.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d099fce8fa4
make hash keys movable and not copyable; r=erahm

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3d099fce8fa4