1655580 - Fenix leaks 7MB (heap-unclassified) every time the browser app is foregrounded

Status: VERIFIED FIXED

(GeckoView :: General, defect, P1)

Description

[Markus Stange [:mstange]]

From bug 1625499 comment 8 and 9:

(In reply to Kevin Brosnan [:kbrosnan] from bug 1625499 comment #8)

hwinnemoe Fenix #11284

I did some measurements via about:memory using the following STR:

1. Cold startup Fenix, no Addons activated
2. Switch to PBM
3. Open about:memory
4. Press Measure and copy the log
5. Repeatedly switch between other apps and Fenix, e.g. switching 20 times between Settings and Fenix, thus switching to Fenix 10 times
6. Press Measure and copy the log
7. Use Close private tabs to close the single tab
8. Open about:memory again
9. Press Measure and copy the log

Here are the logs for

• STR Step 4: 11284 STR Step 4.txt
• STR Step 6: 11284 STR Step 6.txt
• STR Step 9: 11284 STR Step 9.txt

(In reply to Henrik Winnemöller from bug 1625499 comment #9)

I did some follow-up investigations with multiple tabs on this issue based on my findings in Fenix #11284. The used Nightly version was Nightly 200723 06:01 (Build #22050610). The device was a Sony Xperia Z2, Android 6.0.1, without active WR.

STR Variant 1 - Single tab for comparison

1. Cold startup the browser on a fresh profile
2. Open about:memory
3. Press Measure and note down the heap-unclassified memory
4. Repeatedly switch between the browser and another app, thus opening up the browser 10 times
5. Repeat steps 3 and 4.

Results for STR Variant 1 - Single tab for comparison
heap-unclassified after switching to the browser app for ...
10 times - 087.77 MiB
20 times - 165.02 MiB
30 times - 235.79 MiB

STR Variant 2 - Multiple tabs

1. Cold startup the browser on a fresh profile
2. Open two tabs for about:memory
3. Switch to the first tab.
4. Press Measure and note down the heap-unclassified memory
5. Repeatedly switch between the browser and another app, thus opening up the browser 10 times
6. Press Measure and note down the heap-unclassified memory
7. Switch to the second tab.
8. Repeat Steps 5 and 6.
9. Switch to the first tab.
10. Repeat Steps 5 and 6.
11. Switch to the second tab.
12. Repeat Steps 5 and 6.
13. Switch to the first tab.
14. Repeat Steps 5 and 6.
15. Switch to the first tab.
16. Repeat Steps 5 and 6.
17. Switch to the second tab.
18. Repeat Steps 5 and 6.

Results for STR Variant 2 - Multiple tabs
heap-unclassified after switching to the browser app for ... (Note: numbers are cumulative switches)
10 times on 1. tab - 087.97 MiB
10 times on 2. tab - 159.00 MiB
20 times on 1. tab - 236.55 MiB
20 times on 2. tab - 306.90 MiB
30 times on 1. tab - 370.16 MiB
30 times on 2. tab - 440.54 MiB

This seems to grow unrelated to the number of tabs. Note: Nightly users might not see this in this extent since they normally get daily updates. But a Beta or Production user will most likely experience this when using the browser for several days. It would be interesting to look at telemetry for how often users open up the browser (by switching to it or unlocking the screen) to judge the severity of this issue.

Comment 1

[Markus Stange [:mstange]]

This sounds rather serious. Switching apps is something that happens quite a lot of times over the course of a day, and mobile devices are very memory-constrained.

Comment 2

[Henrik Winnemöller]

I tried to reproduce this issue in GeckoView Example 81.0a1, Build ID 20200727203201 via this link: No heap-unclassified memory growth is shown after repeatedly foregrounding the app.

Comment 3

[Markus Stange [:mstange]]

Thanks! That indicates that Fenix is implicated here somehow; but about:memory only tracks Gecko memory usage, so it must be something Fenix triggers inside Gecko.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

My wild guess is that this is related to screenshots, I think we take one every time we show a page. Maybe related to Bug 1647797 ?

Comment 5

[Henrik Winnemöller]

Assuming that ScreenshotBuilder.capture() uses RGBA_8888, which I infer from this line, I can perform the following calculations: The device I used for the investigations in Comment 0 has a visible viewport in Fenix of 1080 by 1704 pixels. Thus, a RGBA_8888 uncompressed capture takes 7.02 MiB per capture, which seems to be the near the mean heap-unclassified memory growth per foregrounding action. Testing against devices with different resolutions could be helpful here to verify if the heap is actually filled with capture data.

In addition, my device takes about the same amount of time to present the tab in Fenix after foregrounding it as it takes to take a fullscreen capture.

Comment 6

[Markus Stange [:mstange]]

I think I found the bug: FlipScreenPixels creates and leaks a ScopedMap object which has a strong reference to the surface.

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1655580 - Stop leaking surfaces that were captured with RequestScreenPixels. r=agi

Comment 8

[Markus Stange [:mstange]]

I've confirmed that this patch fixes the issue as originally reported. heap-unclassified stayed at 24MB even after many iterations of app switching, whereas without the fix it was increasing by 7MB every time.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ba66dd08e1e
Stop leaking surfaces that were captured with RequestScreenPixels. r=geckoview-reviewers,agi

Comment 10

[Atila Butkovits]

Backed out as requested by mstange.

Backout link: https://hg.mozilla.org/integration/autoland/rev/00da6f158726e9842eb6d2f0b52af48bed72061b

Comment 11

[Markus Stange [:mstange]]

There was a lint failure - "Unused import - java.nio.ByteBuffer". I'm fixing that and waiting for a try push before relanding.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b7e82679e2a
Stop leaking surfaces that were captured with RequestScreenPixels. r=geckoview-reviewers,agi

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/3b7e82679e2a

Comment 14

[Henrik Winnemöller]

Verified as fixed in Fenix Nightly 200803 06:07 (Build #22160612).
Bad: Nightly 200730 06:21 (Build #22120630)
Good: Nightly 200803 06:07 (Build #22160612)

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 9167177 [details]
Bug 1655580 - Stop leaking surfaces that were captured with RequestScreenPixels. r=agi

Beta/Release Uplift Approval Request

• User impact if declined: Large memory leak in Fenix, causing the app to be terminated more frequently than necessary
• Is this code covered by automated tests?: No
• 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: Medium
• Why is the change risky/not risky? (and alternatives if risky): Low-to-medium, I'd say. Low because it's a small straightforward fix, medium because it's in a part of the code base that I'm not very familiar with so I might have made mistakes.
• String changes made/needed:

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 9167177 [details]
Bug 1655580 - Stop leaking surfaces that were captured with RequestScreenPixels. r=agi

fix memory leak in geckoview, approved for 80.0b4

Comment 17

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/49af6aad45d9

Comment 18

[Henrik Winnemöller]

Verified as fixed in Beta 80.0.0-beta.4 (Build #2015756489).
Bad: Beta 80.0.0-beta.3
Good: Beta 80.0.0-beta.4

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9167177 [details]
Bug 1655580 - Stop leaking surfaces that were captured with RequestScreenPixels. r=agi

Approved for Fenix 79.0.5.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/069237d871e2

Comment 23

[Henrik Winnemöller]

Verified as fixed in Fenix 79.0.5 (Build #2015758617)
Bad: Fenix 79.0.4
Good: Fenix 79.0.5