Last Comment Bug 678820 - about:memory - nsMappedAttributes::SizeOf - "ASSERTION: mBufferSize and mAttrCount are expected to be the same"
: about:memory - nsMappedAttributes::SizeOf - "ASSERTION: mBufferSize and mAttr...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 336383 669904
  Show dependency treegraph
 
Reported: 2011-08-14 06:21 PDT by Jesse Ruderman
Modified: 2011-08-24 17:27 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (131 bytes, text/html)
2011-08-14 06:21 PDT, Jesse Ruderman
no flags Details
stack trace (1.01 KB, text/plain)
2011-08-14 06:22 PDT, Jesse Ruderman
no flags Details
Patch v1 (1.19 KB, patch)
2011-08-24 09:23 PDT, Mounir Lamouri (:mounir)
jonas: review+
bzbarsky: feedback+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-08-14 06:21:53 PDT
Created attachment 552967 [details]
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 2011-08-14 06:22:41 PDT
Created attachment 552968 [details]
stack trace
Comment 2 Boris Zbarsky [:bz] 2011-08-23 00:59:21 PDT
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) PTO Until July 5th 2011-08-23 01:11:23 PDT
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 [:bz] 2011-08-23 01:14:36 PDT
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) PTO Until July 5th 2011-08-23 01:21:57 PDT
Hmm.. if we're updating mBufferSize we might as well just remove the assertion, right?
Comment 6 Boris Zbarsky [:bz] 2011-08-23 01:31:50 PDT
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) PTO Until July 5th 2011-08-23 01:50:05 PDT
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 [:bz] 2011-08-23 10:35:06 PDT
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) PTO Until July 5th 2011-08-23 13:38:49 PDT
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) 2011-08-24 09:23:33 PDT
Created attachment 555433 [details] [diff] [review]
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 [:bz] 2011-08-24 09:35:47 PDT
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) 2011-08-24 10:27:08 PDT
That was quick, thanks :)
Comment 13 Matt Brubeck (:mbrubeck) 2011-08-24 17:27:35 PDT
http://hg.mozilla.org/mozilla-central/rev/22e2f80c2983

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