1416313 - Clients engine should account for client records exceeding the record size limit

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Lina Butler [:lina]]

It's possible for client command lists to blow the record size limit: several sent tabs with long URLs, and a repair request listing many GUIDs, might be enough to do this. This is especially bad because breaking the clients engine will prevent that client from syncing again, with no way to clean up the list.

We should consider either evicting old commands, refusing to add new commands until the client syncs and clears them out, or both. The tabs engine already does something similar: https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/services/sync/modules/engines/tabs.js#215-248

Comment 1

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

I thought this was going to be a quick patch I could do while I reinstalled Xcode. It ended up being much trickier than that, but oh well.

Patch incoming.

Comment 2

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

Attachment: Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

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

Comment 3

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

Comment on attachment 8927449 [details]
Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

https://reviewboard.mozilla.org/r/198778/#review203864

I suspect this might need to clear the commands out from our stored lists somehow too, but it's unclear to me the best way to do that. Thoughts?

Worth noting that while dropping commands isn't ideal, I think we need to do so since without doing that sync is completely broken.

::: services/sync/modules/engines/clients.js:886
(Diff revision 1)
>     *        to all remote clients.
>     * @param title
>     *        Title of the page being sent.
>     */
>    async sendURIToClientForDisplay(uri, clientId, title) {
> -    this._log.info("Sending URI to client: " + uri + " -> " +
> +    this._log.trace("Sending URI to client: " + uri + " -> " +

This is otherwise unrelated to the patch but it's PII we'd log without trace logging enabled.

::: services/sync/modules/engines/clients.js:1001
(Diff revision 1)
>          this._log.error(`Preparing to upload record ${id} that we consider stale`);
>          delete record.cleartext.stale;
>        }
>      }
> -
> +    if (record.commands) {
> +      const maxPayloadSize = this.engine.service.getMemcacheMaxRecordPayloadSize();

This is here and not in `_addClientCommand`, since that's used to delete commands, sometimes. It's not completely clear to me what to do about this, though.

Comment 4

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8927449 [details]
Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

https://reviewboard.mozilla.org/r/198778/#review203992

I'm not convinced dropping command is the right thing to do (although I admit to now knowing what is better :). Eg, syncing history deletions (will), and a restore from backup (does, if the stars align) require a command to be delivered for Sync to work correctly. I think we should discuss this a little more, but at the very least, I think we should consider some commands more important than others.

Comment 5

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

Sure, some kind of priority system would be doable. It's worth noting though, that in the current system we won't sync the command either if we hit the state where this patch would do anything.

(Well, that's not strictly true, since IIUC 512k is quite conservative, but it's close to true)

Comment 6

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Thom Chiovoloni [:tcsc] from comment #5)
> It's worth noting
> though, that in the current system we won't sync the command either if we
> hit the state where this patch would do anything.

Yeah, I understand that - I just want to ensure the fix is sane. Off the top of my head, I'd guess we'd drop all repair related first, then all displayURI commands next (both earliest first), then others just considering date?

Comment 7

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

Comment on attachment 8927449 [details]
Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

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

Comment 8

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

Comment on attachment 8927449 [details]
Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

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

Comment 9

[Edouard Oger [:eoger]]

Comment on attachment 8927449 [details]
Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

https://reviewboard.mozilla.org/r/198778/#review204230

Looks great, I left a few comments but I like the cleanliness of that patch!

::: services/sync/modules/engines/tabs.js:217
(Diff revision 3)
>      // Sort tabs in descending-used order to grab the most recently used
>      let tabs = this.getAllTabs(true).sort(function(a, b) {
>        return b.lastUsed - a.lastUsed;
>      });
> -    let encoder = new TextEncoder("utf-8");
> -    // Figure out how many tabs we can pack into a payload.
> +    const maxPayloadSize = this.engine.service.getMemcacheMaxRecordPayloadSize();
> +    let records = Utils.tryFitRecords(tabs, maxPayloadSize);

Looks pretty clean now!

::: services/sync/modules/engines/tabs.js:219
(Diff revision 3)
>        return b.lastUsed - a.lastUsed;
>      });
> -    let encoder = new TextEncoder("utf-8");
> -    // Figure out how many tabs we can pack into a payload.
> -    // We use byteLength here because the data is not encrypted in ascii yet.
> -    let size = encoder.encode(JSON.stringify(tabs)).byteLength;
> +    const maxPayloadSize = this.engine.service.getMemcacheMaxRecordPayloadSize();
> +    let records = Utils.tryFitRecords(tabs, maxPayloadSize);
> +
> +    this._log.trace("Created tabs " + records.length + " of " + tabs.length);

Could we log if we had drop some tabs?

