Closed Bug 1131199 Opened 9 years ago Closed 9 years ago

Consider making PLDHashInitEntry infallible

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(3 files)

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: nobody → continuation
comm-central only defines PLDHashtableOps in 3 places, and none of them define an init entry.
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+
(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+
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee: continuation → n.nethercote
Assignee: n.nethercote → continuation
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.

Attachment

General

Created:
Updated:
Size: