1343726 - Respect max_record_payload_bytes limit

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, enhancement, P1)

Description

[:Grisha Kruglov]

Bug 1253111 added support for info/configuration endpoint, but somehow it entirely omitted max_record_payload_bytes attribute.

Bug 1291821, among other things, lifted limits for number of bookmarks & form history items a client can sync.

max_record_payload_bytes defines a max limit of the BSO payload's size, which is currently at 256kB. As we currently ignore it but do respect the much higher max_post_bytes currently set at 1MB, we might attempt uploading a BSO whose payload size is just under 1MB. My expectation is that this attempt will result in a 400 error from the server; client will then fail sync of whatever collection tried this.

For bookmarks, of particular interest is the "mobile bookmarks" folder which is a root folder for all mobile bookmarks. Its BSO lists all of children GUIDs. With a 256kB limit and UTF8 encoding gets us something like 15-17k children until we go past the per-payload limit.

For now, reasonable steps to deal with this seem to be:
- modify InfoConfiguration stage to fetch and expose max_record_payload_bytes
- for bookmarks (and in general), make sure that BSO which might offend is processed and refused by the uploader as early as possible, so that we're not making any POSTs
- ideally bubble up some error reporting to the user, so that we're not just failing silently

Alternatively, it might make sense to refuse syncing if bookmark count is above some number as we did prior to Bug 1291821, but seems like a worse approach as it won't benefit at all if/once max_record_payload_bytes is raised.

Comment 1

[:Grisha Kruglov]

Attachment: Bug 1343726 - Respect max_record_payload_bytes limit while uploading records

If we'll try to upload a record whose payload BSO field is larger than the limit specified
by the server, fail that record during BatchingUploader's processing.

Consequently, Synchronizer will fail current sync stage and advance to the next.

Previous behaviour was to essentially rely on the server to fail our POST request,
at which point we'll fail current sync stage. So in a way, this is an optimization to
avoid making network requests which we know will fail.

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

Comment 2

[:Grisha Kruglov]

Attachment: Bug 1343726 - Ensure that bookmark folders are processed first by the uploader

Since we're uploading records atomically, order in which they're processed by the uploader
only matters if we want to do sanity checks on certain types of records.

We'd like to ensure that we process the "mobile root" bookmark record first, so that we increase
our chances of avoiding making a failing network request if that record's payload is too large.

Mobile root folder will have an _id=-3, and so ordering by _id lets us easily get at it first.

This ordering also applies to history, but that shouldn't matter since, again, we're doing atomic
uploads and all records are applied on the server at the same time.

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

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8847336 [details]
Bug 1343726 - Respect max_record_payload_bytes limit while uploading records

https://reviewboard.mozilla.org/r/120336/#review122312

::: commit-message-a8d49:3
(Diff revision 1)
> +Bug 1343726 - Respect max_record_payload_bytes limit while uploading records r=rnewman
> +
> +If we'll try to upload a record whose payload BSO field is larger than the limit specified

If we

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java:143
(Diff revision 1)
>                      new Server15PreviousPostFailedException(), guid
>              );
>              return;
>          }
>  
> +        final String payloadField = (String) record.toJSONObject().get(CryptoRecord.KEY_PAYLOAD);

Getting a JSON object for a record is a bunch of work, and you repeat that work on line 167. Here you get the JSON object and pull out a single field; on 167 you get the JSON object and turn it into bytes.

Do this only once, and then turn the JSON object you inspected into bytes!

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/uploaders/BatchingUploader.java:151
(Diff revision 1)
> +                    new IllegalRecordException(), guid
> +            );
> +            return;
> +        }
> +
> +        final byte[] payloadBytes = Record.stringToJSONBytes(payloadField);

Here you're taking a large payload string and filling a new byte array for it in order to count the number of bytes.

Then you're doing this again for the record as a whole on line 167 in order to check the upload payload limit.

Can you avoid doing this work twice? The payload is encrypted and encoded, right? If it's in a fixed ASCII alphabet, you can simply count the number of characters in `recordObject.get(KEY_PAYLOAD)` — UTF-8 will encode those into single bytes on the wire.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8847338 [details]
Bug 1343726 - Ensure that bookmark folders are processed first by the uploader

https://reviewboard.mozilla.org/r/120338/#review122316

Do you think this is really necessary, given that you'd need 17,000 children of Mobile Bookmarks to hit a problem, and likely it'll be hit incrementally with a single change affecting two records?

Is this a scalable solution, when bookmark organization is being built for Android right now, and this problem is more likely to affect desktop-sourced folders?

What other solutions did you consider? E.g.,

- Querying for changed folders first (perhaps ordering by type, or using UNION ALL) instead of sorting by ID.
- Maintaining a materialized count of children and querying that directly, instead of waiting for record addition to fail.

This is also the kind of thing that begs for a comment…

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserRepositoryDataAccessor.java:156
(Diff revision 1)
>     */
>    public Cursor fetchSince(long timestamp) throws NullCursorException {
>      return queryHelper.safeQuery(".fetchSince",
>                                   getAllColumns(),
>                                   dateModifiedWhere(timestamp),
> -                                 null, null);
> +                                 null, BrowserContract.CommonColumns._ID + " ASC");

