1357946 - Crash in TabsPayload.Tab.(fromJSON(JSON) -> TabsPayload.Tab?).(getLastUsed #1)(JSON) -> UInt64?

Status: RESOLVED FIXED

(Firefox for iOS :: General, enhancement, P1)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

The trace is a bit more complex, but the method in the bug summary is high up and has a line number attached.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: 2017-04-19_23-03-47.9790_+0200-fe4157c0b79ff2ff5d11519ec5517c41daac2177.crash

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Does TabsPayload.isValid need a more extensive check? It only checks if self["tabs"] is an array, but it does not validate the items inside the array.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Stefan Arentz [:st3fan] from comment #2)
> Does TabsPayload.isValid need a more extensive check? It only checks if
> self["tabs"] is an array, but it does not validate the items inside the
> array.

No, it relies on failable processing internally -- we'd rather have the valid tabs of a client than have one problem make the whole client 'disappear'.

We do that here:

            func getLastUsed(_ json: JSON) -> Timestamp? {
                // This might be a string or a number.
                if let num = json["lastUsed"].int64 {
                    return Timestamp(num * 1000)
                }

                if let num = json["lastUsed"].string {
                    // Try parsing.
                    return decimalSecondsStringToTimestamp(num)
                }

                return nil
            }

Obviously something is going on here: despite what the documentation says, the string case should be a failure in extremis (that's a really, really old bug).

https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/tabs.js#193

is what desktop does: integer seconds.

The string case is not happening in extremis: again, this is a case of iOS now producing records that we fail to parse.

A desktop tab record looks like this:

{"title": "Gradual OLTP DB Development - from Zero to 10 Billion Transactions per Year and Beyond - IT Hare on Soft.ware",
 "urlHistory": ["http://ithare.com/gradual-oltp-db-development-from-zero-to-10-billion-transactions-per-year-and-beyond/"],
 "icon": "http://ithare.com/favicon.ico",
 "lastUsed": 1492649651}

an iOS Firefox 7.* record looks like this:

{"icon": null,
 "title": "https://www.outsideonline.com/1785631/whats-worlds-best-midlayer",
 "lastUsed": "1492316843992",
 "urlHistory": ["https://www.outsideonline.com/1785631/whats-worlds-best-midlayer"]}

As you can see, not only is that `lastUsed` at the wrong granularity, it's also not decimal seconds.

So there are two bugs here:

- We're not resilient against some kind of malformed data. The code looks fine, but apparently it's not.
- We're generating invalid tab records.

Comment 4

[Richard Newman [:rnewman]]

Attachment: PR

Reviews requested on GitHub.