1839250 - OutOfMemoryError at mozilla.components.feature.recentlyclosed.RecentlyClosedMiddleware$addTabsToStorage

Status: VERIFIED FIXED

(Firefox for Android :: Tabs, defect)

Description

[Jonathan Almeida [:jonalmeida]]

Steps to reproduce

1. Have some number of tabs (explicit state unknown)
2. Close a tab.

Expected behaviour

• Don't crash.

Actual behaviour

• Crash 💥.

Device information

• Firefox version: Firefox 114
• Android device model: Pixel 6
• Android OS version: Android 13

Any additional information?

32bc98a-72ad-41bb-beda-95c192658065

java.lang.OutOfMemoryError: Failed to allocate a 75497480 byte allocation with 67036424 free bytes and 63MB until OOM, target footprint 268435456, growth limit 268435456

----

java.lang.OutOfMemoryError: Failed to allocate a 75497480 byte allocation with 67036424 free bytes and 63MB until OOM, target footprint 268435456, growth limit 268435456
    at java.util.Arrays.copyOf(Arrays.java:3766)
    at java.lang.AbstractStringBuilder.ensureCapacityInternal(AbstractStringBuilder.java:125)
    at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:650)
    at java.lang.StringBuilder.append(StringBuilder.java:203)
    at org.json.JSONStringer.string(JSONStringer.java:354)
    at org.json.JSONStringer.value(JSONStringer.java:261)
    at org.json.JSONObject.writeTo(JSONObject.java:734)
    at org.json.JSONStringer.value(JSONStringer.java:246)
    at org.json.JSONArray.writeTo(JSONArray.java:616)
    at org.json.JSONStringer.value(JSONStringer.java:242)
    at org.json.JSONObject.writeTo(JSONObject.java:734)
    at org.json.JSONStringer.value(JSONStringer.java:246)
    at org.json.JSONObject.writeTo(JSONObject.java:734)
    at org.json.JSONObject.toString(JSONObject.java:702)
    at org.mozilla.geckoview.GeckoSession$SessionState.toString(GeckoSession.java:18)
    at java.lang.String.valueOf(String.java:3657)
    at mozilla.components.browser.engine.gecko.GeckoEngineSessionState.writeTo(GeckoEngineSessionState.kt:11)
    at mozilla.components.browser.session.storage.FileEngineSessionStateStorage.write(FileEngineSessionStateStorage.kt:37)
    at mozilla.components.feature.recentlyclosed.RecentlyClosedTabsStorage.addTabState$feature_recentlyclosed_release(RecentlyClosedTabsStorage.kt:90)
    at mozilla.components.feature.recentlyclosed.RecentlyClosedTabsStorage.addTabsToCollectionWithMax(RecentlyClosedTabsStorage.kt:99)
    at mozilla.components.feature.recentlyclosed.RecentlyClosedMiddleware$addTabsToStorage$1.invokeSuspend(RecentlyClosedMiddleware.kt:41)
    at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:9)
    at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:107)
    at kotlinx.coroutines.internal.LimitedDispatcher.run(LimitedDispatcher.kt:13)
    at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:3)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:79)
    Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@2cfb9be, Dispatchers.IO]

Comment 1

[Jonathan Almeida [:jonalmeida]]

This is what we learnt from our debugging today, is that we have had bugs like this before and they are significantly better than they are now with this patch, but users can still get into a perma-fail state:

1. The OOM error is happening when we're trying to write the GeckoSession.SessionState to disk in the FileEngineSessionStateStorage.
2. We're using the same writeTo API that causes the OOM as we do in the BrowserStateWriter which calls the same write action every 30 secs (at minimum), but that does not throw an error (the user reported 100% reproducible when closing a tab even after reboots) - we don't know why this is still.
3. We investigated different kinds of ways to grab information from the user that they can perform on their side, but our logging for user logs are quite bare in our logs.
   • We can't ask for a regular performance profile because the app crashes before we can grab one.
   • The profile information is bare for the information we want to grab.
4. We did notice something related - all our release applications are now profileable by default, but may not be something we are meant to have for release users. i.e. Profileable for benchmark profiles but not profileable for release users. While it's meant to be a low overhead change it might not be meant for product use (to be further investigated). The timing of that change landing is suspect when we see our application foreground process kills started spiking.

In the fourth point, this might be the reason for getting into an OOM, but doesn't explain the second point.

TL;DR, there is nothing useless we can ask them to do for us because the data we would get isn't enough to give us reasonable action. We can try to investigate/revert the profileable part in our manifest as a side-effect of this bug report.

Will still file a bug for this report since there is a useful stack trace and we can revisit it.

Comment 2

[Jonathan Almeida [:jonalmeida]]

