1474883 - Memory leak due to animated frames

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[ce-ce-mel]

Attachment: memory-report.json.gz

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704003137

Steps to reproduce:

This happened on two different computers (both Firefox 61.0.1 + Windows 1803), using a new profile. It does not happen in safe mode.

Step to reproduce:

1. Visit https://www.proximus.be/en/id_cr_roaming/personal/products/mobilus/mobile-and-roaming-options/international-and-roaming/international-and-roaming-rates.html
2. Wait a few seconds without changing tab 

NB: change tab, close it or remove the animated block (the globe on the right side of the page) before it fills your RAM completely

NB2: there is a memory report attached. However, since defocusing the tab result of memory freeing, I do not know if it the file is relevant.


Actual results:

Memory usage keeps growing rapidly (in my case, 15 s was enough to allocate 12 GB)


Expected results:

A decent memory allocation

Comment 1

[David Olah]

I managed to reproduce this issue on Firefox 61.0.1, 62.0b7 and Nightly 63.0a1 (2018-07-15), Ubuntu 16.04, Max OS and Windows 10.

Comment 2

[Timothy Nikkel (:tnikkel)]

A tip for measuring image related memory with about:memory, open a new window for about:memory, that way the problematic tab can stay as a foreground tab and we shouldn't get rid of an image memory from it.

I suspect this is actually graphics. The animating thing on that page isn't an animated image. I am able to reproduce on Windows but not Mac (also points to graphics as almost everything imagelib is crossplatform). When I reproduce nothing much seems to show up in about memory (including images which are quite well measured by about:memory). Graphics drivers and such are less well measured by about:memory.

Comment 3

[Jamie Nicol [:jnicol]]

I cannot reproduce on Linux, but certainly can on Windows, either with or without advanced layers. Turning off acceleration altogether fixes it, which explains the reporter not experiencing it in safe mode.

Will dig deeper.

Comment 4

[Jamie Nicol [:jnicol]]

I've bisected this, and the bad change is a change I made - bug 1339578. This introduces a minimum size for creating active layers for animated transforms. Setting layers.min-active-layer-size = 0 in about:config stops the leak.

I do sometimes question whether that was a good change - like all layerization heuristics sometimes it's good and sometimes it's bad. But in any case - there is an underlying bug. We're leaking memory somewhere in the inactive code path.

Comment 5

[Jamie Nicol [:jnicol]]

Attachment: testcase.html

Each of the animated icons is actually a glyph rendered using a special font, with an animated transform. What's happening here is that with inactive layers we render them all in to a single active layer, which means there are only two d2d render targets (a backbuffer and frontbuffer) which live forever. Furthermore, as they are rasterized directly in to the page's main painted layer, they are rasterized with rotational transforms (which keep changing values).

This is causing D2D's glyph cache to grow very large, as it will have a different version of each glyph for each rotation.

When using active layers, the layers and therefore the render targets only live a short while. And more importantly, the glyphs are rasterized un-rotated in to their layer buffer, then composited with a rotation - so the glyph cache does not grow large.

Attached is a reduced test case which reproduces the bug.

Even with this explanation, however, it still surprises me quite how much memory it is using.

As for how to fix this: the glyph cache seems to be pruned when calling EndDraw/BeginDraw. We do this in DrawTargetD2D1::FlushInternal. We deliberately avoid doing this as much as possible, however, as it is costly. So we should do it every so often when rendering lots of glpyhs, but not too often.

Comment 6

[Jamie Nicol [:jnicol]]

Attachment: Bug 1474883 - Ensure D2D glyph cache is pruned after rendering 1000 transformed glyphs.

Rendering glyphs at many different rotations was causing the D2D glyph
cache to grow very large. Calling EndDraw/BeginDraw will prune the
cache, but is costly, so only do it for every 1000 glyphs.

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

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 8995969 [details]
Bug 1474883 - Ensure D2D glyph cache is pruned after rendering 1000 transformed glyphs.

https://reviewboard.mozilla.org/r/260258/#review267298

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f86485d19c69
Ensure D2D glyph cache is pruned after rendering 1000 transformed glyphs. r=bas

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f86485d19c69

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This sounds bad. Please nominate for Beta & ESR60 approval when you get a chance.

Comment 11

[Jamie Nicol [:jnicol]]

Comment on attachment 8995969 [details]
Bug 1474883 - Ensure D2D glyph cache is pruned after rendering 1000 transformed glyphs.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Very bad memory leak.
User impact if declined: On affected websites firefox can begin to use gigabytes of memory per a few seconds and keeps increasing until the system becomes unresponsive or it crashes. Not sure how frequently affected websites would occur.
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): Could theoretically have performance impact. Though I didn't notice on talos or my own profiles.
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: Very long-standing issue, uncovered by bug 1339578 (which was still over a year ago)
[User impact if declined]: On affected websites firefox can begin to use gigabytes of memory per a few seconds and keeps increasing until the system becomes unresponsive or it crashes. Not sure how frequently affected websites would occur.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Don't think so.
[Why is the change risky/not risky?]: Uses an existing code path to be more conservative with memory usage. Minor performance risk.
[String changes made/needed]: N/A

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8995969 [details]
Bug 1474883 - Ensure D2D glyph cache is pruned after rendering 1000 transformed glyphs.

Fixes a massive memory leak for Windows users. Approved for 62.0b16 and ESR 60.2.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a376138dcac6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/151d3c2ef92d