Closed Bug 1273342 Opened 4 years ago Closed 3 years ago

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

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: markh, Assigned: tcsc)

Details

Attachments

(1 file)

+++ 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.
Depends on: 1273348
No longer depends on: 1273341
No longer depends on: 1273348
Priority: P2 → P3
Priority: P3 → P2
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.
Priority: P2 → P1
(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)
Assignee: nobody → tchiovoloni
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.
Attachment #8809488 - Flags: review?(markh)
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!
Attachment #8809488 - Flags: review?(markh) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90b691bf09f5
Don't retry skippable records that fail to sync due to being too large. r=markh
https://hg.mozilla.org/mozilla-central/rev/90b691bf09f5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.