1407069 - Playing A YouTube Video With WebRender Enabled And Leaving It Alone Causes Shared Memory To Continuously Rise

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Keith Hizal]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171009100134

Steps to reproduce:

This is on openSUSE Tumbleweed with a Radeon RX460.
Enable WebRender and GPU Process.
Play a YouTube video.
Disable subtitles (and possibly annotations as well)
While the video is playing, do not interact with the video window in any way.


Actual results:

Shared Memory usage of the Web Process and the GPU process will climb continuously until the computer runs out of memory.

Strangely, the Shared Memory resets itself to normal if you interact with the YouTube video by either hovering over it with your mouse or enabling subtitles.


Expected results:

Shared Memory should not reach 4GBs and crash the web browser.

Comment 1

[Sotaro Ikeda [:sotaro]]

I take a look.

Comment 2

[Sotaro Ikeda [:sotaro]]

It is a fallout from pre AsyncImagePipelineManager era. On current gecko, each video pipe line has each pipeline id and epoch. We need to use them. But WebRenderImageHost::UseTextureHost() still use WebRenderBridgeParent's pipeline id and epoch.

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/wr/WebRenderImageHost.cpp#197

Comment 3

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> It is a fallout from pre AsyncImagePipelineManager era. On current gecko,
> each video pipe line has each pipeline id and epoch. We need to use them.
> But WebRenderImageHost::UseTextureHost() still use WebRenderBridgeParent's
> pipeline id and epoch.
> 
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/wr/
> WebRenderImageHost.cpp#197

It is a fallout, but it accidentally minimize the memory leak.

Actual source of memory leak is injected by Bug 1383786. It minimize updating DisplayList. The updating DisplayList updates also epoch. AsyncImagePipelineManager uses the epoch to recognize end of usage of WebRenderTextureHost by WebRender.

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> 
> Actual source of memory leak is injected by Bug 1383786. It minimize
> updating DisplayList. The updating DisplayList updates also epoch.
> AsyncImagePipelineManager uses the epoch to recognize end of usage of
> WebRenderTextureHost by WebRender.

If there is a update of WebRenderTextureHost usage in resourceUpdates, we need to push DisplayList with current gecko and WebRender.

Comment 5

[Sotaro Ikeda [:sotaro]]

Bug 1383786 also removed a case that ImageHost does not have TextureHost for composite. We also need to handle this case.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Make ApplyAsyncImages() as to push display list if WebRenderTextureHosts usages are updated

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Do not hold external image in WebRenderImageHost::SetCurrentTextureHost() if ImageHost is async

Comment 8

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa48a598dda9a905009ddcd69bc8f4a188490ff4

Comment 9

[Sotaro Ikeda [:sotaro]]

R4 failed because of TEST-UNEXPECTED-PASS.

REFTEST TEST-UNEXPECTED-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/bugs/579323-1.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/bugs/579323-1-ref.html | image comparison, max difference: 0,

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> R4 failed because of TEST-UNEXPECTED-PASS.
> 
> REFTEST TEST-UNEXPECTED-PASS |
> file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/
> bugs/579323-1.html ==
> file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/
> bugs/579323-1-ref.html | image comparison, max difference: 0,

The test was set failed by Bug 1403457 because of regression of bug 1383786.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Update reftest.list

Comment 12

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5a305e5ea4bef79d6d1e871124b9a0807bb5dc7

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 8917657 [details] [diff] [review]
patch part 1 - Make ApplyAsyncImages() as to push display list if WebRenderTextureHosts usages are updated

Review of attachment 8917657 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, this looks like a step in the wrong direction. If there is a bug in the way we manage the lifetime of external images, let's figure it out and fix it, but it is important to not rebuild scenes when we don't need to, especially during video playback.

Comment 14

[Sotaro Ikeda [:sotaro]]

I created the patch to fix the problem soon. We could address the lifetime of external images in a different bug.

Comment 15

[Nicolas Silva [:nical]]

I understand. I don't think we need to hurry about fixing this to the point of undoing something that we want. Let's use this bug to look for a long term solution (but we should definitely fix this soon-ish even if it takes a week or two).

Comment 16

[Darkspirit]

