Closed
Bug 1131199
Opened 9 years ago
Closed 9 years ago
Consider making PLDHashInitEntry infallible
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(3 files)
1.18 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
20.77 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
3.26 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Assignee: nobody → continuation
Assignee | ||
Comment 1•9 years ago
|
||
comm-central only defines PLDHashtableOps in 3 places, and none of them define an init entry.
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8562141 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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•9 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.
Comment 6•9 years ago
|
||
(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|.
Updated•9 years ago
|
Attachment #8562142 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 7•9 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
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d347fa7ced40 https://hg.mozilla.org/mozilla-central/rev/37f2c6609e42
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 9•9 years ago
|
||
Attachment #8563025 -
Flags: review?(continuation)
Updated•9 years ago
|
Assignee: continuation → n.nethercote
Updated•9 years ago
|
Assignee: n.nethercote → continuation
Assignee | ||
Comment 10•9 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.
Description
•