Last Comment Bug 667887 - Implement nsMappedAttributes::SizeOf()
: Implement nsMappedAttributes::SizeOf()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 668013
Blocks: 663271
  Show dependency treegraph
 
Reported: 2011-06-28 06:35 PDT by Mounir Lamouri (:mounir)
Modified: 2011-07-25 05:58 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.44 KB, patch)
2011-06-28 06:35 PDT, Mounir Lamouri (:mounir)
jst: review+
Details | Diff | Review
Patch v2 (1.50 KB, patch)
2011-06-28 15:53 PDT, Mounir Lamouri (:mounir)
bzbarsky: review-
Details | Diff | Review
Patch v3 (1.54 KB, patch)
2011-07-05 08:57 PDT, Mounir Lamouri (:mounir)
bzbarsky: review+
Details | Diff | Review

Description Mounir Lamouri (:mounir) 2011-06-28 06:35:32 PDT
Created attachment 542444 [details] [diff] [review]
Patch v1
Comment 1 Boris Zbarsky [:bz] 2011-06-28 07:56:17 PDT
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.
Comment 2 Mounir Lamouri (:mounir) 2011-06-28 15:53:28 PDT
Created attachment 542636 [details] [diff] [review]
Patch v2

This patch tries to take into account Boris comment.
Comment 3 Boris Zbarsky [:bz] 2011-06-28 20:00:49 PDT
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.
Comment 4 Mounir Lamouri (:mounir) 2011-07-05 08:57:24 PDT
Created attachment 543969 [details] [diff] [review]
Patch v3
Comment 5 Boris Zbarsky [:bz] 2011-07-12 13:06:24 PDT
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.
Comment 6 Marco Bonardo [::mak] 2011-07-20 06:56:15 PDT
this has been backed out by ehsan due to bustage with all the other changesets in the same push
Comment 7 Marco Bonardo [::mak] 2011-07-25 05:58:05 PDT
http://hg.mozilla.org/mozilla-central/rev/a3b60c8ebad3

Note You need to log in before you can comment on or make changes to this bug.