768901 - DMD double-reports heap blocks from the CSS code

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nathan Froyd [:froydnj]]

I attempted to DMD firefox with the given URL and got quite a number of double reports:

==14443== Double report of heap block 0x3F62BE20:
==14443==  Allocated
==14443==    at 0x4C284AF: malloc (vg_replace_malloc.c:267)
==14443==    by 0x74630D8: moz_xmalloc (mozalloc.cpp:54)
==14443==    by 0x7F6E76C: nsCSSStyleSheet::nsCSSStyleSheet() (mozalloc.h:200)
==14443==    by 0x7F6E7B2: NS_NewCSSStyleSheet(nsCSSStyleSheet**) (nsCSSStyleSheet.cpp:2158)
==14443==    by 0x7F46304: mozilla::css::Loader::CreateSheet(nsIURI*, nsIContent*, nsIPrincipal*, bool, bool, nsAString_internal const&, mozilla::css::StyleSheetState&, bool*, nsCSSStyleSheet**) (Loader.cpp:1202)
==14443==    by 0x7F483A1: mozilla::css::Loader::LoadStyleLink(nsIContent*, nsIURI*, nsAString_internal const&, nsAString_internal const&, bool, nsICSSLoaderObserver*, bool*) (Loader.cpp:1884)
==14443==    by 0x80BEEF6: nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument*, nsICSSLoaderObserver*, bool*, bool*, bool) (nsStyleLinkElement.cpp:280)
==14443==    by 0x80BEF63: nsStyleLinkElement::UpdateStyleSheetInternal(nsIDocument*, bool) (nsStyleLinkElement.cpp:190)
==14443==    by 0x81B73DC: nsStyleLinkElement::UpdateStyleSheetInternal() (nsStyleLinkElement.h:58)
==14443==    by 0x81B6ECF: nsRunnableMethodImpl<void (nsHTMLLinkElement::*)(), true>::Run() (nsThreadUtils.h:349)
==14443==    by 0x804A914: nsContentUtils::RemoveScriptBlocker() (nsContentUtils.cpp:4879)
==14443==    by 0x8079BF4: nsDocument::EndUpdate(unsigned int) (nsDocument.cpp:3956)
==14443==  Previously reported by 'windows'
==14443==    at 0x82EEDC3: DOMStyleMallocSizeOf(void const*) (nsWindowMemoryReporter.cpp:102)
==14443==    by 0x7F6DBFC: nsCSSStyleSheetInner::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSStyleSheet.cpp:978)
==14443==    by 0x7F6DC7A: nsCSSStyleSheet::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSStyleSheet.cpp:849)
==14443==    by 0x8061B12: SizeOfStyleSheetsElementIncludingThis(nsIStyleSheet*, unsigned long (*)(void const*), void*) (nsDocument.cpp:9635)
==14443==    by 0x8A70A12: SizeOfElementIncludingThisEnumerator(void const*, void*) (nsVoidArray.cpp:737)
==14443==    by 0x8A71406: nsVoidArray::EnumerateForwards(bool (*)(void const*, void*), void*) const (nsVoidArray.cpp:704)
==14443==    by 0x8A71500: nsVoidArray::SizeOfExcludingThis(unsigned long (*)(void const*, unsigned long (*)(void const*), void*), unsigned long (*)(void const*), void*) const (nsVoidArray.cpp:755)
==14443==    by 0x80639D0: nsDocument::DocSizeOfExcludingThis(nsWindowSizes*) const (nsCOMArray.h:98)
==14443==    by 0x8215C0C: nsXMLDocument::DocSizeOfExcludingThis(nsWindowSizes*) const (nsXMLDocument.cpp:574)
==14443==    by 0x8061AF5: nsIDocument::DocSizeOfIncludingThis(nsWindowSizes*) const (nsDocument.cpp:9627)
==14443==    by 0x82BA622: nsGlobalWindow::SizeOfIncludingThis(nsWindowSizes*) const (nsGlobalWindow.cpp:10167)
==14443==    by 0x82EF445: CollectWindowReports(nsGlobalWindow*, nsWindowSizes*, nsTHashtable<nsUint64HashKey>*, nsIMemoryMultiReporterCallback*, nsISupports*) (nsWindowMemoryReporter.cpp:157)
==14443==  Now reported by 'windows'
==14443==    at 0x82EEDC3: DOMStyleMallocSizeOf(void const*) (nsWindowMemoryReporter.cpp:102)
==14443==    by 0x7F6DBFC: nsCSSStyleSheetInner::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSStyleSheet.cpp:978)
==14443==    by 0x7F6DC7A: nsCSSStyleSheet::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSStyleSheet.cpp:849)
==14443==    by 0x7F6DC34: nsCSSStyleSheetInner::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSStyleSheet.cpp:981)
==14443==    by 0x7F6DC7A: nsCSSStyleSheet::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSStyleSheet.cpp:849)
==14443==    by 0x8061B12: SizeOfStyleSheetsElementIncludingThis(nsIStyleSheet*, unsigned long (*)(void const*), void*) (nsDocument.cpp:9635)
==14443==    by 0x8A70A12: SizeOfElementIncludingThisEnumerator(void const*, void*) (nsVoidArray.cpp:737)
==14443==    by 0x8A71406: nsVoidArray::EnumerateForwards(bool (*)(void const*, void*), void*) const (nsVoidArray.cpp:704)
==14443==    by 0x8A71500: nsVoidArray::SizeOfExcludingThis(unsigned long (*)(void const*, unsigned long (*)(void const*), void*), unsigned long (*)(void const*), void*) const (nsVoidArray.cpp:755)
==14443==    by 0x80639D0: nsDocument::DocSizeOfExcludingThis(nsWindowSizes*) const (nsCOMArray.h:98)
==14443==    by 0x8215C0C: nsXMLDocument::DocSizeOfExcludingThis(nsWindowSizes*) const (nsXMLDocument.cpp:574)
==14443==    by 0x8061AF5: nsIDocument::DocSizeOfIncludingThis(nsWindowSizes*) const (nsDocument.cpp:9627)

