1026985 - Stop reporting the memory use of nsAttrValue's MiscContainer objects if they are shared

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

I'm seeing a single twice-reported entry in a DMD report. In that entry the "Reported at" and "Reported again at" stacks are identical. From code inspection I can't see how that is possible.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: twice reported section

Comment 2

[Jonathan Watt [:jwatt]]

This happened when I ran Firefox with the -layoutdebug command line argument, then loaded:

http://www.w3schools.com/CSSref/tryit.asp?filename=trycss_gen_counter-reset

before triggering DMD's report.

Comment 3

[Jonathan Watt [:jwatt]]

Consider the stack:

  WindowsMallocSizeOf
  nsAttrValue::SizeOfExcludingThis
  nsAttrAndChildArray::SizeOfExcludingThis
  mozilla::dom::FragmentOrElement::SizeOfExcludingThis
  nsINode::SizeOfIncludingThis
  nsDocument::DocAddSizeOfExcludingThis
  nsHTMLDocument::DocAddSizeOfExcludingThis

We can be pretty sure that nsDocument::DocAddSizeOfExcludingThis is not being called twice since if it was we'd be getting much more than just the single twice-reported entry.

nsDocument::DocAddSizeOfExcludingThis is clearly at this line when nsINode::SizeOfIncludingThis is called:

https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp?rev=72961f73cb4a#11955

and that section of code can't be creating a duplicate report - if the nsINode was in the child list twice then the nsINode itself would also appear as twice-reported.

nsINode::SizeOfIncludingThis also doesn't seem to be responsible since it just does |aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf)|.

FragmentOrElement::SizeOfExcludingThis calls mAttrsAndChildren.SizeOfExcludingThis() only once, and nsAttrValue::SizeOfExcludingThis only calls aMallocSizeOf() once on the MiscContainer that is being double reported.

Comment 4

[Jonathan Watt [:jwatt]]

Any thoughts?

Comment 5

[Eric Rahm [:erahm]]

This is probably a bug in the memory reporter (not DMD) where a shared MiscContainer is actually being twice reported. A comment in |nsAttrValue::ClearMiscContainer| implies that MiscContainers can be shared [1]. I haven't completely traced through, but it looks like two nsAttrAndChildArrays are sharing a MiscContainer.

[1] http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsAttrValue.cpp#1768

Comment 6

[Jonathan Watt [:jwatt]]

You're right. I should have spotted that, thanks!

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 8

[Nicholas Nethercote [inactive]]

Thanks for spotting, erahm!

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8442235 [details] [diff] [review]
patch

r=me

Comment 10

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f5029982f2

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b1f5029982f2