Last Comment Bug 667887 - Implement nsMappedAttributes::SizeOf()
: Implement nsMappedAttributes::SizeOf()
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla8
Assigned To: Mounir Lamouri (:mounir)
: Andrew Overholt [:overholt]
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image Mounir Lamouri (:mounir) 2011-06-28 06:35:32 PDT
Created attachment 542444 [details] [diff] [review]
Patch v1
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image 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 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image Mounir Lamouri (:mounir) 2011-07-05 08:57:24 PDT
Created attachment 543969 [details] [diff] [review]
Patch v3
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 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 User image 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 User image Marco Bonardo [::mak] 2011-07-25 05:58:05 PDT

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