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

RESOLVED FIXED in mozilla9

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: mounir)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla9
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
Created attachment 552968 [details]
stack trace

Comment 2

6 years ago
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?)

Comment 4

6 years ago
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?

Comment 6

6 years ago
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.

Comment 8

6 years ago
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.
(Assignee)

Comment 10

6 years ago
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.
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #555433 - Flags: review?(jonas)
Attachment #555433 - Flags: feedback?(bzbarsky)
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All

Comment 11

6 years ago
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+
Attachment #555433 - Flags: review?(jonas) → review+
(Assignee)

Comment 12

6 years ago
That was quick, thanks :)
Whiteboard: [needs review] → [inbound]
http://hg.mozilla.org/mozilla-central/rev/22e2f80c2983
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.