1343735 - Respect max_record_payload_bytes limit

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

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

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

Desktop recently added support for info/configuration endpoint, but somehow it entirely omitted max_record_payload_bytes attribute. We should fix that.

Comment 1

[Edouard Oger [:eoger]]

Attachment: Bug 1343735 - Respect max_record_payload_bytes limit.

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

Comment 2

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

Comment on attachment 8847776 [details]
Bug 1343735 - Respect max_record_payload_bytes limit.

https://reviewboard.mozilla.org/r/120702/#review122716

Looks great, but I think it needs a couple of minor tweaks.

::: services/sync/modules/engines.js:1644
(Diff revision 1)
>            out = this._createRecord(id);
>            if (this._log.level <= Log.Level.Trace)
>              this._log.trace("Outgoing: " + out);
>  
>            out.encrypt(this.service.collectionKeys.keyForCollection(this.name));
> +          let payloadLength = Utils.strByteLength(JSON.stringify(out.payload));

I don't think we need the strByteLength here, as I believe the encrypted data will all be ascii, and more importantly, the "post queue" doesn't do this - it just counts the length of the JSON.stringify. IOW, if it really *is* necessary here, the postQueue should change to also do that.

See also the end of bug 1343726 comment 3.

::: services/sync/modules/engines.js:1646
(Diff revision 1)
>              this._log.trace("Outgoing: " + out);
>  
>            out.encrypt(this.service.collectionKeys.keyForCollection(this.name));
> +          let payloadLength = Utils.strByteLength(JSON.stringify(out.payload));
> +          if (payloadLength > this.maxRecordPayloadBytes) {
> +            this._modified.delete(id); // Do not attempt to sync that record again

I think we only want to remove it from `_modified` is `this.allowSkippedRecords` - sadly, we *want* bookmarks to continiously fail forever, otherwise we *will* skip the record next sync, which we can't do is the engine says we shouldn't.

::: services/sync/modules/engines/tabs.js:214
(Diff revision 1)
>        return b.lastUsed - a.lastUsed;
>      });
>  
>      // Figure out how many tabs we can pack into a payload.
>      // See bug 535326 comment 8 for an explanation of the estimation
> -    // If the server configuration is absent, we use the old max payload size of 28K
> +    let size = Utils.strByteLength(JSON.stringify(tabs));

(although I think you are correct to count utf-8 here, as we are estimating based on the pre-encrypted data)

::: services/sync/tests/unit/test_syncengine_sync.js:1387
(Diff revision 1)
>  
>      // Check we didn't upload to the server
>      do_check_eq(collection.payload("flying"), undefined);
>      // And that we won't try to upload it again next time.
>      do_check_eq(engine._tracker.changedIDs["flying"], undefined);
>  

might be worth tweaking/cloning this test to ensure the case when allowSkippedRecords is true (meaning it *should* remain in the tracked list, effectively killing the engine :/

Comment 3

[Edouard Oger [:eoger]]

Comment on attachment 8847776 [details]
Bug 1343735 - Respect max_record_payload_bytes limit.

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

Comment 4

[Edouard Oger [:eoger]]

Thank you for the review Mark, I amended my patch with your comments.

Comment 5

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

Comment on attachment 8847776 [details]
Bug 1343735 - Respect max_record_payload_bytes limit.

https://reviewboard.mozilla.org/r/120702/#review124172

Looks great - sorry for the delay.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6c6563d5512
Respect max_record_payload_bytes limit. r=markh

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6c6563d5512