Consider making PLDHashInitEntry infallible

RESOLVED FIXED in Firefox 38

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → continuation
(Assignee)

Comment 1

3 years ago
comm-central only defines PLDHashtableOps in 3 places, and none of them define an init entry.
(Assignee)

Comment 2

3 years ago
Created attachment 8562141 [details] [diff] [review]
part 1 - Allocation of CompareCacheHashEntryPtr::entry is infallible.
Attachment #8562141 - Flags: review?(nfroyd)
(Assignee)

Comment 3

3 years ago
Created attachment 8562142 [details] [diff] [review]
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.
Attachment #8562142 - Flags: review?(nfroyd)
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.
Attachment #8562141 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 5

3 years ago
(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.
(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|.
Attachment #8562142 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 7

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/d347fa7ced40
https://hg.mozilla.org/mozilla-central/rev/37f2c6609e42
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Created attachment 8563025 [details] [diff] [review]
, part 3 - Update comments relating to PLDHashTableOp::initEntry
Attachment #8563025 - Flags: review?(continuation)
Assignee: continuation → n.nethercote
Assignee: n.nethercote → continuation
(Assignee)

Comment 10

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

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

Thanks.
Attachment #8563025 - Flags: review?(continuation) → review+
You need to log in before you can comment on or make changes to this bug.