1875100 - RAM usage grows too much on minimized browser window with sidebar panel containing APNG background

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[YUKI "Piro" Hiroshi]

Attachment: Testcase XPI

Steps to reproduce

1. Run Firefox on Windows.
2. Go to about:debugging and load the attached testcase XPI as a temporary addon.
3. Ensure that the sidebar panel of the testcase addon is visible.
4. Go to about:processes and memorize the PID of the "Extensions" process.
5. Hit Ctrl-Shift-ESC to open Task Manager of Windows.
6. Switch to "Details" panel.
7. Right-click on the header row and turn the "Working Set" column visible.
8. See the process row of the PID memorized at the step 4.
9. Minimize the browser window.

Expected result

The working set of the process does not grow.

Actual result

The working set of the process starts growing.
It would stop when you restore the window from minimized state.

Environment

• Windows 11 23H2
• Nightly 123.0a1 (build ID: 20240116050321)

Notes

The animation background image of the testcase is cited from the theme:
https://addons.mozilla.org/en-US/firefox/addon/a-n-i-m-a-t-e-d-kitty-cat/
I tried to create new APNG image to reproduce but I couldn't, so I just copied the file.

Here is the original bug report about this problem: https://github.com/piroor/treestyletab/issues/3413

Comment 1

[Timothy Nikkel (:tnikkel)]

Thanks for reporting. This sounds similar to an issue we were dealing with last year, perhaps there is another case that wasn't fixed.

Comment 2

[Timothy Nikkel (:tnikkel)]

I can reproduce on macOS.

Comment 3

[Timothy Nikkel (:tnikkel)]

The animated image is in a remote iframe uri moz-extension://6c35841f-7382-4b8f-9311-f838e4f0bc24/sidebar.html and it is in a content process. So I would guess that it's compositor does not get paused (like content tabs) but its refresh driver does not get throttled (unlike content tabs) so both fixes we did for this problem miss this case.

Comment 4

[Timothy Nikkel (:tnikkel)]

Options to fix:

1. find some property in this process that mirrors the paused compositor from the parent
2. create some property in this process that mirrors the paused compositor
3. fix bug 1819154

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Something like bug 1847600 but for the regular sidebar rather than the shopping sidebar would do too afaict.

Comment 6

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

Something like bug 1847600 but for the regular sidebar rather than the shopping sidebar would do too afaict.

I'm a bit lost - it looks to me like the issue here is a minimized parent window not causing stuff in child windows to pause - do we think it ought to be the responsibility of frontend code to do work there to pass along the "hey the parent widget window got minimized" state for all the remote children? I would have expected the graphics/core code to do this itself...

Comment 7

[Timothy Nikkel (:tnikkel)]

Feels like it would be more fool proof to have layout/gfx/core code do it to me, but I'm open to other thoughts/info.

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to YUKI "Piro" Hiroshi from comment #0)

I tried to create new APNG image to reproduce but I couldn't, so I just copied the file.

Probably because the animated images needs to be above a certain total size so that we only keep around a subset of the image frames instead of all of them to trigger this bug.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Timothy Nikkel (:tnikkel) from comment #7)

Feels like it would be more fool proof to have layout/gfx/core code do it to me, but I'm open to other thoughts/info.

We don't do it super-generally because the tab switcher relies on manually managing activeness, but we could find a way of doing this like bug 1847584 or so...

Comment 10

[Timothy Nikkel (:tnikkel)]

Sure for activeness, but it seems like it shouldn't be hard for the content process to know if it's associated compositor is paused (which is the thing more directly causing this bug, throttled refresh drivers/activeness are related but not the cause, a throttled refresh driver whose compositor is not paused won't experience this bug AFAIK), but I don't know those pipes very well.

Failing that maybe we could add another patch to SharedSurfacesAnimation::SetCurrentFrame to skip sending the resource update if nsRefreshDriver::IsWaitingForPaint.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1875100 - Propagate top level activeness automatically to top descendants. r=nika!

For that, opt in tabbrowser and the shopping sidebar to manual
activeness management.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Can you confirm that D198942 fixes the issue for you? Top level window activeness is always "is the compositor paused", so it should.

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

Can you confirm that D198942 fixes the issue for you? Top level window activeness is always "is the compositor paused", so it should.

Sorry, it did not fix the issue for me.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Okay, latest version fixes the issue for me on Windows (I can't see the issue on Linux because we don't get vsync for the minimized window at all afaict). Can you check on mac?

Comment 15

[Timothy Nikkel (:tnikkel)]

Latest version fixes the problem for me on mac.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1875100 - Make gamepad mochitests don't rely on creating an impossible situation. r=nika,cmartin

I want to restrict a bit activeness setting (in particular, making sure
that this is only explicitly set for top levels, and in the parent
process).

This holds, in practice, except for these tests.

Ideally these tests should be ported to browser mochitests or so, but
that seems like a fair amount of work the way these are set up, and
these tests create an otherwise impossible situation that I want to
prevent (tab being active, one child iframe being active, and another
inactive).

I filed bug 1876117 for this.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7dced60d446e
Make gamepad mochitests don't rely on creating an impossible situation. r=cmartin

Comment 18

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/7dced60d446e

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ecd780688279
Propagate top level activeness automatically to top descendants. r=nika,tabbrowser-reviewers,mconley,extension-reviewers,robwu,geckoview-reviewers,owlish

Comment 20

[Cristian Tuns]

Backed out for causing multiple failures in test_suspend_media_by_inactive_docshell.html

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | dom/media/test/test_suspend_media_by_inactive_docshell.html | Test timed out. -

And possibly this: https://treeherder.mozilla.org/logviewer?job_id=446271413&repo=autoland

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22c6ddd752f4
Propagate top level activeness automatically to top descendants. r=nika,tabbrowser-reviewers,mconley,extension-reviewers,robwu,geckoview-reviewers,owlish,kaya

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/039742954901
Trivial lint fix.
https://hg.mozilla.org/integration/autoland/rev/f3d1619b2e33
Don't call window.close() from a crashtest to fix some orange.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox123 to wontfix.

For more information, please visit BugBot documentation.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e792abe83b59
Fix a windows crashtest timeout.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

The patch that landed is test-only, but this is complex enough to be risky to uplift.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87220c621617
Hopefully fix windows / androids crashtest timeouts for good.

Comment 27

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/22c6ddd752f4
https://hg.mozilla.org/mozilla-central/rev/039742954901
https://hg.mozilla.org/mozilla-central/rev/f3d1619b2e33
https://hg.mozilla.org/mozilla-central/rev/e792abe83b59
https://hg.mozilla.org/mozilla-central/rev/87220c621617

Comment 28

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #3)

The animated image is in a remote iframe uri moz-extension://6c35841f-7382-4b8f-9311-f838e4f0bc24/sidebar.html and it is in a content process. So I would guess that it's compositor does not get paused (like content tabs) but its refresh driver does not get throttled (unlike content tabs) so both fixes we did for this problem miss this case.

Do you know how I can create an iframe like this in a browser chrome test? I have a browser chrome test for the other two cases that we fixed, but I'm not sure how to create an iframe to reproduce this in a test.

Comment 29

[Emilio Cobos Álvarez (:emilio)]

If you have an actual browser test, probably something like this?