Closed Bug 820134 Opened 12 years ago Closed 2 years ago

DMD reports ~500kb of unreported memory in four string allocations in CSS parser parseIdent. At least some are from bug 820940.

Categories

(Core :: CSS Parsing and Computation, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: justin.lebar+bug, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

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.
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 :(
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?
Summary: New DMD reports ~500kb of unreported memory in four string allocations in CSS parser → New DMD reports ~500kb of unreported memory in four string allocations in CSS parser parseIdent
> 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.
Whiteboard: [MemShrink]
A 125KB-average identifier is still pretty weird, yes.  ;)
I've seen this twice now, which indicates to me that this may be a legit leak (or something).
Assignee: nobody → justin.lebar+bug
Whiteboard: [MemShrink] → [MemShrink:P2]
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?
> 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...
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.
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]
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.
Depends on: 820940
Summary: New DMD reports ~500kb of unreported memory in four string allocations in CSS parser parseIdent → DMD reports ~500kb of unreported memory in four string allocations in CSS parser parseIdent. At least some are from bug 820940.
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.
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.  :(
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.
> 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 :/
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.

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Assignee: justin.lebar+bug → nobody

The CSS implementation has changed a lot in the last decade. If we are continuing to see unreported memory, we can file a new bug.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.