Closed Bug 678820 Opened 8 years ago Closed 8 years ago

about:memory - nsMappedAttributes::SizeOf - "ASSERTION: mBufferSize and mAttrCount are expected to be the same"

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: jruderman, Assigned: mounir)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

Attached file testcase
1. Load the testcase.
2. Open about:memory.

###!!! ASSERTION: mBufferSize and mAttrCount are expected to be the same.: 'mAttrCount == mBufferSize', file content/base/src/nsMappedAttributes.cpp, line 285
Attached file stack trace
So in this case mAttrCount == 1 but mBufferSize == 2.

This happens because nsAttrAndChildArray::GetModifiableMapped is called with aWillAddAttr set to true, because nsAttrAndChildArray::SetAndTakeMappedAttr passes PR_TRUE for it.  But nsMappedAttributes::SetAndTakeAttr will reuse an existing attr name/value pair...  So we end up allocating an nsMappedAttributes which is one attr name/value too big in this case.  And that's what the assert ends up triggering on.

We could try to fix this overallocation, but I'm not entirely sure it's worth it.  We should seriously consider making mBufferSize non-debug (since it's not like the size of nsMappedAttributes would change!) and just using it here instead of using mAttrCount as a proxy.

Jonas, thoughts?
I'm not a huge fan of adding non-debug state, and the code that goes along with managing it, just to silence this assertion.

Is there some memory API that we can call which will return the true size of a malloc'ed buffer? Keeping in mind that performance isn't critical here (right?)
Performance is not critical for SizeOf(), yes.  Not sure whether we have such an API...

I assume you're not interested in making the assertion actually valid by making mBufferSize correct in this situation?  ;)
Hmm.. if we're updating mBufferSize we might as well just remove the assertion, right?
Well, the assertion is there because it's not obvious a priori that mBufferSize will always equal mAttrCount.  And in fact it caught a case when it's not.  Now this case happens to be one where we are in fact allocating too much space.  Should we try to avoid doing that, or not worry about it?
The other code explicitly does not worry about it since it uses >= comparisons.

I thought the concern here was that we wanted to ensure that we account for all allocated memory. Including the possible over-allocation.
The main concern here is to account for all allocated memory.

A secondary concern is whether we care about the over-allocation in this situation.  Perhaps we do not.
I don't care too much about the over allocation no. It seems unlikely to me that it'll cause a lot of extra memory use as it only happens when you set (mostly deprecated) styling attributes using the DOM.

I'm fine with making mBufferSize non-debug if that's what we need to do to do proper accounting. Though we could also just use mAttrCount and not worry about what likely is in the order of hundreds of bytes at the most.
Attached patch Patch v1Splinter Review
This patch prevents nsAttrAndChildArray from asking for a new slot when it's actually not needed. That way, we can keep the assert and we will know if that kind of situation happens again.
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #555433 - Flags: review?(jonas)
Attachment #555433 - Flags: feedback?(bzbarsky)
Whiteboard: [needs review]
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 555433 [details] [diff] [review]
Patch v1

This seems ok as long as there's no noticeable perf hit.
Attachment #555433 - Flags: feedback?(bzbarsky) → feedback+
That was quick, thanks :)
Whiteboard: [needs review] → [inbound]
http://hg.mozilla.org/mozilla-central/rev/22e2f80c2983
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.