1350789 - Possible error in CSSStyleSheet::SizeOfIncludingThis

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Nicholas Nethercote [inactive]]

Bug 1290218 refactored some layout data structures, and may have introduced a bug in memory reporting.

In this code in CSSStyleSheet::SizeOfIncludingThis:

> const CSSStyleSheet* s = this;
> while (s) {
>   // Each inner can be shared by multiple sheets.  So we only count the inner
>   // if this sheet is the last one in the list of those sharing it.  As a
>   // result, the last such sheet takes all the blame for the memory
>   // consumption of the inner, which isn't ideal but it's better than
>   // double-counting the inner.  We use last instead of first since the first
>   // sheet may be held in the nsXULPrototypeCache and not used in a window at
>   // all.
>   if (mInner->mSheets.LastElement() == s) {
>     n += Inner()->SizeOfIncludingThis(aMallocSizeOf);
>   }
> 
>   // Measurement of the following members may be added later if DMD finds it
>   // is worthwhile:
>   // - s->mRuleCollection
>   // - s->mRuleProcessors
>   //
>   // The following members are not measured:
>   // - s->mOwnerRule, because it's non-owning
> 
>   s = s->mNext ? s->mNext->AsGecko() : nullptr;
> }

The uses of mInner and Inner() within the loop are suspicious. They should
probably be s->mInner and s->Inner().

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1350789 - Make CSSStyleSheet::SizeOfIncludingThis correctly check for shared inners.

Review commit: https://reviewboard.mozilla.org/r/123838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123838/

Comment 2

[Brad Werth [:bradwerth]]

Comment on attachment 8851510 [details]
Bug 1350789 - Make CSSStyleSheet::SizeOfIncludingThis correctly check for shared inners.

https://reviewboard.mozilla.org/r/123838/#review126388

::: layout/style/CSSStyleSheet.cpp:188
(Diff revision 1)
>      // result, the last such sheet takes all the blame for the memory
>      // consumption of the inner, which isn't ideal but it's better than
>      // double-counting the inner.  We use last instead of first since the first
>      // sheet may be held in the nsXULPrototypeCache and not used in a window at
>      // all.
> -    if (mInner->mSheets.LastElement() == s) {
> +    if (s->Inner()->mSheets.LastElement() == s) {

My understanding is that the sheets reachable by the mNext pointer all share the same inner with this. If that's true, then the patch does not change behavior, though I agree it reads better this way. My assumption could be confirmed with the addition of:

MOZ_ASSERT(s->Inner() == Inner(), "All linked sheets should share the same inner.");

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Brad Werth [:bradwerth] from comment #2)
> My understanding is that the sheets reachable by the mNext pointer all share
> the same inner with this. If that's true, then the patch does not change
> behavior, though I agree it reads better this way. My assumption could be
> confirmed with the addition of:

I don't think that's true.  The mFirstChild / mNext linked list is of child style sheets, so one for each @import rule in a sheet.  They will use the same inner only if they refer to the same URL.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ca911e49616
Make CSSStyleSheet::SizeOfIncludingThis correctly check for shared inners. r=bradwerth

Comment 5

[Brad Werth [:bradwerth]]

(In reply to Cameron McCormack (:heycam) from comment #3)
> I don't think that's true.  The mFirstChild / mNext linked list is of child
> style sheets, so one for each @import rule in a sheet.  They will use the
> same inner only if they refer to the same URL.

Thanks for setting me straight. Still learning this stuff.

Comment 6

[Nicholas Nethercote [inactive]]

> My understanding is that the sheets reachable by the mNext pointer all share
> the same inner with this. If that's true, then the patch does not change
> behavior, though I agree it reads better this way.

If that's true then we're counting that inner multiple times. Should we |break| after the first match?

Comment 7

[Nicholas Nethercote [inactive]]

Ugh, ignore comment 6.

> The mFirstChild / mNext linked list is of child
> style sheets, so one for each @import rule in a sheet.  They will use the
> same inner only if they refer to the same URL.

So is it possible that there will be at least some inner sharing on this list? If so, we'll be double-counting inners.

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> So is it possible that there will be at least some inner sharing on this
> list?

Yes.

> If so, we'll be double-counting inners.

No, since there will be at most one CSSStyleSheet that we encounter in this loop that will be in the inner's mSheet's last element position.

(There might no such CSSStyleSheet that we encounter in this loop, but that just means that the CSSStyleSheet we've chosen to apportion the inner's size to must have a different parent sheet, and so we'll encounter it in some other sheet's CSSStyleSheet::SizeOfIncludingThis call.)

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ca911e49616