(In reply to Keith Hizal from comment #0)
> Linux x86_64; rv:58.0
> This is on openSUSE Tumbleweed with a Radeon RX460.
> Enable WebRender and GPU Process.

I have Nightly 58 x64 @ Debian Testing (KDE, Radeon RX480(!), same prefs) and saw similar, but not a problem with "shared memory", rather "memory"(attachment 8914537 [details]): I think(!) blob-images was the cause: bug 1405168. Since I disabled it I haven't had such a memory problem so far. I may be totally wrong and don't know what I'm talking about. (Unrelated: bug 1402067)

Comment 17

[Nicolas Silva [:nical]]

I filed https://github.com/servo/webrender/issues/1870 to address the missing webrender bits.

Comment 18

[Sotaro Ikeda [:sotaro]]

I created Bug 1408348 just to address lifetime handling of WebRenderTextureHost, I am not sure yet https://github.com/servo/webrender/issues/1870 could help well to the lifetime handling.

Comment 19

[Keith Hizal]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #16)
> I have Nightly 58 x64 @ Debian Testing (KDE, Radeon RX480(!), same prefs)
> and saw similar, but not a problem with "shared memory", rather
> "memory"(attachment 8914537 [details]): I think(!) blob-images was the
> cause: bug 1405168. Since I disabled it I haven't had such a memory problem
> so far. I may be totally wrong and don't know what I'm talking about.
> (Unrelated: bug 1402067)

I disabled blob images and still encountered the same problem as before.

Comment 20

[Nicolas Silva [:nical]]

Attachment: Associate epochs to frames with image updates

UpdatePipelineResources was temporarily added to webrender's API to address this use case (bumping the epoch when resources are updated with no new display list).

In the longer run we'll refactor webrender's API to more flexibly compose transactions and the new UpdatePipelineResources message will be removed, but in the mean time we can address this bug.

Comment 21

[Nicolas Silva [:nical]]

> I created Bug 1408348 just to address lifetime handling of WebRenderTextureHost, I am not sure yet https://github.com/servo/webrender/issues/1870 could help well to the lifetime handling.

Oops I missed that, the patch I just posted may not be up to date with your latest work, I'll have a look.

Comment 22

[Nicolas Silva [:nical]]

Attachment: Associate epochs to frames with image updates

Updated patch. I had forgotten to call HoldExternalImage when not going through the set_display_list path.

With this patch I have been able to confirm that all epochs are correctly reported back when updating images using the UpdatePipelineResources api.
However, the number of live TextureHosts is still growing to worryingly high levels (~600 just watching a video). It appears that the vast majority most of the deallocations are happening when the TextureParent is destroyed, which would indicate that a lot of the textures are held alive on the content side.
Looks like there is more to this issue than the compositor-side lifetime management.

Comment 23

[Nicolas Silva [:nical]]

Comment on attachment 8920125 [details] [diff] [review]
Associate epochs to frames with image updates

Review of attachment 8920125 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/AsyncImagePipelineManager.cpp
@@ +272,5 @@
>        // We don't need to update the display list, either because we can't or because
>        // the previous one is still up to date.
>        // We may, however, have updated some resources.
> +      mApi->UpdatePipelineResources(resourceUpdates, pipelineId, epoch);
> +      if (mCurrentTexture) {

Sorry there's a mistake here (should be pipeline->mCurrentTexture), it's fixed locally.

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #22)
> Created attachment 8920125 [details] [diff] [review]
> Associate epochs to frames with image updates
> 
> Updated patch. I had forgotten to call HoldExternalImage when not going
> through the set_display_list path.
> 
> With this patch I have been able to confirm that all epochs are correctly
> reported back when updating images using the UpdatePipelineResources api.
> However, the number of live TextureHosts is still growing to worryingly high
> levels (~600 just watching a video). It appears that the vast majority most
> of the deallocations are happening when the TextureParent is destroyed,
> which would indicate that a lot of the textures are held alive on the
> content side.

:nical, Isn't the problem be fixed with attachment 8917658 [details] [diff] [review]?

Comment 25

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8920125 [details] [diff] [review]
Associate epochs to frames with image updates

Review of attachment 8920125 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks!

Comment 26

[Sotaro Ikeda [:sotaro]]

Assigned to nical, since he created the patch.

Comment 27

[Nicolas Silva [:nical]]

> :nical, Isn't the problem be fixed with attachment 8917658 [details] [diff] [review] [diff] [review]?

Marvelous! it does fix the problem indeed \o/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73adbbe06358
Associate epochs to async image updates. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/647efba379ee
Do not hold external image in SetCurrentTextureHost if ImageHost is async. r=nical

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/73adbbe06358
https://hg.mozilla.org/mozilla-central/rev/647efba379ee