382027 - Loading gmail leaks nsCSSValuePair objects

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Start the browser with empty page, go to www.gmail.com, login, wait until the page has loaded, logout, close browser.
Few (~15) nsCSSValuePair objects are leaked.

Comment 1

[-fullmetaljacket-]

related to bug 365983?

Comment 2

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

The number leaked seems to correspond to an imbalance between nsCSSDeclaration::CompressFrom and ~nsCSSDeclaration.  I see the same number of constructors from the following stack, so they may be the source:

<nsCSSValuePair> 0x09B61454 777 Ctor (16)
nsCSSValuePair (/builds/trunk/mozilla/layout/style/nsCSSStruct.h:112)
.L385 (/builds/trunk/mozilla/layout/style/nsCSSDataBlock.cpp:818)
nsCSSDeclaration::CompressFrom(nsCSSExpandedDataBlock*) (/builds/trunk/mozilla/layout/style/nsCSSDeclaration.h:115)
CSSParserImpl::ParseProperty(nsCSSProperty, nsAString_internal const&, nsIURI*, nsIURI*, nsIPrincipal*, nsCSSDeclaration*, int*) (/builds
nsDOMCSSDeclaration::ParsePropertyValue(nsCSSProperty, nsAString_internal const&) (/builds/trunk/mozilla/layout/style/nsDOMCSSDeclaration
nsDOMCSSDeclaration::SetPropertyValue(nsCSSProperty, nsAString_internal const&) (/builds/trunk/mozilla/layout/style/nsDOMCSSDeclaration.c
CSS2PropertiesTearoff::SetDisplay(nsAString_internal const&) (/builds/trunk/mozilla/layout/style/nsCSSPropList.h:385)

Comment 3

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

Attachment: patch

This fixes it.  I think the leak is harmless, though, and I'm not sure if there's a better accounting technique we could use so that we do less work.

Comment 4

[Eli Friedman]

This leak also occurs with the following markup:
<svg xmlns="http://www.w3.org/2000/svg" stroke="black" stroke-width="20" />

Comment 5

[Robert Sayre]

Comment on attachment 274379 [details] [diff] [review]
patch

This patch also fixes a leak in layout/reftests/bugs/371043-1.html

Comment 6

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

Attachment: patch

So this has the leak fix, and also some changes to nsCSSValue that are somewhat based on changes I recall eli writing a while ago (although he'd copied the contents of Reset to two places, which I objected to).  This makes Reset inline the check against a null unit, but the rest not be inlined.

Comment 7

[Eli Friedman]

Comment on attachment 277779 [details] [diff] [review]
patch

>+    if (mUnit != eCSSUnit_Null)
>+      DoReset();

You can make the check here stronger (although slightly larger in codesize) by checking "if (mUnit >= eCSSUnit_String && mUnit <= eCSSUnit_Image).

We could also rearrange nsCSSUnit to make this check faster (I know that nsCSSUnit is arranged into groups, but is there some specific reason for the current numbering scheme?).

Comment 8

[Boris Zbarsky [:bzbarsky]]

So are we expanding into an expanded block that already has something in it?  If so, why?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Er, nevermind.  I see what's going on here.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277779 [details] [diff] [review]
patch

>+                static_cast<nsCSSValue*>(prop)->~nsCSSValue();

So could we assert that prop has the null value and only do the ~nsCSSValue if refcount logging is enabled?  That would fix the logging code and shouldn't leak unless the assert fails (which it really shouldn't).

Similar for the other changes in this file.

Of course we can also just call the dtor unconditionally.

r+sr=bzbarsky either way, though we could also look into Eli's suggestions.

Comment 11

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

Attachment: patch

This does the assert-and-ifdef approach Boris suggested in comment 10.

Comment 12

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

Checked in to trunk.

Comment 13

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

I also had to fix the operator== to handle the union not being initialized in the cases where it didn't need to be.  (That could have been an existing problem if sizeof(float) > sizeof(PRInt32).)

Comment 14

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

I just checked in a test that catches the mistake in comment 13 through something other than an unexpected pass (not really a test for this bug).