1972255 - [V-T] Artificial testcase rapidly leads to rapid memory use and OOM/system hang

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mayank Bansal]

Attachment: sync VT.html

Comment 1

[Mayank Bansal]

Attachment: async VT.html

Comment 2

[Mayank Bansal]

Testcases generated using chatgpt. They lead to rapid memory increase on Firefox and eventual OOM. Works nicely on Chrome.
I am not 100% sure if the issue is specific to V-T, or is in general perf issue.

Comment 3

[Mayank Bansal]

Attachment: about:support

Comment 4

[Mayank Bansal]

Attachment: VT in shadow DOM - works great.html

This seems to work great - generates two animations on the page. Chrome only shows a single animation.
https://share.firefox.dev/3SVi7YS

Comment 5

[Mayank Bansal]

Attachment: VT in iframe - works great.html

Vt in iframes. Works great on Firefox.
https://share.firefox.dev/4lsor6z

Comment 6

[Mayank Bansal]

Attachment: VT - empty 1ms firing - oom.html

This leads to explosive OOM on Firefox. Chrome seems to be just fine

Comment 7

[Emilio Cobos Álvarez (:emilio)]

I can't seem to reproduce OOM in comment 6, even if I leave it running for a while. But I do reproduce other issues like the browser chrome flickering... That must be on the WR side, nical can you look at that?

Mayank, I assume you're on windows? Does the OOM reproduce with SW-WR? Do you have any other non-default settings?

Comment 8

[Mayank Bansal]

I use win11. See my about:support in comment 3.
I can repro on a new profile with both hw-wr and sw-wr.
To repro, you may have to click inside the tab or maybe wait for a few seconds.

Edit: Profile with memory tracking : https://share.firefox.dev/4naLgNJ

Comment 9

[Mayank Bansal]

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

I can't seem to reproduce OOM in comment 6, even if I leave it running for a while. But I do reproduce other issues like the browser chrome flickering... That must be on the WR side, nical can you look at that?

I see very bad flickering on Android.

Comment 10

[Nicolas Silva [:nical]]

With the OOM'ing test case we are continuously starting new transitions (skipping the previous one), and allocating new images for the old state which are rendered with the offscreen transaction but never deallocated unless switching to another tab (in which case they get all freed in one go). While the test is doing its thing we are never sending a non-offscreen transaction (only offscreen ones) which means we don't get to send the DeleteSnapshotImage commands and the deferred deletion list just keeps growing.

Since we are never pushing a real transaction (replaces references to pending images), we can't just send an IPC message to flush the pending deletion list. We either need to send a new display list, or keep track of what captured elements have been used by a display non-offscreen display list. Those that have not been used can be deleted right away when the transition is skipped (the could be put in a separate deletion list that offscreen transactions are allowed to clear).

Comment 11

[Nicolas Silva [:nical]]

Thinking about it more, this requires a pretty contrived situation: the page has no interactive elements and nothing changes so we don't build any transaction, and we continuously start transitions that don't change anything visually (otherwise it would also cause a transaction that would free the old image keys).

Perhaps we can detect that we have sent a number of offscreen transactions without sending a normal one, and skip transitions altogether (without ever starting it). I don't know if that's easy to express. In the mean time I'm exploring marking whether a snapshot key is used so that the ones that aren't can be destroyed more proactively.

Comment 12

[Nicolas Silva [:nical]]

I have a not pretty but not terrible fix for this which I can't submit right now because of some infra issues. I'll submit it early next week.

Comment 13

[Nicolas Silva [:nical]]

Attachment: Bug 1972255 - Allow discarding unused old state image keys in offscreen transactions. r=#view-transitions-reviewers

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6964584294f3
https://hg.mozilla.org/integration/autoland/rev/5791af6a7a13
Allow discarding unused old state image keys in offscreen transactions. r=view-transitions-reviewers,emilio

Comment 15

[pstanciu]

Backed out for causing BP bustages @ViewTransition.cpp

• https://hg.mozilla.org/integration/autoland/rev/83a3f034bb049b3145b9b659cbb30b54d838357e
• Push with failures
• Failure Log

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/74aef5e7346f
https://hg.mozilla.org/integration/autoland/rev/865eb11864fd
Allow discarding unused old state image keys in offscreen transactions. r=view-transitions-reviewers,emilio

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/865eb11864fd