::: services/sync/modules/engines/tabs.js:221
(Diff revision 3)
> -    let encoder = new TextEncoder("utf-8");
> -    // Figure out how many tabs we can pack into a payload.
> -    // We use byteLength here because the data is not encrypted in ascii yet.
> -    let size = encoder.encode(JSON.stringify(tabs)).byteLength;
> -    let origLength = tabs.length;
> -    const maxPayloadSize = this.getMaxRecordPayloadSize();
> +    const maxPayloadSize = this.engine.service.getMemcacheMaxRecordPayloadSize();
> +    let records = Utils.tryFitRecords(tabs, maxPayloadSize);
> +
> +    this._log.trace("Created tabs " + records.length + " of " + tabs.length);
> +
> +    records.forEach(tab => {

Nit (feel free to ignore): We could wrap this in a if(this._log.level <= Log.TRACE) so we don't iterate for nothing

::: services/sync/modules/util.js:371
(Diff revision 3)
>    /**
> +   * Helper utility function to fit an array of records so that when serialized,
> +   * they will be within payloadSizeMaxBytes. Returns a new array without the
> +   * items.
> +   */
> +  tryFitRecords(records, payloadSizeMaxBytes) {

I'm a bit concerned about the "record" denomination, I'm scared that we might mix this up with a Sync record (I did).
Feel free to ignore if you think that's ok.

::: services/sync/modules/util.js:372
(Diff revision 3)
> +   * Helper utility function to fit an array of records so that when serialized,
> +   * they will be within payloadSizeMaxBytes. Returns a new array without the
> +   * items.
> +   */
> +  tryFitRecords(records, payloadSizeMaxBytes) {
> +    // Copy this so that callers don't have to do it in advance.

Didn't we already slice commands?

::: services/sync/modules/util.js:374
(Diff revision 3)
> +   * items.
> +   */
> +  tryFitRecords(records, payloadSizeMaxBytes) {
> +    // Copy this so that callers don't have to do it in advance.
> +    records = records.slice();
> +    let encoder = new TextEncoder("utf-8");

This should probably be cached (see https://searchfox.org/mozilla-central/source/services/crypto/modules/WeaveCrypto.js#55)

::: services/sync/modules/util.js:393
(Diff revision 3)
> +      // Estimate a little more than the direct fraction to maximize packing
> +      let cutoff = Math.ceil(records.length * maxSerializedSize / size);
> +      records = records.slice(0, cutoff + 1);
> +
> +      // Keep dropping off the last entry until the data fits.
> +      while (encoder.encode(JSON.stringify(records)).byteLength > maxSerializedSize) {

We could maybe factorize encoder.encode(JSON.stringify(records)).byteLength in  an arrow function.

Comment 10

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

Comment on attachment 8927449 [details]
Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

https://reviewboard.mozilla.org/r/198778/#review204230

> Could we log if we had drop some tabs?

This will log if we did. Do you think we need better or non-trace logging? I guess a warn(dropped N tabs to meet size restrictions) or something could be appropriate, so I've changed it to that.

> Nit (feel free to ignore): We could wrap this in a if(this._log.level <= Log.TRACE) so we don't iterate for nothing

I was tempted but left it since it already was this way. You've convinced me to change it though.

> Didn't we already slice commands?

We do before the sort to avoid reording in place, but having the function modify in place seems like a bit of a footgun to me, so semantically I think it makes more sense to have it return a new array.

> This should probably be cached (see https://searchfox.org/mozilla-central/source/services/crypto/modules/WeaveCrypto.js#55)

I did this, but I suspect it doesn't matter.

Comment 11

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

Comment on attachment 8927449 [details]
Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

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

Comment 12

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8927449 [details]
Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

https://reviewboard.mozilla.org/r/198778/#review204334

Awesome, thanks

::: services/sync/modules/engines/clients.js:887
(Diff revision 4)
>     *        to all remote clients.
>     * @param title
>     *        Title of the page being sent.
>     */
>    async sendURIToClientForDisplay(uri, clientId, title) {
> -    this._log.info("Sending URI to client: " + uri + " -> " +
> +    this._log.trace("Sending URI to client: " + uri + " -> " +

I don't really care, but info does seem the appropriate level here.

::: services/sync/modules/engines/clients.js:1003
(Diff revision 4)
>          delete record.cleartext.stale;
>        }
>      }
> -
> +    if (record.commands) {
> +      const maxPayloadSize = this.engine.service.getMemcacheMaxRecordPayloadSize();
> +      // The reverse() calls ensure we drop the oldest commands first. This

This comment now looks stale WRT reverse()?

::: services/sync/modules/engines/clients.js:1031
(Diff revision 4)
> +        // of the list, so that they are removed first.
> +        return origIdxB - origIdxA;
> +      });
> +      let truncatedCommands = Utils.tryFitItems(commands, maxPayloadSize);
> +      if (truncatedCommands.length != record.commands.length) {
> +        // Should we record event telemetry here?

It's not really clear what action we could/would take, so it's probably not worthwhile atm.

::: services/sync/modules/util.js:381
(Diff revision 4)
> +    // Copy this so that callers don't have to do it in advance.
> +    records = records.slice();
> +    let encoder = Utils.utf8Encoder;
> +    const computeSerializedSize = () =>
> +      encoder.encode(JSON.stringify(records)).byteLength;
> +    // Figure out how many tabs we can pack into a payload.

s/tabs/record/ here

Comment 13

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

Comment on attachment 8927449 [details]
Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

https://reviewboard.mozilla.org/r/198778/#review204358

::: services/sync/modules/engines/clients.js:887
(Diff revision 4)
>     *        to all remote clients.
>     * @param title
>     *        Title of the page being sent.
>     */
>    async sendURIToClientForDisplay(uri, clientId, title) {
> -    this._log.info("Sending URI to client: " + uri + " -> " +
> +    this._log.trace("Sending URI to client: " + uri + " -> " +

Even given that we try not to log uris with trace logging off?

Comment 14

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Thom Chiovoloni [:tcsc] from comment #13)
> Even given that we try not to log uris with trace logging off?

Doh - no :)

Comment 15

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

Comment on attachment 8927449 [details]
Bug 1416313 - Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d1a6d34cbda
Drop old or low priority commands rather than failing to sync the clients engine if too many commands are sent. r=eoger,markh

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4d1a6d34cbda