==14443== Double report of heap block 0x15E68730:
==14443==  Allocated
==14443==    at 0x4C284AF: malloc (vg_replace_malloc.c:267)
==14443==    by 0x74630D8: moz_xmalloc (mozalloc.cpp:54)
==14443==    by 0x7F574CB: (anonymous namespace)::CSSParserImpl::ParseImportRule(void (*)(mozilla::css::Rule*, void*), void*) (mozalloc.h:200)
==14443==    by 0x7F50C3D: (anonymous namespace)::CSSParserImpl::ParseAtRule(void (*)(mozilla::css::Rule*, void*), void*, bool) (nsCSSParser.cpp:1510)
==14443==    by 0x7F5B967: nsCSSParser::ParseSheet(nsAString_internal const&, nsIURI*, nsIURI*, nsIPrincipal*, unsigned int, bool) (nsCSSParser.cpp:901)
==14443==    by 0x7F48972: mozilla::css::Loader::ParseSheet(nsAString_internal const&, mozilla::css::SheetLoadData*, bool&) (Loader.cpp:1611)
==14443==    by 0x7F48EBE: mozilla::css::SheetLoadData::OnStreamComplete(nsIUnicharStreamLoader*, nsISupports*, unsigned int, nsAString_internal const&) (Loader.cpp:959)
==14443==    by 0x7CFD2B4: nsUnicharStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (nsUnicharStreamLoader.cpp:92)
==14443==    by 0x7CD7889: nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (nsBaseChannel.cpp:713)
==14443==    by 0x7CE0AB5: nsInputStreamPump::OnStateStop() (nsInputStreamPump.cpp:555)
==14443==    by 0x7CE0D83: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:373)
==14443==    by 0x8A96589: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:82)
==14443==  Previously reported by 'windows'
==14443==    at 0x82EEDC3: DOMStyleMallocSizeOf(void const*) (nsWindowMemoryReporter.cpp:102)
==14443==    by 0x7F64F3B: mozilla::css::ImportRule::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSRules.cpp:479)
==14443==    by 0x7F64E0D: mozilla::css::Rule::SizeOfCOMArrayElementIncludingThis(mozilla::css::Rule*, unsigned long (*)(void const*), void*) (nsCSSRules.cpp:93)
==14443==    by 0x8A70A12: SizeOfElementIncludingThisEnumerator(void const*, void*) (nsVoidArray.cpp:737)
==14443==    by 0x8A71406: nsVoidArray::EnumerateForwards(bool (*)(void const*, void*), void*) const (nsVoidArray.cpp:704)
==14443==    by 0x8A71500: nsVoidArray::SizeOfExcludingThis(unsigned long (*)(void const*, unsigned long (*)(void const*), void*), unsigned long (*)(void const*), void*) const (nsVoidArray.cpp:755)
==14443==    by 0x7F6DC17: nsCSSStyleSheetInner::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCOMArray.h:98)
==14443==    by 0x7F6DC7A: nsCSSStyleSheet::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSStyleSheet.cpp:849)
==14443==    by 0x8061B12: SizeOfStyleSheetsElementIncludingThis(nsIStyleSheet*, unsigned long (*)(void const*), void*) (nsDocument.cpp:9635)
==14443==    by 0x8A70A12: SizeOfElementIncludingThisEnumerator(void const*, void*) (nsVoidArray.cpp:737)
==14443==    by 0x8A71406: nsVoidArray::EnumerateForwards(bool (*)(void const*, void*), void*) const (nsVoidArray.cpp:704)
==14443==    by 0x8A71500: nsVoidArray::SizeOfExcludingThis(unsigned long (*)(void const*, unsigned long (*)(void const*), void*), unsigned long (*)(void const*), void*) const (nsVoidArray.cpp:755)
==14443==  Now reported by 'windows'
==14443==    at 0x82EEDC3: DOMStyleMallocSizeOf(void const*) (nsWindowMemoryReporter.cpp:102)
==14443==    by 0x7F64F3B: mozilla::css::ImportRule::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSRules.cpp:479)
==14443==    by 0x7F64E0D: mozilla::css::Rule::SizeOfCOMArrayElementIncludingThis(mozilla::css::Rule*, unsigned long (*)(void const*), void*) (nsCSSRules.cpp:93)
==14443==    by 0x8A70A12: SizeOfElementIncludingThisEnumerator(void const*, void*) (nsVoidArray.cpp:737)
==14443==    by 0x8A71406: nsVoidArray::EnumerateForwards(bool (*)(void const*, void*), void*) const (nsVoidArray.cpp:704)
==14443==    by 0x8A71500: nsVoidArray::SizeOfExcludingThis(unsigned long (*)(void const*, unsigned long (*)(void const*), void*), unsigned long (*)(void const*), void*) const (nsVoidArray.cpp:755)
==14443==    by 0x7F6DC17: nsCSSStyleSheetInner::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCOMArray.h:98)
==14443==    by 0x7F6DC7A: nsCSSStyleSheet::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSStyleSheet.cpp:849)
==14443==    by 0x7F6DC34: nsCSSStyleSheetInner::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSStyleSheet.cpp:981)
==14443==    by 0x7F6DC7A: nsCSSStyleSheet::SizeOfIncludingThis(unsigned long (*)(void const*)) const (nsCSSStyleSheet.cpp:849)
==14443==    by 0x8061B12: SizeOfStyleSheetsElementIncludingThis(nsIStyleSheet*, unsigned long (*)(void const*), void*) (nsDocument.cpp:9635)
==14443==    by 0x8A70A12: SizeOfElementIncludingThisEnumerator(void const*, void*) (nsVoidArray.cpp:737)

