1492706 - OOM during GeckoApp.restoreSessionTabs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Session Restore, defect, P3)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

This bug was filed from the Socorro interface and is
report bp-46f9bda2-3a74-4915-ad86-6f2fb0180919.
=============================================================

Maybe we should handle OOM situations more gracefully during session restore.

Java stack trace:

java.lang.OutOfMemoryError
	at java.lang.AbstractStringBuilder.enlargeBuffer(AbstractStringBuilder.java:94)
	at java.lang.AbstractStringBuilder.append0(AbstractStringBuilder.java:132)
	at java.lang.StringBuilder.append(StringBuilder.java:124)
	at org.json.JSONStringer.string(JSONStringer.java:344)
	at org.json.JSONStringer.value(JSONStringer.java:252)
	at org.json.JSONObject.writeTo(JSONObject.java:672)
	at org.json.JSONStringer.value(JSONStringer.java:237)
	at org.json.JSONArray.writeTo(JSONArray.java:602)
	at org.json.JSONStringer.value(JSONStringer.java:233)
	at org.json.JSONObject.writeTo(JSONObject.java:672)
	at org.json.JSONStringer.value(JSONStringer.java:237)
	at org.json.JSONArray.writeTo(JSONArray.java:602)
	at org.json.JSONStringer.value(JSONStringer.java:233)
	at org.json.JSONObject.writeTo(JSONObject.java:672)
	at org.json.JSONStringer.value(JSONStringer.java:237)
	at org.json.JSONArray.writeTo(JSONArray.java:602)
	at org.json.JSONStringer.value(JSONStringer.java:233)
	at org.json.JSONObject.writeTo(JSONObject.java:672)
	at org.json.JSONStringer.value(JSONStringer.java:237)
	at org.json.JSONArray.writeTo(JSONArray.java:602)
	at org.json.JSONStringer.value(JSONStringer.java:233)
	at org.json.JSONObject.writeTo(JSONObject.java:672)
	at org.json.JSONStringer.value(JSONStringer.java:237)
	at org.json.JSONArray.writeTo(JSONArray.java:602)
	at org.json.JSONStringer.value(JSONStringer.java:233)
	at org.json.JSONObject.writeTo(JSONObject.java:672)
	at org.json.JSONObject.toString(JSONObject.java:641)
	at org.mozilla.gecko.GeckoApp.restoreSessionTabs(GeckoApp.java:1637)
	at org.mozilla.gecko.GeckoApp.access$200(GeckoApp.java:113)
	at org.mozilla.gecko.GeckoApp$9.run(GeckoApp.java:1153)
	at android.os.Handler.handleCallback(Handler.java:733)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:136)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43)

Comment 1

[Marcia Knous [:marcia]]

It looks as if in 64 a single install is generating the crashes.

Comment 2

[Jan Henning [:JanH]]

Hmm, we've also had stack overflows when parsing the session string into JSONObjects in the first place (bug 1352149) and now apparently also crashes for the reverse procedure, i.e. turning the JSONObjects back into a String.
Although some of those crashes apparently happen before even that stage, when we simply try reading the file into a String [1] and don't do any JSON-specific processing yet.

Other than just catching those errors and treating it as a session restore failure, I wonder whether it might be possible to use a streaming JSON parser here (in fact that was suggested by kats way back in bug 836948 comment 2, but unfortunately never got anywhere)?
The limiting factor is that for the session restore on startup, we still need to turn the whole session data back into a string for sending to Gecko, so in the worst case a streaming parser might avoid OOMs while reading and parsing the file, but still run into trouble when turning the processed session data back into a single, large JSON string. Unfortunately I've got no idea how likely that scenario would be, i.e. how much a streaming JSON parser would really buy us.

[1] https://dxr.mozilla.org/mozilla-central/rev/90853a59691e0c77014c38049bfb38b2b6ca1e16/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoProfile.java#762

Comment 4

[Jan Henning [:JanH]]

The RecentTabsAdapter can fail in that manner as well, and it seems that in 15 - 20 % of those cases, the OOM doesn't happen while reading/parsing a humongous session store file, but already when attempting to launch the parse thread [1][2].

[1] https://crash-stats.mozilla.com/search/?signature=~OutOfMemoryError&java_stack_trace=~RecentTabsAdapter&product=FennecAndroid&date=%3E%3D2018-10-26T20%3A23%3A05.000Z&date=%3C2018-11-26T19%3A23%3A05.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature
[2] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java#146,201

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1492706 - Part 1: Catch OOM during startup session restore. r?nalexander

We just treat this like a defective session store file and first fall back to
the backup (although if the OOM is caused by a too-large file, it is likely that
the backup will be too large as well) and then turn of session restoring
completely.

We don't plug those failures into the session restore telemetry, though, because
that is supposed to only cover truly defective files.

Comment 6

[Jan Henning [:JanH]]

Attachment: Bug 1492706 - Part 2: Cover common OOM causes in the Recent tabs panel. r?nalexander

Looking at Crash Stats, the most common causes of OOMs involving the RecentTabs-
Adapter happen while reading the previous session store file into memory for
parsing, respectively while stringifying the parsed data back into a flat String
for further storage.

In the former case, we give up completely, because there's nothing we can do
short of switching to a streaming JSON parser (which is out of scope for this
bug), while in the latter case, we only skip the affected tab in the hope that
at least some tabs might be small enough to not cause an OOM.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bfc4d495548f
Part 1: Catch OOM during startup session restore. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/acefb73b4d60
Part 2: Cover common OOM causes in the Recent tabs panel. r=nalexander

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/bfc4d495548f
https://hg.mozilla.org/mozilla-central/rev/acefb73b4d60

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 9027638 [details]
Bug 1492706 - Part 1: Catch OOM during startup session restore. r?nalexander

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Session restore

User impact if declined: If memory is tight, large amounts of serialised session store data can cause an OOM crash.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Part 2

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just adding try-catch statements for OOMErrors for the relevant bits so we no longer crash and instead just skip the too-large data.

String changes made/needed: none

Comment 12

[Liz Henry (:lizzard)]

Comment on attachment 9027638 [details]
Bug 1492706 - Part 1: Catch OOM during startup session restore. r?nalexander

Avoiding an OOM sounds good. Let's uplift now and maybe we will see results with next week's Fennec beta 7 build.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/325595e085d7
https://hg.mozilla.org/releases/mozilla-beta/rev/0337b9cc8e0f

Comment 14

[Levente Sacal]

Based on the comments above as no manual QE is needed marking it as Verified.