Closed Bug 667887 Opened 8 years ago Closed 8 years ago

Implement nsMappedAttributes::SizeOf()

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
No description provided.
Attachment #542444 - Flags: review?(jst)
This is wrong.  Mapped attributes are shared across elements, so adding them to the size of mAttrsAndChildren is wrong.  You want to add them to the size of the document itself, by going through the HTML stylesheet.
Attachment #542444 - Flags: review?(jst) → review+
Depends on: 668013
No longer depends on: 667883
Attached patch Patch v2 (obsolete) — Splinter Review
This patch tries to take into account Boris comment.
Attachment #542444 - Attachment is obsolete: true
Attachment #542636 - Flags: review?(bzbarsky)
Whiteboard: [needs review]
Comment on attachment 542636 [details] [diff] [review]
Patch v2

So this sets the size to:

 sizeof(nsMappedAttributes) + mAttrCount * (sizeof(void*) + sizeof(InternalAttr))

This looks wrong.  The correct size of an nsMappedAttributes, from |nsMappedAttributes::operator new| is:

  sizeof(nsMappedAttributes) - sizeof(void*) + mBufferSize*sizeof(InternalAttr)

In particular, nsMappedAttributes allocates all its storage for InternalAttr structs inline.  Now your first problem is that mBufferSize is debug-only....

I wonder whether using mAttrCount in practice is ok because we never have long-lived nsMappedAttributes for mAttrCount != mBufferSize.
Attachment #542636 - Flags: review?(bzbarsky) → review-
Attached patch Patch v3Splinter Review
Attachment #542636 - Attachment is obsolete: true
Attachment #543969 - Flags: review?(bzbarsky)
Comment on attachment 543969 [details] [diff] [review]
Patch v3

Can you add an assert that mAttrCount == mBufferSize in SizeOf?  If that ever fails to be true, we want to know.

r=me with that.
Attachment #543969 - Flags: review?(bzbarsky) → review+
Flags: in-testsuite-
Whiteboard: [needs review] → [inbound]
Flags: in-testsuite-
this has been backed out by ehsan due to bustage with all the other changesets in the same push
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/a3b60c8ebad3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.