Bug 1343735 - Respect max_record_payload_bytes limit. r=markh

MozReview-Commit-ID: 1aDwl1cq2Hh

--HG--
extra : rebase_source : a29abc5c1f3b9eefea26fe93b5ee82f4b3d12924

services/sync/modules/constants.js

@@ -78,6 +78,10 @@ APPS_STORE_BATCH_SIZE:                 50,      // same as MOBILE_BATCH_SIZE
 // (how many records are fetched at a time from the server when batching is used).
 DEFAULT_DOWNLOAD_BATCH_SIZE:           1000,
 
+
+// Default maximum size for a record payload
+DEFAULT_MAX_RECORD_PAYLOAD_BYTES:      262144,  // 256KB
+
 // score thresholds for early syncs
 SINGLE_USER_THRESHOLD:                 1000,
 MULTI_DEVICE_THRESHOLD:                300,

services/sync/modules/engines.js

@@ -931,6 +931,14 @@ SyncEngine.prototype = {
     Svc.Prefs.set(this.name + ".lastSyncLocal", value.toString());
   },
 
+  get maxRecordPayloadBytes() {
+    let serverConfiguration = this.service.serverConfiguration;
+    if (serverConfiguration && serverConfiguration.max_record_payload_bytes) {
+      return serverConfiguration.max_record_payload_bytes;
+    }
+    return DEFAULT_MAX_RECORD_PAYLOAD_BYTES;
+  },
+
   /*
    * Returns a changeset for this sync. Engine implementations can override this
    * method to bypass the tracker for certain or all changed items.
@@ -1633,6 +1641,13 @@ SyncEngine.prototype = {
             this._log.trace("Outgoing: " + out);
 
           out.encrypt(this.service.collectionKeys.keyForCollection(this.name));
+          let payloadLength = JSON.stringify(out.payload).length;
+          if (payloadLength > this.maxRecordPayloadBytes) {
+            if (this.allowSkippedRecord) {
+              this._modified.delete(id); // Do not attempt to sync that record again
+            }
+            throw new Error(`Payload too big: ${payloadLength} bytes`);
+          }
           ok = true;
         } catch (ex) {
           this._log.warn("Error creating record", ex);

services/sync/modules/engines/tabs.js

@@ -210,13 +210,11 @@ TabStore.prototype = {
     });
 
     // 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 = JSON.stringify(tabs).length;
+    // We use byteLength here because the data is not encrypted in ascii yet.
+    let size = new TextEncoder("utf-8").encode(JSON.stringify(tabs)).byteLength;
     let origLength = tabs.length;
-    const MAX_TAB_SIZE = (this.engine.service.serverConfiguration ?
-                          this.engine.service.serverConfiguration.max_record_payload_bytes :
-                          28672) / 4 * 3 - 1500;
+    // See bug 535326 comment 8 for an explanation of the estimation
+    const MAX_TAB_SIZE = this.engine.maxRecordPayloadBytes / 4 * 3 - 1500;
     if (size > MAX_TAB_SIZE) {
       // Estimate a little more than the direct fraction to maximize packing
       let cutoff = Math.ceil(tabs.length * MAX_TAB_SIZE / size);

services/sync/tests/unit/test_syncengine_sync.js

@@ -1343,8 +1343,8 @@ add_task(async function test_uploadOutgoing_toEmptyServer() {
   }
 });
 
-
-add_task(async function test_uploadOutgoing_huge() {
+async function test_uploadOutgoing_max_record_payload_bytes(allowSkippedRecord) {
+  _("SyncEngine._uploadOutgoing throws when payload is bigger than max_record_payload_bytes");
   let collection = new ServerCollection();
   collection._wbos.flying = new ServerWBO("flying");
   collection._wbos.scotsman = new ServerWBO("scotsman");
@@ -1352,17 +1352,19 @@ add_task(async function test_uploadOutgoing_huge() {
   let server = sync_httpd_setup({
       "/1.1/foo/storage/rotary": collection.handler(),
       "/1.1/foo/storage/rotary/flying": collection.wbo("flying").handler(),
+      "/1.1/foo/storage/rotary/scotsman": collection.wbo("scotsman").handler(),
   });
 
   await SyncTestingInfrastructure(server);
   generateNewKeys(Service.collectionKeys);
 
   let engine = makeRotaryEngine();
-  engine.allowSkippedRecord = true;
+  engine.allowSkippedRecord = allowSkippedRecord;
   engine.lastSync = 1;
-  engine._store.items = { flying: "a".repeat(1024 * 1024) };
+  engine._store.items = { flying: "a".repeat(1024 * 1024), scotsman: "abcd" };
 
   engine._tracker.addChangedID("flying", 1000);
+  engine._tracker.addChangedID("scotsman", 1000);
 
   let meta_global = Service.recordManager.set(engine.metaURL,
                                               new WBORecord(engine.metaURL));
@@ -1370,23 +1372,49 @@ add_task(async function test_uploadOutgoing_huge() {
                                          syncID: engine.syncID}};
 
   try {
-
     // Confirm initial environment
     do_check_eq(engine.lastSyncLocal, 0);
     do_check_eq(collection.payload("flying"), undefined);
+    do_check_eq(collection.payload("scotsman"), undefined);
 
     engine._syncStartup();
     engine._uploadOutgoing();
+
+    if (!allowSkippedRecord) {
+      do_throw("should not get here");
+    }
+
     engine.trackRemainingChanges();
 
-    // 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.
+    // Check we uploaded the other record to the server
+    do_check_true(collection.payload("scotsman"));
+    // And that we won't try to upload the huge record next time.
     do_check_eq(engine._tracker.changedIDs["flying"], undefined);
 
+  } catch (e) {
+    if (allowSkippedRecord) {
+      do_throw("should not get here");
+    }
+
+    engine.trackRemainingChanges();
+
+    // Check that we will try to upload the huge record next time
+    do_check_eq(engine._tracker.changedIDs["flying"], 1000);
   } finally {
+    // Check we didn't upload the oversized record to the server
+    do_check_eq(collection.payload("flying"), undefined);
     await cleanAndGo(engine, server);
   }
+}
+
+
+add_task(async function test_uploadOutgoing_max_record_payload_bytes_disallowSkippedRecords() {
+  return test_uploadOutgoing_max_record_payload_bytes(false);
+});
+
+
+add_task(async function test_uploadOutgoing_max_record_payload_bytes_allowSkippedRecords() {
+  return test_uploadOutgoing_max_record_payload_bytes(true);
 });
 
 

services/sync/tests/unit/test_tab_store.js

@@ -93,7 +93,7 @@ function test_createRecord() {
   store.shouldSkipWindow = mockShouldSkipWindow;
   store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, "http://foo.com", 1, 1);
 
-  let numtabs = Math.ceil(20000. / 77.);
+  let numtabs = 2600; // Note: this number is connected to DEFAULT_MAX_RECORD_PAYLOAD_BYTES
 
   store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, "http://foo.com", 1, 1);
   record = store.createRecord("fake-guid");
@@ -104,7 +104,7 @@ function test_createRecord() {
   store.getWindowEnumerator = mockGetWindowEnumerator.bind(this, "http://foo.com", 1, numtabs);
   record = store.createRecord("fake-guid");
   ok(record instanceof TabSetRecord);
-  equal(record.tabs.length, 256);
+  equal(record.tabs.length, 2501);
 }
 
 function run_test() {