1742051 - memory leak with chrome://browser/skin/tabbrowser/loading-burst.svg when repeatly open and close same single page

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[dickeylth]

Attachment: memory snapshots with init(step 1) and end(step 4)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/95.0.4638.69 Safari/537.36

Steps to reproduce:

1. Launch firefox app; open Activity Monitor on macOS and filter firefox process memory usage;
2. open new tab, goto https://google.com;
3. when the page loads complete, close the page;
4. repeat step 2-3 for 15+ times, and watch the memory consumption of firefox

• system: macOS Big Sur(11.4)
• firefox version: tested with firefox release 94 and firefox nightly 96, both reproducible.

Related github issue for details: https://github.com/puppeteer/puppeteer/issues/7771

Actual results:

The memory usage by the firefox main process is increasing gradually from 300MB+ to 700MB.

Expected results:

The memory consumption should kept stable instead of increase.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Diffing both the initial and the end memory reports I can see quite a lot of memory (325MB) being hold by chrome://browser/skin/tabbrowser/loading-burst.svg.

340,844,544 B (100.0%) -- explicit
├──325,558,080 B (95.52%) -- images
│  ├──325,242,224 B (95.42%) -- chrome/vector/used/progress=18f
│  │  ├──325,259,312 B (95.43%) -- image(0x0, chrome://browser/skin/tabbrowser/loading-burst.svg)
│  │  │  ├──324,239,408 B (95.13%) -- unlocked/types=400
│  │  │  │  ├────3,035,312 B (00.89%) -- surface(870x871, svgContext:[ viewport=(11x11) contextPaint=( fill=ffff840a ) ])
│  │  │  │  │    ├──3,035,136 B (00.89%) ── decoded-nonheap [+]
│  │  │  │  │    └────────176 B (00.00%) ── decoded-heap [+]
│  │  │  │  ├────3,018,928 B (00.89%) -- surface(868x869, svgContext:[ viewport=(11x11) contextPaint=( fill=ffff840a ) ])
│  │  │  │  │    ├──3,018,752 B (00.89%) ── decoded-nonheap [+]
│  │  │  │  │    └────────176 B (00.00%) ── decoded-heap [+]
│  │  │  │  ├────3,010,736 B (00.88%) -- surface(868x867, svgContext:[ viewport=(11x11) contextPaint=( fill=ffff840a ) ])
│  │  │  │  │    ├──3,010,560 B (00.88%) ── decoded-nonheap [+]
│  │  │  │  │    └────────176 B (00.00%) ── decoded-heap [+]
│  │  │  │  ├────2,965,680 B (00.87%) -- surface(860x861, svgContext:[ viewport=(11x11) contextPaint=( fill=ffff840a ) ])
│  │  │  │  │    ├──2,965,504 B (00.87%) ── decoded-nonheap [+]
│  │  │  │  │    └────────176 B (00.00%) ── decoded-heap [+] 

That's an SVG as used in the tabbrowser at this location, and I wonder how/if the comment above the code block still applies given that we still seem to hit a leak here:

https://searchfox.org/mozilla-central/rev/0f3e259c24e52932387318ac503bfad3c82baa44/browser/base/content/tabbrowser-tab.js#7-8,15

Comment 2

[Julian Descottes [:jdescottes]]

This SVG is also mentioned in the following bug about memory usage: https://bugzilla.mozilla.org/show_bug.cgi?id=1395908

Comment 3

[:Gijs (he/him)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #1)

That's an SVG as used in the tabbrowser at this location, and I wonder how/if the comment above the code block still applies given that we still seem to hit a leak here:

That comment isn't about this kind of leak, it's about "leaking" variables into the global scope of the browser window, which is why it's put things in a code block. It has nothing to do with memory leaks.

Comment 4

[:Gijs (he/him)]

Reading all the comments in bug 1395908 I think this is just a dupe.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I also read all the comments but I didn't get the feeling it's a dupe. Discussions were all around the high memory usage of this specific SVG image. If a solution might be found in the future and the memory usage will decrease we would still have this leak, right?

Comment 6

[:Gijs (he/him)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #5)

I also read all the comments but I didn't get the feeling it's a dupe. Discussions were all around the high memory usage of this specific SVG image. If a solution might be found in the future and the memory usage will decrease we would still have this leak, right?

It's not really a leak, it'd be a leak if the surfaces were never again used/usable, and in bug 1395908 comment 9, jwatt explicitly addressed why they are not released.

Either way it's fundamentally an issue of "we use a lot of memory for these image surfaces", it doesn't really make sense to have more than one bug on file describing exactly the same thing.

Comment 7

[:Gijs (he/him)]

Hm, no, maybe I'm wrong if the same surface for the same image is now stored multiple times.

In that case, we could really do with a regression window.

Comment 8

[Daniel Holbert [:dholbert]]

I noticed one interesting thing in comment 1 (as well as in my own measurement when I repro'd this locally): it looks like we've cached this surface at a wide variety of sizes, for some reason. Quoting comment 1:

│  │  │  │  ├────3,035,312 B (00.89%) -- surface(870x871,
│  │  │  │  ├────3,018,928 B (00.89%) -- surface(868x869
│  │  │  │  ├────3,010,736 B (00.88%) -- surface(868x867
│  │  │  │  ├────2,965,680 B (00.87%) -- surface(860x861

I think those WxY values (870x871 etc) are sizes at which we have been asked to render this SVG image, and we're just caching the rasterization.

So the question is, why are we being asked to render it at a bunch of varying sizes?

Comment 9

[Daniel Holbert [:dholbert]]

Aha, we're being asked to render it at a bunch of varying sizes because it has a "scale-up" animation applied to the element that uses this SVG as a background image:
https://searchfox.org/mozilla-central/source/browser/themes/shared/tabs.inc.css#166

So: the handful of cached varying-size surfaces here are just whatever sizes we happen to have been asked to render that element at, during its quick scale-up animation. And they're cached so that we can paint them faster if we're asked to render them again. (Unfortunately, given the arbitrary nature of animation time sampling, it's unlikely we actually will get asked to render them again, so we're caching them for nothing, kinda.)

The good news is that they're cached in an explicit cache, so it's not like they're explicitly leaked. There's a bound to how many we'll save, and we'll eventually evict entries that aren't used (when we run up against the cache limits), etc.

Comment 10

[Daniel Holbert [:dholbert]]

It looks like our "factor-of-2-mode" caching/surface-size-selection heuristic could theoretically help cut down on the amount of surfaces that we'd be storing here. (That was implemented in bug 1370412, to eventually only pick surface-sizes that are a factor of 2, or something to that effect; though I think that heuristic is only for raster images, based on the comments on that bug.)

In bug 1370412 comment 40, aosmond posted some thoughts applying this sort of heuristic (or others) to SVGs, and mentioned he'd like to see bugs where it would matter. This bug here seems to be such a bug. :) aosmond, do you have suggestions/thoughts for how we could improve our unfortunate caching behavior here, with SVG being rendered at arbitrary sizes from a scale-up animation?

(Alternately, it's possible there's a better way to implement this "burst" effect, such that we're not rendering an image at a bunch of arbitrary interpolated scaled-up sizes. But nonetheless this is probably something that "real" web content does as well -- it's probably not just our tab-strip -- so it would be nice to make our caching behavior have an answer for this scenario that doesn't involve just indefinite-cache-growth.)

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

This should have been implemented as part of bug 1456558 for SVG images. It is possible it has regressed.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

My expectation here is that we should have entered factor of 2 mode, since none of these surfaces are marked with cannot_substitute in the memory report (that would mean either sync decoding was requested or high quality scaling was disabled). So clearly there is a flaw here.

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

There is a blind spot in the implementation. Namely:

https://searchfox.org/mozilla-central/rev/4ca2f73ae9346709d39de2c8fe33874e4203b9e6/image/SurfaceCache.cpp#483

If we don't have some sort of size as a baseline to guide us for the scaling, we won't enter factor-of-2 mode. We could/should use GetIntrinsicRatio as a fallback, and barring that, perhaps generate our own heuristic (if > 50% of the images have roughly the same aspect ratio, consider that as our factor-of-2 ratio?).

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1742051 - Allow factor-of-2 scaling for vector images without intrinsic size.

If a vector image does not have an intrinsic size as returned by
VectorImage::GetWidth and VectorImage::GetHeight, currently factor-of-2
scaling is disabled. With this patch, we just assume a default size of
100x100, adjusted by the intrinsic ratio if available, to get the
baseline size for determing appropriate factor-of-2 sizes.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/653b181d3cef
Allow factor-of-2 scaling for vector images without intrinsic size. r=tnikkel

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/653b181d3cef

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

dickeylth, would you mind checking with a recent Nightly if the leak is now gone for you case? Thanks!

Comment 18

[Daniel Holbert [:dholbert]]

I'm hoping this is indeed fixed -- at the very least, we do seem to be better at handling loading-burst.svg as discussed here. Here's what I see in about:memory after performing a slightly abbreviated versions of the STR (with not-quite-15+ newtabs because I'm lazy):

├───15.94 MB (08.15%) -- images
│   ├──15.32 MB (07.83%) -- chrome
│   │  ├──15.31 MB (07.83%) -- vector/used/progress=18f
│   │  │  ├──13.10 MB (06.70%) -- image(0x0, chrome://browser/skin/tabbrowser/loading-burst.svg)
│   │  │  │  ├──12.82 MB (06.56%) -- locked/factor2/types=400
│   │  │  │  │  ├───9.77 MB (04.99%) -- surface(1600x1600, svgContext:[ viewport=(11x11) contextPaint=( fill=ffff840a ) ])
│   │  │  │  │  │   ├──9.77 MB (04.99%) ── decoded-nonheap
│   │  │  │  │  │   └──0.00 MB (00.00%) ── decoded-heap
│   │  │  │  │  ├───2.44 MB (01.25%) -- surface(800x800, svgContext:[ viewport=(11x11) contextPaint=( fill=ffff840a ) ])
│   │  │  │  │  │   ├──2.44 MB (01.25%) ── decoded-nonheap
│   │  │  │  │  │   └──0.00 MB (00.00%) ── decoded-heap
│   │  │  │  │  └───0.62 MB (00.32%) -- (2 tiny)
│   │  │  │  │      ├──0.61 MB (00.31%) ++ surface(400x400, svgContext:[ viewport=(11x11) contextPaint=( fill=ffff840a ) ])
│   │  │  │  │      └──0.00 MB (00.00%) ++ surface(25x25, svgContext:[ viewport=(11x11) contextPaint=( fill=ffff840a ) ])
│   │  │  │  └───0.27 MB (00.14%) -- (2 tiny)
│   │  │  │      ├──0.23 MB (00.12%) ++ unlocked/types=400
│   │  │  │      └──0.04 MB (00.02%) ── source

Looks like we're caching the loading-burst.svg rasterization at a small handful of sizes, all of which are 25x25 times two to some power. (We have 25x25, 400x400, 800x800, and 1600x1600.) So, the factor-of-two stuff seems to be working for this image now (compared to e.g. comment 8 where we had a bunch of similar sizes cached).

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hm, maybe a dumb question but why do we need image sizes of 1600x1600 and 800x800 for this case when we have a background image that only has to fit into the tab stack?

Comment 20

[:Gijs (he/him)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #19)

Hm, maybe a dumb question but why do we need image sizes of 1600x1600 and 800x800 for this case when we have a background image that only has to fit into the tab stack?

This was already discussed in the other bug, see e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1395908#c4

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #19)

Hm, maybe a dumb question but why do we need image sizes of 1600x1600 and 800x800 for this case when we have a background image that only has to fit into the tab stack?

Another response to this (distinct from comment 20):
(1) the image actually has to be bigger than the tab-title -- on my system, it looks like we play this growing-circle-animation with the circle centered near the left edge of the tab. So when the animation completes (with the circle fully covering up the tab), I would bet the image is actually being drawn at ~2x the tab width, i.e. with the circle's radius approximately equal to the tab width.

(2) My Firefox tabs are about 450 display pixels wide.*

(3) So: based on (1) and (2), we probably end up being asked to draw the image at a size of around 900x900 (or perhaps even a bit larger) at the end of the animation. Given this and our imagelib power-of-2 surface-sizing behavior, it makes sense that we would draw using 800x800 and then 1600x1600 rasterized surfaces (with 1600x1600 being the closest power-of-2 that we get when rounding up the ~900-display-pixel target size).

• Note that I have 200% pixel-scaling enabled in my OS -- so our tab-titles might really be 225 CSS pixels (or in that neighborhood), but they draw at ~450 display pixels; and the display pixels are what matter for image rasterization purposes.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

As an aside from the imagelib perspective, we may be able to improve upon this when we switch to blob recordings for SVG images. That will enable partial rasterization for the cases where only part of the image is in view.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Is this something we want to nominate for uplift?

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

I'm content to let it ride the trains. It would be good to make sure this doesn't have any unintended consequences.

Comment 25

[dickeylth]

Sorry for the delay, I've retried the issue with Firefox Nightly and the issue seems still happened:

https://github.com/puppeteer/puppeteer/issues/7771#issuecomment-1001478775

Related profile attachments are here: https://github.com/puppeteer/puppeteer/issues/7771#issuecomment-1001481365

Comment 26

[Andrew Osmond [:aosmond] (he/him)]

From the memory reports, I can only see this:

│ ├──0.38 MB (81.28%) -- image(480x16, chrome://browser/skin/tabbrowser/loading.svg)
│ │ ├──0.23 MB (50.27%) -- locked/types=400
│ │ │ ├──0.12 MB (25.14%) -- surface(960x32, svgContext:[ viewport=(480x16) contextPaint=( fill=fffefbfb ) ])
│ │ │ │ ├──0.12 MB (25.10%) ── decoded-nonheap
│ │ │ │ └──0.00 MB (00.04%) ── decoded-heap
│ │ │ └──0.12 MB (25.14%) -- surface(960x32, svgContext:[ viewport=(480x16) contextPaint=( fill=ffff840a ) ])
│ │ │ ├──0.12 MB (25.10%) ── decoded-nonheap
│ │ │ └──0.00 MB (00.04%) ── decoded-heap

They have different fill colours so it stores the same size twice since they are different.

Comment 27

[Catalin Sasca, Desktop QA [:csasca]]

Reproduced the issue on Firefox 96.0a1 under macOS 11.6.2 by following the STR from Comment 0.

The issue is fixed on Firefox 97.0b9 and 98.0a1 (2022-01-30) under the same system.