...and more like this, but I think those illustrate the point well enough: we have memory shared by nsCSSStyleSheetInner and we're double-counting that.  Looks like the style memory reporting has to become a bit more sophisticated.

Comment 1

[Nicholas Nethercote [inactive]]

I've seen these also when the error console is open, and (I think?) when multiple browser windows are open.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like nsCSSStyleSheet::SizeOfIncludingThis assumes that it owns its mInner.  In fact multiple style sheets can share an mInner (which is then copy-on-write if there are multiple owners) -- that's why there's a sheet vs. inner separation.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(It should probably just check if it is the inner's mSheets[0].)

Comment 5

[Nicholas Nethercote [inactive]]

> Looks like nsCSSStyleSheet::SizeOfIncludingThis assumes that it owns its
> mInner.  In fact multiple style sheets can share an mInner (which is then
> copy-on-write if there are multiple owners) -- that's why there's a sheet
> vs. inner separation.
>
> (It should probably just check if it is the inner's mSheets[0].)

So nsCSSStyleSheetInner::mSheets holds pointers to each of the nsCSSStyleSheets that share the inner?  I see.  There will be a bit of arbitrariness in terms of who gets blamed for the memory consumption of the inner, but that seems hard to avoid and double-counting is worse.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Don't double-count shared nsCSSStyleSheetInners.

This simple patch follows dbaron's suggestion.  It removes all of DMD's
double-counting warnings for both (a) the two-window desktop browser case,
and (b) B2G.  Hurray.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 669799 [details] [diff] [review]
Don't double-count shared nsCSSStyleSheetInners.

s->mInner in the if condition, as long as we're using "s" everywhere here, and r=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

One note, though: How do we decide which nsCSSStyleSheets to measure right now?  If we're not doing the ones in the caches in the css::Loader for the document, this patch can cause us to miss sheets altogether in cases when we prefetched...

I wonder whether we should avoid sticking the prefetch sheets into the table, since that can cause use to use more memory when CSSOM is used.   Maybe a followup bug on that?

Comment 9

[Boris Zbarsky [:bzbarsky]]

And definitely a followup on memory-reporting those hashtables in the loader, please!

Comment 10

[Nicholas Nethercote [inactive]]

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

Comment 11

[Nicholas Nethercote [inactive]]

> And definitely a followup on memory-reporting those hashtables in the
> loader, please!

Bug 799796.



> One note, though: How do we decide which nsCSSStyleSheets to measure right
> now?  If we're not doing the ones in the caches in the css::Loader for the
> document, this patch can cause us to miss sheets altogether in cases when we
> prefetched...

We just measure sheets in documents and the style-sheet-cache, IIRC.

> I wonder whether we should avoid sticking the prefetch sheets into the
> table, since that can cause use to use more memory when CSSOM is used.  
> Maybe a followup bug on that?

Can you file that bug?  I don't understand the idea enough to write something sensible.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Can you file that bug?  I don't understand the idea enough to write something sensible.

Filed bug 799816.

Comment 13

[Ed Morley [:emorley]]

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

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 669799 [details] [diff] [review]
Don't double-count shared nsCSSStyleSheetInners.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): memory reporting.

User impact if declined: misleading memory reporting which will hurt understanding of B2G memory consumption.

Testing completed (on m-c, etc.): has been on m-c for 16 days without problem.

Risk to taking this patch (and alternatives if risky): minimal;  very simple patch and the code only runs when viewing about:memory or triggering a memory report dump.

String or UUID changes made by this patch: none.

Comment 15

[bhavana bajaj [:bajaj]]

Comment on attachment 669799 [details] [diff] [review]
Don't double-count shared nsCSSStyleSheetInners.

Approving for aurora as it is low risk and help understanding memory consumption better

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f37aaa95fee3