Closed
Bug 1493080
Opened 6 years ago
Closed 6 years ago
Memory leak when using videos with transparency as materials with webGL
Categories
(Core :: Graphics, defect, P3)
Tracking
()
VERIFIED
FIXED
mozilla65
People
(Reporter: dwastberg, Assigned: lsalzman)
References
Details
(Keywords: memory-footprint, regression, reproducible, Whiteboard: [MemShrink:P2])
Attachments
(3 files)
(deleted),
application/x-gzip
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
jrmuizel
:
review+
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180830143136
Steps to reproduce:
Visit https://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Hello%20World.html&label=Showcases&gist=88a3a9c6af80ecc95e1b66e32bbf4478
using firefox 62.0 and Windows 7 and leave it running for a few minutes.
Actual results:
After leaving the site open for 5 minutes Task Manager reports Firefox using over 8GB of memory. I was unable to reproduce the bug in Windows 10, and it only happens if the video material uses transparency.
Expected results:
Memory usage should stay roughly constant.
Someone on the Cesium project pointed out that it might be a similar bug to this one found in Chromium a while back: https://bugs.chromium.org/p/chromium/issues/detail?id=634012
Comment 2•6 years ago
|
||
I can reproduce the memory usage on Windows10 Nightly64.04.
Windows10 Task Manager shows almost 100%/8GB memory use, but Nightly shows only 350MB. I do not know why?
Status: UNCONFIRMED → NEW
status-firefox63:
--- → affected
status-firefox64:
--- → affected
Component: Untriaged → Graphics
Ever confirmed: true
Keywords: memory-footprint,
reproducible
Product: Firefox → Core
Comment 3•6 years ago
|
||
Comment 4•6 years ago
|
||
Updated•6 years ago
|
Component: Graphics → Canvas: WebGL
Updated•6 years ago
|
Whiteboard: [MemShrink]
Comment 5•6 years ago
|
||
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=286197fbaa34a127416e50970926ae18fb66d926&tochange=286203c73311ec77e93adfaddc6e7e0cc770fbd4
Regressed by:
286203c73311 Bas Schouten — Bug 1416862: Reverse DrawTargetSkia snapshot ownership model r=dvander
62f80084da60 Bas Schouten — Bug 1423281: Store the userdata for freeing our memory on the longer living snapshot. r=dvander
@:bas.schouten,
Your bunch of patch seems to cause thr memory problem. Could you please look into this?
Blocks: 1423281
status-firefox62:
--- → affected
status-firefox-esr60:
--- → affected
Component: Canvas: WebGL → Graphics
Flags: needinfo?(bas)
Keywords: regression
Version: 62 Branch → 58 Branch
Updated•6 years ago
|
Priority: -- → P3
Updated•6 years ago
|
Updated•6 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Too late for 63 but we could still take a patch for 65.
Comment 7•6 years ago
|
||
(In reply to Alice0775 White from comment #5)
> Regression window:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=286197fbaa34a127416e50970926ae18fb66d926&tochange=2862
> 03c73311ec77e93adfaddc6e7e0cc770fbd4
>
> Regressed by:
> 286203c73311 Bas Schouten — Bug 1416862: Reverse DrawTargetSkia snapshot
> ownership model r=dvander
> 62f80084da60 Bas Schouten — Bug 1423281: Store the userdata for freeing our
> memory on the longer living snapshot. r=dvander
>
> @:bas.schouten,
> Your bunch of patch seems to cause thr memory problem. Could you please look
> into this?
These patches solves a UAF/security issue. They cause our data to be correctly deallocated. If this causes other components to use more memory it means they basically are not freeing their snapshots as soon as they could presumably. So that sounds like it would be a WebGL issue. (Or possibly something about the page)
In any case this memory use should go away when the page is closed so it isn't technically a 'leak'.
Flags: needinfo?(bas)
Comment 9•6 years ago
|
||
(In reply to Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #8)
> Jeff, could you look in to this?
I don't have bandwidth.
Flags: needinfo?(jgilbert) → needinfo?(dbolter)
Comment 10•6 years ago
|
||
Lee do you have any bandwidth to look at this?
Flags: needinfo?(dbolter) → needinfo?(lsalzman)
Assignee | ||
Comment 11•6 years ago
|
||
A snapshot of the DrawTarget owned by BufferTextureData is loaned out. This snapshot keeps a reference to the owning TextureClient in user data. And that TextureClient in turn points to BufferTextureData.
So if DrawTarget now points to the SourceSurface snapshot, like after Bas' patch, we have a reference cycle.
BufferTextureData does not really need to point to the DrawTarget at all, and it's just a wrapper around existing data, so there is not much harm just getting rid of the pointer and reconstructing it on demand.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Flags: needinfo?(lsalzman)
Attachment #9024871 -
Flags: review?(jmuizelaar)
Updated•6 years ago
|
Attachment #9024871 -
Flags: review?(jmuizelaar) → review+
Comment 12•6 years ago
|
||
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/219b5cd5af70
remove reference cycle between BufferTextureData and DrawTargets. r=jrmuizel
Assignee | ||
Comment 13•6 years ago
|
||
Alice, can you check if the patch on inbound fixes the issue for you?
Flags: needinfo?(alice0775)
Comment 14•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 15•6 years ago
|
||
(In reply to Lee Salzman [:lsalzman] from comment #13)
> Alice, can you check if the patch on inbound fixes the issue for you?
Fix range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=56814f076c2be5b23f8e8fa6f9e8ece64914a201&tochange=219b5cd5af707408a79c0a5b036212f35519a0c9
Indeed, I can confirm that the patch on inbound fixes the issue.
Flags: needinfo?(alice0775)
Comment 16•6 years ago
|
||
Is this worth uplifting to beta?
Comment 17•6 years ago
|
||
This seems pretty bad and grafts cleanly to Beta & ESR60 as-landed. Please nominate for approval when you get a chance.
Flags: qe-verify+
Flags: needinfo?(lsalzman)
Assignee | ||
Comment 18•6 years ago
|
||
Comment on attachment 9024871 [details] [diff] [review]
remove reference cycle between BufferTextureData and DrawTargets
[Beta/Release Uplift Approval Request]
Feature/Bug causing the regression: Bug 1416862
User impact if declined: Shared memory leaks during video playback.
Is this code covered by automated tests?: Yes
Has the fix been verified in Nightly?: Yes
Needs manual test from QE?: No
If yes, steps to reproduce:
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Just removes an unnecessary reference that was causing a reference cycle, leading to the leak.
String changes made/needed:
[ESR Uplift Approval Request]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Potentially severe memory leaks during video playback on all platforms.
User impact if declined: Shared memory leaks during video playback.
Fix Landed on Version: 65
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Just removes an unnecessary reference that was causing a reference cycle, leading to the leak.
String or UUID changes made by this patch:
Flags: needinfo?(lsalzman)
Attachment #9024871 -
Flags: approval-mozilla-esr60?
Attachment #9024871 -
Flags: approval-mozilla-beta?
Comment 19•6 years ago
|
||
I have managed to reproduce this issue on an affected Firefox 64.0a1 (20180920220102) build using Windows 7 x64 by following the STR from comment 0.
This issue is verified fixed using Firefox 65.0a1 (20181116100115) on Windows 7 x64
tracking-firefox-esr60:
--- → 64+
Comment on attachment 9024871 [details] [diff] [review]
remove reference cycle between BufferTextureData and DrawTargets
Fix for memory leak, verified in nightly, let's uplift to beta and esr60.
Attachment #9024871 -
Flags: approval-mozilla-esr60?
Attachment #9024871 -
Flags: approval-mozilla-esr60+
Attachment #9024871 -
Flags: approval-mozilla-beta?
Attachment #9024871 -
Flags: approval-mozilla-beta+
Comment 21•6 years ago
|
||
bugherder uplift |
Comment 22•6 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Flags: qe-verify+
Comment 23•6 years ago
|
||
The issue is verified fixed on Firefox Beta 64.0b11 and latest esr60 build from treeherder (8d50553b267a8c38d2fd1c0a38c14bd1b32e) following STR from Comment 0. Tests were performed on Windows 7 (x64).
You need to log in
before you can comment on or make changes to this bug.
Description
•