1166806 - Gracefully handle history records that fail to apply

Status: NEW

(Firefox for iOS :: Sync, defect, P3)

Description

[Richard Newman [:rnewman]]

If we hit a record that won't apply, we apply all the other records in the batch, then we fail. If we repeatedly hit the same failing record, we'll never upload, and we'll never advance our timestamp.

We should track failing records and move forward, much as we do on other platforms.

(This approach only applies to synchronizers like Bug 1141849.)

Comment 1

[Richard Newman [:rnewman]]

There are two places this could happen.

1. We get a response from the server that parses as JSON, but some or all of the records in the response fail Record<T>.fromEnvelope — e.g., they don't decrypt, or they decrypt to an invalid payload.

This happens in the StorageClient, and we immediately drop those malformed records.

2. HistoryPayload.asPlace yields a Place that doesn't apply. If the URI is invalid we'll crash (but I'm about to turn this into a plain String op).

It's not possible that we'll cause a URL uniqueness conflict, because we switch the local GUID if one exists, prior to applying.

No NOT NULL constraints should fail, because we're applying from a parsed record with non-null fields.

So a failure here would most likely be a DB error.


We can't many of get the kinds of failures that desktop sees, such as "invalid visit type" from PlacesUtils, because we either bake that validation into our parsing (e.g., we silently drop invalid visits in HistoryPayload), or we don't do the validation at all.


tl;dr: failure to apply records is much less likely to happen here than on desktop (or even on Android), so I don't think this is as urgent as it might appear.

Comment 2

[Richard Newman [:rnewman]]

And yet:

2015-05-21 19:15:40.817 [Error] [HistorySynchronizer.swift:53] allSucceed: Record application failed: Optional(Abort due to constraint violation UNIQUE constraint failed: history.url)

Digging into how that happened now!

Comment 3

[Richard Newman [:rnewman]]

Can't repro. My guess is Bug 1167490.

Comment 4

[Richard Newman [:rnewman]]

My other guess is that we were triggering the application of records in parallel!

Comment 5

[Richard Newman [:rnewman]]

This is part-done, but we should make sure it's rock solid by 2.0 (or sooner if we get lots of user complaints!).

Comment 6

[Stephan Leroux [:sleroux]]

Do we want to schedule this as part of 2.0?