1131199 - Consider making PLDHashInitEntry infallible

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

I've looked at a bunch of these, and they seem to always return true. Making this infallible would simplify the code a little bit.

The only one I've found so far that ever returns false is CompareCacheInitEntry in nsCertTree.cpp, but I think that can only fail if the entry passed in is null, but we'd have already crashed on a null deref in PLDHashTable::Add if that was the case, so I think that can't happen.

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

comm-central only defines PLDHashtableOps in 3 places, and none of them define an init entry.

Comment 2

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 1 - Allocation of CompareCacheHashEntryPtr::entry is infallible.

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 2 - Make PLDHashtInitEntry infallible.

This makes init entry infallible and drops the unused table argument, which lets us simplify PLDHashTable a tiny bit.  It also fixes up an incorrect comment about the behavior of infallible Add failure.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8562141 [details] [diff] [review]
part 1 - Allocation of CompareCacheHashEntryPtr::entry is infallible.

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

Yeah, it's even a placement new, which can't fail even with the standard library ([new.delete.placement]), let alone mozalloc.

::: security/manager/ssl/src/nsCertTree.cpp
@@ +75,5 @@
>  CompareCacheInitEntry(PLDHashTable *table, PLDHashEntryHdr *hdr,
>                       const void *key)
>  {
>    new (hdr) CompareCacheHashEntryPtr();
>    CompareCacheHashEntryPtr *entryPtr = static_cast<CompareCacheHashEntryPtr*>(hdr);

You could make this |CompareCacheHashEntryPtr* entryPtr = new (hdr) ...| while you're here, if you like.

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> Yeah, it's even a placement new, which can't fail even with the standard
> library ([new.delete.placement]), let alone mozalloc.
No, what it is checking is the result of the allocation in the CompartCacheHashEntryPtr ctor:
  entry = new CompareCacheHashEntry;
Which at one point presumably could fail.

> You could make this |CompareCacheHashEntryPtr* entryPtr = new (hdr) ...|
> while you're here, if you like.

Will do.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> > Yeah, it's even a placement new, which can't fail even with the standard
> > library ([new.delete.placement]), let alone mozalloc.
> No, what it is checking is the result of the allocation in the
> CompartCacheHashEntryPtr ctor:
>   entry = new CompareCacheHashEntry;
> Which at one point presumably could fail.

Doh.  Too many things called |entry|.

Comment 7

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Landed without the comment change in pldhash, as that comment was removed with the backout of njn's lazy allocator patch.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d347fa7ced40
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/37f2c6609e42

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d347fa7ced40
https://hg.mozilla.org/mozilla-central/rev/37f2c6609e42

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: , part 3 - Update comments relating to PLDHashTableOp::initEntry

Comment 10

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Comment on attachment 8563025 [details] [diff] [review]
, part 3 - Update comments relating to PLDHashTableOp::initEntry

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

Thanks.

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a11106f92c

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d4a11106f92c