1273342 - Don't attempt to sync history records with a URL size greater than our maximum record size

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

+++ This bug was initially created as a clone of Bug #1273341 +++

Via bug 1235474, we see failures attempting to upload large history records - probably those with a huge URL. There is some point where the URL is simply too big for us to successfully upload. We will continue to try and upload the record "forever"(ish)

We should consider imposing a maximum size of a URL, beyond which we just decline to sync the history record.

Comment 1

[Thom Chiovoloni [:tcsc] (ex-moco)]

Should we be checking the size of the title as well?

Record size is 256k, but there's some fuzziness for JSON stringify, base64 encode, encryption, etc (I guess strictly speaking it would be `(length(url) + length(title) + maxVisitsSize + padding) * 4/3 * encryptionOverhead` -- maxVisitsSize appears to be around 700b, since there are only ever 20 visits we include, and they appear to be ~34-35b).

Either way, my gut says that we should just add a check to make sure the url *and* title are both less than 65535, since this is the default places limit. Its probably overly conservative, but it seems very unlikely to be hit in practice, since AFAICT it only can come up due to addons (I could be wrong about this, though).

More generally, I feel that we should do something to handle these cases where the sync fails every time on specific records -- For history this could be as simple as adding an entry into the database to say its unsyncable after we fail to sync it due to size once.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to Thom Chiovoloni [:tcsc] from comment #1)

> More generally, I feel that we should do something to handle these cases
> where the sync fails every time on specific records -- For history this
> could be as simple as adding an entry into the database to say its
> unsyncable after we fail to sync it due to size once.

ISTM that for things like history we should just avoid re-adding it to the tracker in case of failure due to this - don't bother with trying to determine ourselves if it is too big (even though I suggested we should in comment 0), but instead just try the upload. It means that if sync is reset, we will then retry the oversized record, but that seems OK and avoids writing anything to the DB about the record.

(We should also think about incoming too - if somehow a record ended up on the server that we could successfully read but then fail to apply due to local constraints, we probably want to ignore that error too. I suspect we will fail to progress past that record currently)

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1273342 - Don't retry skippable records that fail to sync due to being too large.

Review commit: https://reviewboard.mozilla.org/r/92066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92066/

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8809488 [details]
Bug 1273342 - Don't retry skippable records that fail to sync due to being too large.

https://reviewboard.mozilla.org/r/92066/#review93384

A test for this seems fairly easy - let me know if it is really painful.

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8809488 [details]
Bug 1273342 - Don't retry skippable records that fail to sync due to being too large.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92066/diff/1-2/

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8809488 [details]
Bug 1273342 - Don't retry skippable records that fail to sync due to being too large.

https://reviewboard.mozilla.org/r/92066/#review94822

Thanks for the test!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90b691bf09f5
Don't retry skippable records that fail to sync due to being too large. r=markh

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/90b691bf09f5