1493080 - Memory leak when using videos with transparency as materials with webGL

Status: VERIFIED FIXED

(Core :: Graphics, defect, P3)

Description

[dwastberg]

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.

Comment 1

[dwastberg]

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

[Alice0775 White]

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?

Comment 3

[Alice0775 White]

Attachment: memory-report.json.gz

Comment 4

[Alice0775 White]

Attachment: about:support

Comment 5

[Alice0775 White]

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?

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 63 but we could still take a patch for 65.

Comment 7

[Bas Schouten (:bas.schouten)]

(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'.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Jeff, could you look in to this?

Comment 9

[Kelsey Gilbert [: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.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Lee do you have any bandwidth to look at this?

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: remove reference cycle between BufferTextureData and DrawTargets

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/219b5cd5af70
remove reference cycle between BufferTextureData and DrawTargets. r=jrmuizel

Comment 13

[Lee Salzman [:lsalzman]]

Alice, can you check if the patch on inbound fixes the issue for you?

Comment 14

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/219b5cd5af70

Comment 15

[Alice0775 White]

(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.

Comment 16

[Julien Cristau [:jcristau]]

Is this worth uplifting to beta?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This seems pretty bad and grafts cleanly to Beta & ESR60 as-landed. Please nominate for approval when you get a chance.

Comment 18

[Lee Salzman [: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:

Comment 19

[Maria Berlinger [:mberlinger], Services QA]

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

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6cc73e9b5335

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/a7cabf306d05

Comment 23

[Catalin Sasca, Desktop QA [:csasca]]

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).