(In reply to Jonathan Almeida [:jonalmeida] from comment #1)

2. We're using the same writeTo API that causes the OOM as we do in the BrowserStateWriter which calls the same write action every 30 secs (at minimum), but that does not throw an error (the user reported 100% reproducible when closing a tab even after reboots) - we don't know why this is still.

From our discussion again today, we found that the BrowserStateWriter is wrapped in a try-catch block that does in fact catch an OutOfMemoryError that we previously thought we might not have been able to catch. We wanted to see how often, if ever this was happening, but it doesn't seem like we have the crash reporter connected there in order to receive those error reports (to be fixed).

Titouan and Kaya are looking to reproduce the bug locally in order to put up a proposed patch.

Comment 3

[Titouan Thibaud [:titouan]]

Kaya [:kaya] and I investigated this bug today, and here is what we found:

We first produced a crash by simulating a 75MB string in GeckoEngineSessionState.kt, replacing the real actualState.toString().

This helped us confirming that we can catch the OutOfMemoryError, avoid the crash but loose the actualState data that should have been written on disk, and still maintain the app in a consistent state.

The consequence of this is: we loose the state of the tab that has just been close (e.g. its history stack, scroll state, form data...). So if the user opens the Recently closed screen, they will still be able to recover the tab, but with a cleared scroll state and no back stack.

What we are planning to do:

Wrap the crashing code with a try catch. In case of an OOM Error, we try to trim the history attached to the tab (that we suspect to be the heaviest information in the actualState) and try to write it again.
In case it still causes an OOM error (after several tries with each time a more agressive trim), then we can just avoid writing this state and at least avoid a crash.

Comment 4

[Arturo Mejia [:amejia]]

Thanks for investigating!
Just a heads up for the try catch for OutOfMemoryError, in some cases the catch may not work as the system could not have enough memory to allocate new objects. Reference:

"An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."

To have a consistent behaviour, we may need to be more aggressive about trimming memory before getting an OutOfMemoryError.

Comment 5

[Jonathan Almeida [:jonalmeida]]

(In reply to Arturo Mejia [:amejia] from comment #4)

Thanks for investigating!
Just a heads up for the try catch for OutOfMemoryError, in some cases the catch may not work as the system could not have enough memory to allocate new objects. Reference:

"An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."

To have a consistent behaviour, we may need to be more aggressive about trimming memory before getting an OutOfMemoryError.

The reason we're a bit optimistic that catching the OutOfMemoryError will suffice, is because we are doing this in the SessionStorage case where we are calling the same API (writeTo), but that code branch hasn't been a cause of problems.

I did file bug 1839513 to attach a crash reporter instance to that block, so that we forward those crash reports and then validate if we need to consider a more holistic approach (i.e. trimming the session state so we don't try to save all of it).

Comment 6

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1839250 - Avoid crash caused by OOM while saving a just-closed tab into storage (#2581)

Comment 7

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:007, could you have a look please?

For more information, please visit BugBot documentation.

Comment 8

[BMO Github Automation]

Authored by Titouan Thibaud
https://github.com/mozilla-mobile/firefox-android/commit/b6f71b6c4300cbbc5634a34922f5bfcdabc3de39
[main] Bug 1839250 - Avoid crash when an OutOfMemory Error is raised while trying to write a just-closed tab into storage

Comment 9

[Andrei Mocirean]

Hello.

There was no crash on the latest Nightly from 19th of July 2023 (117.0a1) using the provided STR.
Marking the ticket as verified.
Devices used for testing: Google Pixel 7 Pro (Android 14), Samsung Galaxy S22 Ultra (Android 13), Oppo Find X5 (Android 13), Samsung Galaxy Tab A6 (Android 5.1.1).

Thanks!

Comment 10

[BugBot [:suhaib / :marco]]

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

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

For more information, please visit BugBot documentation.

Comment 11

[Chris Peterson [:cpeterson]]

Titouan says he and Kaya think this fix is worth uplift to a 116 dot release.

Comment 12

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1839250 - Avoid crash caused by OOM while saving a just-closed tab into storage (backport #2581) (#3076)

Comment 13

[Titouan Thibaud [:titouan]]

Comment on attachment 9346732 [details] [review]
[mozilla-mobile/firefox-android] Bug 1839250 - Avoid crash caused by OOM while saving a just-closed tab into storage (backport #2581) (#3076)

Beta/Release Uplift Approval Request

• User impact if declined: Some users may continue experience crashes when closing a tab that has a state occupying a lot of memory space (most probably related to a huge back stack)
• 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): The fix is highly inspired by what was already done in the Periodical worker that saves the current tabs on disk, and works correctly. This leads me to consider it as non-risky.

It has also been release in Nightly two weeks ago and we haven't noticed any problem.

• String changes made/needed:
• Is Android affected?: Yes

Comment 14

[Dianna Smith [:diannaS]]

Comment on attachment 9346732 [details] [review]
[mozilla-mobile/firefox-android] Bug 1839250 - Avoid crash caused by OOM while saving a just-closed tab into storage (backport #2581) (#3076)

Approved for Mobile dot release 116.2.0

Comment 15

[BMO Github Automation]

Authored by https://github.com/mergify[bot]
https://github.com/mozilla-mobile/firefox-android/commit/8181cbd69c864cc6a62221f569cc54fe044f4cd6
[releases_v116] Bug 1839250 - Avoid crash when an OutOfMemory Error is raised while trying to write a just-closed tab into storage (#3076)

Comment 16

[Delia Pop (Android QA)]

Marking the version 116 verified fixed as well. No crash occurred in latest RC 116.2.0 with the following devices:

• Google Pixel 7 Pro (Android 14)
• Lenovo tab M10 (Android 10)
• OnePlus 6T (Android 9)
• Huawei MediaPad M2 (Android 5.1.1)