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
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)
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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
>        moz_realloc /Users/jlebar/code/moz/ff-git/src/memory/mozalloc/mozalloc.cpp:97 (0x40e5adea
>        nsStringBuffer::Realloc(nsStringBuffer*, unsigned int) /Users/jlebar/code/moz/ff-git/src/xpcom/string/src/nsSubstring.cpp:208 (0x40c74538
>        nsAString_internal::MutatePrep(unsigned int, unsigned short**, unsigned int*) /Users/jlebar/code/moz/ff-git/src/xpcom/string/src/nsTSubstring.cpp:104 (0x40c74642
>        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
>        nsAString_internal::ReplacePrep(unsigned int, unsigned int, unsigned int) /Users/jlebar/code/moz/B2G/objdir-gecko/dist/include/nsTSubstring.h:720 (0x40c74c64
>        nsAString_internal::Replace(unsigned int, unsigned int, unsigned short) /Users/jlebar/code/moz/ff-git/src/xpcom/string/src/nsTSubstring.cpp:456 (0x41978d16
>        nsAString_internal::Append(unsigned short) /Users/jlebar/code/moz/B2G/objdir-gecko/dist/include/nsTSubstring.h:398 (0x41330b10
>        nsCSSScanner::ParseIdent(int, nsCSSToken&) /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSScanner.cpp:803 (0x4062cbc6
>        nsCSSScanner::Next(nsCSSToken&) /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSScanner.cpp:420 (0x4062ce2a
>        (anonymous namespace)::CSSParserImpl::GetToken(bool) /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSParser.cpp:1288 (0x4061a974
>        (anonymous namespace)::CSSParserImpl::ParseVariant(nsCSSValue&, int, int const*) /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSParser.cpp:4660 (0x4061d8d4
>        ParseValueList /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSParser.cpp:6602 (0x413273e6
>        ParseProperty /Users/jlebar/code/moz/ff-git/src/layout/style/nsCSSParser.cpp:1100 (0x40623b6a
>        nsDOMCSSDeclaration::ParsePropertyValue(nsCSSProperty, nsAString_internal const&, bool) /Users/jlebar/code/moz/B2G/objdir-gecko/dist/include/nsError.h:155 (0x406381b4
>        nsDOMCSSDeclaration::SetPropertyValue(nsCSSProperty, nsAString_internal const&) /Users/jlebar/code/moz/ff-git/src/layout/style/nsDOMCSSDeclaration.cpp:63 (0x40638262
>        mozilla::ErrorResult::operator=(tag_nsresult) /Users/jlebar/code/moz/B2G/objdir-gecko/dist/include/mozilla/ErrorResult.h:66 (0x4193a230
>        mozilla::dom::CSS2PropertiesBinding::genericSetter(JSContext*, unsigned int, JS::Value*) /Users/jlebar/code/moz/B2G/objdir-gecko/dom/bindings/CSS2PropertiesBinding.cpp:13515 (0x40c35528

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] 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] 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] 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


>  '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://":879]
>     this = [object Object]
> 1 anonymous() ["app://":166]
>     <failed to get 'this' value>
> 2 onsuccess() ["app://":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] 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  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.