Last Comment Bug 820134 - DMD reports ~500kb of unreported memory in four string allocations in CSS parser parseIdent. At least some are from bug 820940.
: DMD reports ~500kb of unreported memory in four string allocations in CSS par...
Status: NEW
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal with 4 votes (vote)
: ---
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on: 820940
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2012-12-10 13:53 PST by Justin Lebar (not reading bugmail)
Modified: 2012-12-14 14:54 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Justin Lebar (not reading bugmail) 2012-12-10 13:53:21 PST
In the main B2G process with the gallery app open (see attachment 690543 [details]), I see:

> Unreported: 4 blocks in block group 7 of 533
>  524,288 bytes (516,136 requested / 8,152 slop)
>  2.17% of the heap (29.53% cumulative);  4.28% of unreported (58.14% cumulative)
>  Allocated at
>        realloc /Users/jlebar/code/moz/ff-git/src/memory/build/replace_malloc.c:192 (0x4020b1d0 libmozglue.so+0x41d0)
>        moz_realloc /Users/jlebar/code/moz/ff-git/src/memory/mozalloc/mozalloc.cpp:97 (0x40e5adea libxul.so+0xc37dea)
>        nsStringBuffer::Realloc(nsStringBuffer*, unsigned int) /Users/jlebar/code/moz/ff-git/src/xpcom/string/src/nsSubstring.cpp:208 (0x40c74538 libxul.so+0xa51538)
>        nsAString_internal::MutatePrep(unsigned int, unsigned short**, unsigned int*) /Users/jlebar/code/moz/ff-git/src/xpcom/string/src/nsTSubstring.cpp:104 (0x40c74642 libxul.so+0xa51642)
>        nsAString_internal::ReplacePrepInternal(unsigned int, unsigned int, unsigned int, unsigned int) /Users/jlebar/code/moz/ff-git/src/xpcom/string/src/nsTSubstring.cpp:166 (0x40c74ba2 libxul.so+0xa51ba2)
>        nsAString_internal::ReplacePrep(unsigned int, unsigned int, unsigned int) /Users/jlebar/code/moz/B2G/objdir-gecko/dist/include/nsTSubstring.h:720 (0x40c74c64 libxul.so+0xa51c64)
>        nsAString_internal::Replace(unsigned int, unsigned int, unsigned short) /Users/jlebar/code/moz/ff-git/src/xpcom/string/src/nsTSubstring.cpp:456 (0x41978d16 libxul.so+0xa51d16)
>        nsAString_internal::Append(unsigned short) /Users/jlebar/code/moz/B2G/objdir-gecko/dist/include/nsTSubstring.h:398 (0x41330b10 libxul.so+0x409b10)
>        nsCSSScanner::ParseIdent(int, nsCSSToken&) /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSScanner.cpp:803 (0x4062cbc6 libxul.so+0x409bc6)
>        nsCSSScanner::Next(nsCSSToken&) /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSScanner.cpp:420 (0x4062ce2a libxul.so+0x409e2a)
>        (anonymous namespace)::CSSParserImpl::GetToken(bool) /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSParser.cpp:1288 (0x4061a974 libxul.so+0x3f7974)
>        (anonymous namespace)::CSSParserImpl::ParseVariant(nsCSSValue&, int, int const*) /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSParser.cpp:4660 (0x4061d8d4 libxul.so+0x3fa8d4)
>        ParseValueList /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSParser.cpp:6602 (0x413273e6 libxul.so+0x4003e6)
>        ParseProperty /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSParser.cpp:1100 (0x40623b6a libxul.so+0x400b6a)
>        nsDOMCSSDeclaration::ParsePropertyValue(nsCSSProperty, nsAString_internal const&, bool) /Users/jlebar/code/moz/B2G/objdir-gecko/dist/include/nsError.h:155 (0x406381b4 libxul.so+0x4151b4)
>        nsDOMCSSDeclaration::SetPropertyValue(nsCSSProperty, nsAString_internal const&) /Users/jlebar/code/moz/ff-git/src/layout/style/nsDOMCSSDeclaration.cpp:63 (0x40638262 libxul.so+0x415262)
>        mozilla::ErrorResult::operator=(tag_nsresult) /Users/jlebar/code/moz/B2G/objdir-gecko/dist/include/mozilla/ErrorResult.h:66 (0x4193a230 libxul.so+0xa13230)
>        mozilla::dom::CSS2PropertiesBinding::genericSetter(JSContext*, unsigned int, JS::Value*) /Users/jlebar/code/moz/B2G/objdir-gecko/dom/bindings/CSS2PropertiesBinding.cpp:13515 (0x40c35528 libxul.so+0xa12528)

