1352149 - Crash in java.lang.StackOverflowError: somewhere in JSONParser while parsing the session store JSON file

Status: RESOLVED WORKSFORME

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

Description

[Jan Henning [:JanH]]

This bug was filed from the Socorro interface and is 
report bp-50e31fcb-e847-42b4-a6e4-08e3e2170327.
=============================================================

So far there are only a few reports of this on 52 (current Release), all of which seem to date from approximately the same time and the same installation as well.

The crash happens when attempting to parse the session store data into a JSON object (https://dxr.mozilla.org/mozilla-release/rev/581768df8cc0a21c6760ccc7c691f9b6210c8ab8/mobile/android/base/java/org/mozilla/gecko/SessionParser.java#84).

This could be a session store file that is either too large (the whole file, or maybe just a certain object/property within it - I've no idea what is more critical here) or else corrupted, possibly in combination with a bug in the JSON parser we're using, especially for the latter case. If it was really a corrupted file, the JSON parser should have thrown a JSONException instead of running into a stack overflow.

Comment 1

[Jan Henning [:JanH]]

Further thoughts first recorded on the sync bug:

The crashing phone is a S4 Mini and during debugging some other bug using a large-ish session file provided by somebody, even my old S3 Mini was able to handle something like around 50 tabs fine. Also, according to telemetry [1] some users have session files well into the megabytes, yet at the same time there aren't very many crash reports for this issue around. So maybe the corrupted file triggering a parser bug might be slightly more likely?

[1] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-03-24&keys=__none__!__none__!__none__&max_channel_version=release%252F52&measure=FX_SESSION_RESTORE_FILE_SIZE_BYTES&min_channel_version=null&processType=*&product=Fennec&sanitize=1&sort_keys=submissions&start_date=2017-03-03&table=0&trim=1&use_submission_date=0

Comment 2

[Richard Newman [:rnewman]]

org.json.JSONParser is recursive.

Sessionstore -- at least on desktop -- is also recursive, for child documents:

{windows: [{tabs: [{entries: [{children: [{children: [{…

That crash has 390 lines of JSON parsing stack, with three layers for each level of JSON nesting. We always double-nest our JSON, effectively -- array of objects -- so to get to the first layer of children is 21 stack frames. To hit this crash would be about 60 layers of nested children. I can imagine that being possible with the horrific ad ecosystem.

If this isn't malformed data, it could be legitimately painful data.

You could try to replicate this: construct a tree of documents with 100 nested iframes, and see what the recorded session data is.

If that is the issue, the solutions would be to use a non-recursive parser, to catch the exception, and to control the recorded depth.

Comment 3

[Jan Henning [:JanH]]

(In reply to Richard Newman [:rnewman] from comment #2)
> Sessionstore -- at least on desktop -- is also recursive, for child
> documents:
> 
> {windows: [{tabs: [{entries: [{children: [{children: [{…

Yup, we're using the same basic structure and the actual session history data (everything included in "entries") should be more or less identical [1] to what Desktop would be generating.

As for the possibility of a corrupted file, there's this:
http://stackoverflow.com/questions/31381310/java-lang-stackoverflowerror-while-parsing-a-json-string-with-org-json-jsonobjec

In principle I could try taking real files and truncating them by varying amounts to see if I can get anything else than a JSONException from the parser.

[1] As evidenced by the fact that in bug 1340828 our own session history code could be simply replaced by Desktop's SessionHistory.jsm.

Comment 4

[Richard Newman [:rnewman]]

Yeah, truncation is always a possibility; it'd be a pity if we can't trust the JSON parser to this degree!

Comment 5

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 6

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

Closing because no crashes reported for 12 weeks.