Closed Bug 1493080 Opened 1 year ago Closed 1 year ago

Memory leak when using videos with transparency as materials with webGL

Categories

(Core :: Graphics, defect, P3)

58 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox-esr60 64+ verified
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- verified
firefox65 --- verified

People

(Reporter: dwastberg, Assigned: lsalzman)

References

Details

(Keywords: memory-footprint, regression, reproducible, Whiteboard: [MemShrink:P2])

Attachments

(3 files)

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
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
Component: Untriaged → Graphics
Ever confirmed: true
Product: Firefox → Core
Attached file memory-report.json.gz
Attached file about:support
Component: Graphics → Canvas: WebGL
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
Component: Canvas: WebGL → Graphics
Flags: needinfo?(bas)
Keywords: regression
Version: 62 Branch → 58 Branch
Priority: -- → P3
Whiteboard: [MemShrink] → [MemShrink:P2]
(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)
Jeff, could you look in to this?
Flags: needinfo?(jgilbert)
(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)
Lee do you have any bandwidth to look at this?
Flags: needinfo?(dbolter) → needinfo?(lsalzman)
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)
Attachment #9024871 - Flags: review?(jmuizelaar) → review+
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/219b5cd5af70
remove reference cycle between BufferTextureData and DrawTargets. r=jrmuizel
Alice, can you check if the patch on inbound fixes the issue for you?
Flags: needinfo?(alice0775)
https://hg.mozilla.org/mozilla-central/rev/219b5cd5af70
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
(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)
Is this worth uplifting to beta?
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)
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?
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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+
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.