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

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?)

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?  ;)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. if we're updating mBufferSize we might as well just remove the assertion, right?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 555433 [details] [diff] [review]
Patch v1

This seems ok as long as there's no noticeable perf hit.

Comment 12

[Mounir Lamouri (:mounir)]

That was quick, thanks :)

Comment 13

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/22e2f80c2983