Closed
Bug 1474883
Opened 3 years ago
Closed 3 years ago
Memory leak due to animated frames
Categories
(Core :: Graphics: Layers, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox-esr60 | --- | fixed |
firefox61 | --- | wontfix |
firefox62 | --- | fixed |
firefox63 | --- | fixed |
People
(Reporter: ce-ce-mel, Assigned: jnicol)
References
()
Details
(Keywords: memory-leak, Whiteboard: [MemShrink:P1])
Attachments
(3 files)
144.04 KB,
application/gzip
|
Details | |
835 bytes,
text/plain
|
Details | |
59 bytes,
text/x-review-board-request
|
bas.schouten
:
review+
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr60+
|
Details |
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
Summary: Memory leak due to animated element → Memory leak due to animated frames
Keywords: memory-leak
Comment 1•3 years ago
|
||
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.
Status: UNCONFIRMED → NEW
status-firefox61:
--- → affected
status-firefox62:
--- → affected
status-firefox63:
--- → affected
Component: Untriaged → ImageLib
Ever confirmed: true
OS: Unspecified → All
Product: Firefox → Core
See Also: → 1303736
Version: 61 Branch → Trunk
Whiteboard: [MemShrink]
Comment 2•3 years ago
|
||
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.
Component: ImageLib → Graphics
Assignee | ||
Comment 3•3 years ago
|
||
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.
Assignee: nobody → jnicol
Component: Graphics → Graphics: Layers
OS: All → Windows
Priority: -- → P3
Assignee | ||
Comment 4•3 years ago
|
||
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.
Updated•3 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Assignee | ||
Comment 5•3 years ago
|
||
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 hidden (mozreview-request) |
Comment 7•3 years ago
|
||
mozreview-review |
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
Attachment #8995969 -
Flags: review?(bas) → review+
Pushed by jnicol@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f86485d19c69 Ensure D2D glyph cache is pruned after rendering 1000 transformed glyphs. r=bas
Comment 9•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f86485d19c69
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 10•3 years ago
|
||
This sounds bad. Please nominate for Beta & ESR60 approval when you get a chance.
Assignee | ||
Comment 11•3 years ago
|
||
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
Flags: needinfo?(jnicol)
Attachment #8995969 -
Flags: approval-mozilla-esr60?
Attachment #8995969 -
Flags: approval-mozilla-beta?
Comment 12•3 years ago
|
||
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.
Attachment #8995969 -
Flags: approval-mozilla-esr60?
Attachment #8995969 -
Flags: approval-mozilla-esr60+
Attachment #8995969 -
Flags: approval-mozilla-beta?
Attachment #8995969 -
Flags: approval-mozilla-beta+
Comment 13•3 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a376138dcac6
Comment 14•3 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/151d3c2ef92d
You need to log in
before you can comment on or make changes to this bug.
Description
•