What's the change in the query plan?

This might result in an in-memory sort, which will affect peak memory usage (a bad thing) and make the query slower.

Comment 5

[:Grisha Kruglov]

Comment on attachment 8847338 [details]
Bug 1343726 - Ensure that bookmark folders are processed first by the uploader

https://reviewboard.mozilla.org/r/120338/#review122316

Ordering by `type` does seem like a more generally applicable solution. In fact, our current (default) sort is `type ASC, position ASC, id ASC`, so we already do this, but slowly (there's no index that will help out). Dropping this to just `type ASC` should speed things up, as we'll benefit from the composite index on `(type, is_deleted)`.

```
explain query plan select * from bookmarks where modified > 123 order by type asc;
0|0|0|SCAN TABLE bookmarks USING INDEX bookmarks_type_deleted_index
```

Speaking of desktop folders - currently, if they arrived somehow, I think we can assume that they're likely under the limit. Also, currently mobile users can't manage them, and so those seem like the less likely offenders, until bookmark management arrives.

Generally speaking, I don't know what the scope of this problem is in practice, since there's no telemetry around this - other than server error logs. I think a reasonable approach now is do the simplest and most obvious thing possible, and re-assess if necessary once we get telemetry around this.

Looking over bobm's email with "payload too large" stats, it seems that biggest offenders are tabs (due to tab history?), followed by history (likely due to way too many visits), followed by bookmarks (folders?). Number of offending payloads is low.

> What's the change in the query plan?
> 
> This might result in an in-memory sort, which will affect peak memory usage (a bad thing) and make the query slower.

For bookmarks, we already do a "default" sort here if one isn't provided: "type ASC, position ASC, id ASC" (https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java?q=BrowserProvider.java&redirect_type=direct#1268).

So if anything, this should speed up the query. We don't have a composite index on (type, position, id), but there's an implicit index on the primary key `id`.

For history, by default we sort by `date`, and I'm assuming that switching sort order from one indexed column to another shouldn't affect query's execution profile too much.

Comment 6

[Edouard Oger [:eoger]]

Attachment: DO NOT APPLY

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

Comment 7

[Edouard Oger [:eoger]]

Comment on attachment 8847775 [details]
DO NOT APPLY

Sorry wrong bug, my bad.

Comment 8

[:Grisha Kruglov]

Comment on attachment 8847336 [details]
Bug 1343726 - Respect max_record_payload_bytes limit while uploading records

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

Comment 9

[:Grisha Kruglov]

Comment on attachment 8847338 [details]
Bug 1343726 - Ensure that bookmark folders are processed first by the uploader

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

Comment 10

[:Grisha Kruglov]

Comment on attachment 8847336 [details]
Bug 1343726 - Respect max_record_payload_bytes limit while uploading records

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

Comment 11

[:Grisha Kruglov]

Comment on attachment 8847338 [details]
Bug 1343726 - Ensure that bookmark folders are processed first by the uploader

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

Comment 12

[:Grisha Kruglov]

Richard, could you please take another look at the patches?

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8847336 [details]
Bug 1343726 - Respect max_record_payload_bytes limit while uploading records

https://reviewboard.mozilla.org/r/120336/#review125632

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8847338 [details]
Bug 1343726 - Ensure that bookmark folders are processed first by the uploader

https://reviewboard.mozilla.org/r/120338/#review125634

::: commit-message-9d024:3
(Diff revision 3)
> +Since we're uploading records atomically, order in which they're processed by the uploader
> +only matters if we want to do sanity checks on certain types of records.

Strictly speaking this might not be true -- I imagine the server still preserves some amount of upload order, if only by accident.

::: commit-message-9d024:9
(Diff revision 3)
> +only matters if we want to do sanity checks on certain types of records.
> +
> +We'd like to ensure that we process the "mobile root" bookmark record first, so that we increase
> +our chances of avoiding making a failing network request if that record's payload is too large.
> +
> +Mobile root folder will have an _id=-3, and so ordering by _id lets us easily get at it first.

Clean up this commit message to match the new logic!

::: commit-message-9d024:11
(Diff revision 3)
> +We'd like to ensure that we process the "mobile root" bookmark record first, so that we increase
> +our chances of avoiding making a failing network request if that record's payload is too large.
> +
> +Mobile root folder will have an _id=-3, and so ordering by _id lets us easily get at it first.
> +
> +This ordering also applies to history, but that shouldn't matter since, again, we're doing atomic

No longer true, right?

Comment 15

[:Grisha Kruglov]

Comment on attachment 8847336 [details]
Bug 1343726 - Respect max_record_payload_bytes limit while uploading records

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120336/diff/3-4/

Comment 16

[:Grisha Kruglov]

Comment on attachment 8847338 [details]
Bug 1343726 - Ensure that bookmark folders are processed first by the uploader

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120338/diff/3-4/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b63c89fbcda2
Respect max_record_payload_bytes limit while uploading records r=rnewman
https://hg.mozilla.org/integration/autoland/rev/29b5058903ab
Ensure that bookmark folders are processed first by the uploader r=rnewman

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b63c89fbcda2
https://hg.mozilla.org/mozilla-central/rev/29b5058903ab