Closed
Bug 1170934
Opened 9 years ago
Closed 9 years ago
Two more PLDHashTable tweaks
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files, 1 obsolete file)
9.43 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
More clean-ups.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8614537 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8614538 -
Flags: review?(nfroyd)
Comment 3•9 years ago
|
||
Comment on attachment 8614537 [details] [diff] [review] Remove PLDHashTable::{Init,Fini}() Review of attachment 8614537 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/pldhash.cpp @@ +349,5 @@ > const PLDHashTableOps* ops = mOps; > uint32_t entrySize = mEntrySize; > > + this->~PLDHashTable(); > + new (this) PLDHashTable(ops, entrySize, aLength); This is either clever or slightly devious...probably both.
Attachment #8614537 -
Flags: review?(nfroyd) → review+
Comment 4•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3) > ::: xpcom/glue/pldhash.cpp > @@ +349,5 @@ > > const PLDHashTableOps* ops = mOps; > > uint32_t entrySize = mEntrySize; > > > > + this->~PLDHashTable(); > > + new (this) PLDHashTable(ops, entrySize, aLength); > > This is either clever or slightly devious...probably both. Botond, do you know of any standardese that would prohibit this? I've looked and I haven't found any, but I also might not be looking in the right place.
Flags: needinfo?(botond)
Comment 5•9 years ago
|
||
Comment on attachment 8614538 [details] [diff] [review] (part 2) - Fix some PL_DHashTableRawRemove oddities Review of attachment 8614538 [details] [diff] [review]: ----------------------------------------------------------------- I think I feel OK approving the nsRuleNode.cpp change, but I don't feel comfortable approving the nsDocument.cpp change. Please find a DOM person to review that bit. ::: dom/base/nsDocument.cpp @@ +3964,5 @@ > if (!aSubDoc) { > // aSubDoc is nullptr, remove the mapping > > if (mSubDocuments) { > + PL_DHashTableRemove(mSubDocuments, aElement); I don't think this change is necessarily safe...what if there are outstanding entry pointers into the table when this is called, and we wind up resizing the table? ::: layout/style/nsRuleNode.cpp @@ -1556,5 @@ > nsRuleNode(mPresContext, this, aRule, aLevel, aIsImportantRule); > - if (!next) { > - PL_DHashTableRawRemove(ChildrenHash(), entry); > - NS_WARNING("out of memory"); > - return this; We're saying this is safe because nsRuleNode allocation via the PresContext is infallible: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsIPresShell.h#243 ? That was not especially obvious from the patch description. :)
Attachment #8614538 -
Flags: review?(nfroyd) → feedback+
Comment 6•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4) > (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3) > > ::: xpcom/glue/pldhash.cpp > > @@ +349,5 @@ > > > const PLDHashTableOps* ops = mOps; > > > uint32_t entrySize = mEntrySize; > > > > > > + this->~PLDHashTable(); > > > + new (this) PLDHashTable(ops, entrySize, aLength); > > > > This is either clever or slightly devious...probably both. > > Botond, do you know of any standardese that would prohibit this? I've > looked and I haven't found any, but I also might not be looking in the right > place. My reading of [basic.life] p7 is that this is valid (in fact, there's an example at the end of that paragraph that shows implementing an assignment operator using this technique).
Flags: needinfo?(botond)
Assignee | ||
Comment 7•9 years ago
|
||
> I don't think this change is necessarily safe...what if there are outstanding entry pointers > into the table when this is called, and we wind up resizing the table? Good question. I'll investigate further. > ::: layout/style/nsRuleNode.cpp > @@ -1556,5 @@ > > nsRuleNode(mPresContext, this, aRule, aLevel, aIsImportantRule); > > - if (!next) { > > - PL_DHashTableRawRemove(ChildrenHash(), entry); > > - NS_WARNING("out of memory"); > > - return this; > > We're saying this is safe because nsRuleNode allocation via the PresContext > is infallible: > > http://mxr.mozilla.org/mozilla-central/source/layout/base/nsIPresShell.h#243 > > ? Oh... I incorrectly thought it was placement new. Whoops. I think you're right. There are a bunch of other similar cases in this file; some of them check the return value and some don't. I'll split those changes out into a separate bug and get a layout person to review them. Thank you for the good catches! This wasn't a great patch from me :/
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8614538 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8615043 [details] [diff] [review] (part 2) - Fix the comment for PL_DHashTableRemove() Review of attachment 8615043 [details] [diff] [review]: ----------------------------------------------------------------- This patch now only has the comment fix up. I'm adding r+ because I figure froydnj implicit approved it as part of his f+ for the larger patch, given that he didn't object to this change.
Attachment #8615043 -
Flags: review+
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f6521295d57 https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e19d47a59b
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9f6521295d57 https://hg.mozilla.org/mozilla-central/rev/d0e19d47a59b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•