Last Comment Bug 768901 - DMD double-reports heap blocks from the CSS code
: DMD double-reports heap blocks from the CSS code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla19
Assigned To: Nicholas Nethercote [:njn]
:
: Jet Villegas (:jet)
Mentors:
http://build.chromium.org/f/chromium/...
: 799336 (view as bug list)
Depends on:
Blocks: 799796
  Show dependency treegraph
 
Reported: 2012-06-27 08:19 PDT by Nathan Froyd [:froydnj]
Modified: 2012-10-27 17:01 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Don't double-count shared nsCSSStyleSheetInners. (1.55 KB, patch)
2012-10-09 16:32 PDT, Nicholas Nethercote [:njn]
bzbarsky: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2012-06-27 08:19:23 PDT
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 [:njn] 2012-06-27 15:36:20 PDT
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-10 2012-06-27 15:50:48 PDT
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-10 2012-06-27 15:54:17 PDT
(It should probably just check if it is the inner's mSheets[0].)
Comment 4 Nathan Froyd [:froydnj] 2012-10-09 05:15:17 PDT
*** Bug 799336 has been marked as a duplicate of this bug. ***
Comment 5 Nicholas Nethercote [:njn] 2012-10-09 14:49:44 PDT
> 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 [:njn] 2012-10-09 16:32:09 PDT
Created attachment 669799 [details] [diff] [review]
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 [:bz] (still a bit busy) 2012-10-09 18:09:42 PDT
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 [:bz] (still a bit busy) 2012-10-09 18:13:15 PDT
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 [:bz] (still a bit busy) 2012-10-09 18:13:38 PDT
And definitely a followup on memory-reporting those hashtables in the loader, please!
Comment 10 Nicholas Nethercote [:njn] 2012-10-09 20:25:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b275f72aba11
Comment 11 Nicholas Nethercote [:njn] 2012-10-09 20:29:06 PDT
> 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 [:bz] (still a bit busy) 2012-10-09 20:31:36 PDT
> Can you file that bug?  I don't understand the idea enough to write something sensible.

Filed bug 799816.
Comment 13 Ed Morley [:emorley] 2012-10-11 07:06:27 PDT
https://hg.mozilla.org/mozilla-central/rev/b275f72aba11
Comment 14 Nicholas Nethercote [:njn] 2012-10-26 12:51:39 PDT
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] 2012-10-26 15:29:39 PDT
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] 2012-10-27 17:01:55 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/f37aaa95fee3

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