I've never seen something like this before.  It's possible this memory goes
away quickly and I just happened to run DMD at an inopportune time.
Comment 1 Nicholas Nethercote [:njn] 2012-12-10 14:07:07 PST
There's a bunch of CSS stuff that in theory is covered by existing CSS reporters, but which bz and I haven't been able to track down in the past.  It seems like we're missing some sheets, but we don't know where :(
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-12-10 14:08:26 PST
The current token ident should be transient, yes.  On the other hand, having it be 500KB long is pretty weird.  Looks like this is a scripted style set.. Wonder what's setting what to what?
Comment 3 Justin Lebar (not reading bugmail) 2012-12-10 19:56:36 PST
> On the other hand, having it be 500KB long is pretty weird.

FWIW it's 500KB split between four heap allocations.  I don't know if that makes it any less weird.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-12-10 20:31:52 PST
A 125KB-average identifier is still pretty weird, yes.  ;)
Comment 5 Justin Lebar (not reading bugmail) 2012-12-11 13:30:43 PST
I've seen this twice now, which indicates to me that this may be a legit leak (or something).
Comment 6 Justin Lebar (not reading bugmail) 2012-12-12 09:31:26 PST
The code in question in nsCSSScanner::ParseIdent does

>    if (ident.LowerCaseEqualsLiteral("url")) {
>      NextURL(aToken); // ignore return value, since *we* read something
>      return true;
>    }

So how much do we want to bet this is a data URI?

Should this string be ephemeral?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-12-12 10:23:27 PST
> So how much do we want to bet this is a data URI?

Aha!  Totally no bet.  ;)

That would likely not be ephemeral; it almost certainly gets stored in a CSSValue...
Comment 8 Justin Lebar (not reading bugmail) 2012-12-12 10:28:56 PST
Yeah, one cause of this is

gaia/apps/system/js/boostrap.js's

>SettingsListener.observe(
>  'wallpaper.image',
>  'resources/images/backgrounds/default.png',
>  function setWallpaper(value) {
>    document.getElementById('screen').style.backgroundImage =
>      'url(' + value + ')';
>  }
>);

Apparently we're setting a data: URI here.  This is a length-44740 PRUnichar, so ~89kb.
Comment 9 Justin Lebar (not reading bugmail) 2012-12-12 10:32:15 PST
Then we have the lock-screen's background, another length-44740 string.  I wonder if it's the same string; that would be awesome.

> (gdb) call DumpJSStack()
> 0 ls_updateBackground() ["app://system.gaiamobile.org/js/lockscreen.js":879]
>     this = [object Object]
> 1 anonymous() ["app://system.gaiamobile.org/js/lockscreen.js":166]
>     <failed to get 'this' value>
> 2 onsuccess() ["app://system.gaiamobile.org/shared/js/settings_listener.js":61]
>     this = [object DOMRequest]
Comment 10 Justin Lebar (not reading bugmail) 2012-12-12 10:40:05 PST
Okay, I filed bug 820940 on this gaia behavior.  I wasn't able to see any other instances of huge-string creation in this code other than these two callsites, but maybe Gaia is responsible for all 500kb here.
Comment 11 Justin Lebar (not reading bugmail) 2012-12-13 08:05:40 PST
Boris, do you know where these strings end up being stored?  I feel like it might be worthwhile to add a memory reporter here, even if we end up fixing Gaia.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2012-12-13 11:05:49 PST
Sure.  When we parse a url() in a stylesheet, it get stored in the mString member of a mozilla::css::URLValue struct, and an nsCSSValue holds a pointer to the URLValue.  The nsCSSValue is stored in various places and can get copied and such.  The problem is that copying just addrefs the URLValue, so in practice it can end up getting pointed to from multiple locations.  :(
Comment 13 Andrew McCreight [:mccr8] 2012-12-13 11:20:33 PST
During the MemShrink meeting, njn said that in another case of shared data (images maybe?) we include ref counted things in the count if their refcount is 1. So possibly that would help some.
Comment 14 Nicholas Nethercote [:njn] 2012-12-13 14:04:00 PST
> njn said that in another case of shared data (images maybe?) we include ref
> counted things in the count if their refcount is 1.

It's for strings.  See http://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsStringBuffer.h#141.  We have SizeOfIncludingThisMustBeUnshared() and SizeOfIncludingThisIfUnshared().  You use the former if you know from context that the string isn't shared.

I fully admit to not having any data on how effective this approach is, and how much it causes us to miss.  An obvious idea is to divide the size by the number of references, but that doesn't interact well with DMD, because DMD can't handle the notion of a fractional block :/
Comment 15 Justin Lebar (not reading bugmail) 2012-12-14 14:54:39 PST
One way to handle this is to accumulate data during the window memory reporter which accounts for every CSS string.  Then once we've touched every window, we can figure out which strings are owned by which windows and account for them accordingly.

It's a relatively large and ugly change to the DOM memory reporters, which are already somewhat large and ugly.

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