Closed Bug 1170934 Opened 9 years ago Closed 9 years ago

Two more PLDHashTable tweaks

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(2 files, 1 obsolete file)

More clean-ups.
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+
(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 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+
(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)
> 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 :/
Attachment #8614538 - Attachment is obsolete: true
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+
https://hg.mozilla.org/mozilla-central/rev/9f6521295d57
https://hg.mozilla.org/mozilla-central/rev/d0e19d47a59b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1452202
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: