Bug 616265: Add download limit for history, lift magic number. r=mconnor
☠☠ backed out by 84fe21efcb57 ☠ ☠
authorRichard Newman <rnewman@mozilla.com>
Mon, 06 Dec 2010 17:25:35 -0800
changeset 58765 680d3557cafee52e1db57829238e61c8ee3ee42f
parent 58764 13ea9fc20ed2c9390e0dd81a1782b576492f5d6e
child 58766 4d16d58becaf3ae909de7ab2bd1b67d58e49806d
child 58797 f11c4d60b43cacb5650008d8991e69b69d54f023
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
reviewersmconnor
bugs616265
Bug 616265: Add download limit for history, lift magic number. r=mconnor
services/sync/modules/constants.js
services/sync/modules/engines.js
services/sync/modules/engines/history.js
services/sync/tests/unit/head_helpers.js
services/sync/tests/unit/head_http_server.js
services/sync/tests/unit/test_history_engine.js
services/sync/tests/unit/test_history_tracker.js
services/sync/tests/unit/test_syncengine_sync.js
--- a/services/sync/modules/constants.js
+++ b/services/sync/modules/constants.js
@@ -96,16 +96,18 @@ MODE_TRUNCATE:                         0
 PERMS_FILE:                            0644,
 PERMS_PASSFILE:                        0600,
 PERMS_DIRECTORY:                       0755,
 
 // Number of records to upload in a single POST (multiple POSTS if exceeded)
 // FIXME: Record size limit is 256k (new cluster), so this can be quite large!
 // (Bug 569295)
 MAX_UPLOAD_RECORDS:                    100,
+MAX_HISTORY_UPLOAD:                    5000,
+MAX_HISTORY_DOWNLOAD:                  5000,
 
 // Top-level statuses:
 STATUS_OK:                             "success.status_ok",
 SYNC_FAILED:                           "error.sync.failed",
 LOGIN_FAILED:                          "error.login.failed",
 SYNC_FAILED_PARTIAL:                   "error.sync.failed_partial",
 CLIENT_NOT_CONFIGURED:                 "service.client_not_configured",
 STATUS_DISABLED:                       "service.disabled",
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -134,16 +134,17 @@ EngineManagerSvc.prototype = {
       name = val.name;
     delete this._engines[name];
   }
 };
 
 function Engine(name) {
   this.Name = name || "Unnamed";
   this.name = name.toLowerCase();
+  this.downloadLimit = null;
 
   this._notify = Utils.notify("weave:engine:");
   this._log = Log4Moz.repository.getLogger("Engine." + this.Name);
   let level = Svc.Prefs.get("log.logger.engine." + this.name, "Debug");
   this._log.level = Log4Moz.Level[level];
 
   this._tracker; // initialize tracker to load previously changed IDs
   this._log.debug("Engine initialized");
@@ -490,17 +491,23 @@ SyncEngine.prototype = {
         throw resp;
       }
     }
 
     // Mobile: check if we got the maximum that we requested; get the rest if so.
     let toFetch = [];
     if (handled.length == newitems.limit) {
       let guidColl = new Collection(this.engineURL);
+      
+      // Sort and limit so that on mobile we only get the last X records.
+      guidColl.limit = this.downloadLimit;
       guidColl.newer = this.lastSync;
+      
+      // index: Orders by the sortindex descending (highest weight first).
+      guidColl.sort  = "index";
 
       let guids = guidColl.get();
       if (!guids.success)
         throw guids;
 
       // Figure out which guids weren't just fetched then remove any guids that
       // were already waiting and prepend the new ones
       let extra = Utils.arraySub(guids.obj, handled);
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -14,16 +14,17 @@
  * The Original Code is Weave
  *
  * The Initial Developer of the Original Code is Mozilla.
  * Portions created by the Initial Developer are Copyright (C) 2008
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
  *  Dan Mills <thunder@mozilla.com>
+ *  Richard Newman <rnewman@mozilla.com>
  *
  * Alternatively, the contents of this file may be used under the terms of
  * either the GNU General Public License Version 2 or later (the "GPL"), or
  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
  * in which case the provisions of the GPL or the LGPL are applicable instead
  * of those above. If you wish to allow use of your version of this file only
  * under the terms of either the GPL or the LGPL, and not to allow others to
  * use your version of this file under the terms of the MPL, indicate your
@@ -38,25 +39,27 @@ const EXPORTED_SYMBOLS = ['HistoryEngine
 
 const Cc = Components.classes;
 const Ci = Components.interfaces;
 const Cu = Components.utils;
 
 const GUID_ANNO = "sync/guid";
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/stores.js");
 Cu.import("resource://services-sync/trackers.js");
 Cu.import("resource://services-sync/type_records/history.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/log4moz.js");
 
 function HistoryEngine() {
   SyncEngine.call(this, "History");
+  this.downloadLimit = MAX_HISTORY_DOWNLOAD;
 }
 HistoryEngine.prototype = {
   __proto__: SyncEngine.prototype,
   _recordObj: HistoryRec,
   _storeObj: HistoryStore,
   _trackerObj: HistoryTracker,
 
   _sync: Utils.batchSync("History", SyncEngine),
@@ -317,17 +320,17 @@ HistoryStore.prototype = {
   changeItemID: function HStore_changeItemID(oldID, newID) {
     this.setGUID(this._findURLByGUID(oldID).url, newID);
   },
 
 
   getAllIDs: function HistStore_getAllIDs() {
     // Only get places visited within the last 30 days (30*24*60*60*1000ms)
     this._allUrlStm.params.cutoff_date = (Date.now() - 2592000000) * 1000;
-    this._allUrlStm.params.max_results = 5000;
+    this._allUrlStm.params.max_results = MAX_HISTORY_UPLOAD;
 
     let urls = Utils.queryAsync(this._allUrlStm, "url");
     let self = this;
     return urls.reduce(function(ids, item) {
       ids[self.GUIDForUri(item.url, true)] = item.url;
       return ids;
     }, {});
   },
--- a/services/sync/tests/unit/head_helpers.js
+++ b/services/sync/tests/unit/head_helpers.js
@@ -387,8 +387,24 @@ function SyncTestingInfrastructure(engin
  * @usage _("Hello World") -> prints "Hello World"
  * @usage _(1, 2, 3) -> prints "1 2 3"
  */
 let _ = function(some, debug, text, to) print(Array.slice(arguments).join(" "));
 
 _("Setting the identity for passphrase");
 Cu.import("resource://services-sync/identity.js");
 
+
+/*
+ * Test setup helpers.
+ */
+
+// Turn WBO cleartext into "encrypted" payload as it goes over the wire
+function encryptPayload(cleartext) {
+  if (typeof cleartext == "object") {
+    cleartext = JSON.stringify(cleartext);
+  }
+
+  return {ciphertext: cleartext, // ciphertext == cleartext with fake crypto
+          IV: "irrelevant",
+          hmac: Utils.sha256HMAC(cleartext, Utils.makeHMACKey(""))};
+}
+
--- a/services/sync/tests/unit/head_http_server.js
+++ b/services/sync/tests/unit/head_http_server.js
@@ -254,8 +254,17 @@ ServerCollection.prototype = {
       }
       response.setHeader('X-Weave-Timestamp', ''+Date.now()/1000, false);
       response.setStatusLine(request.httpVersion, statusCode, status);
       response.bodyOutputStream.write(body, body.length);
     };
   }
 
 };
+
+/*
+ * Test setup helpers.
+ */
+function sync_httpd_setup(handlers) {
+  handlers["/1.0/foo/storage/meta/global"]
+      = (new ServerWBO('global', {})).handler();
+  return httpd_setup(handlers);
+}
new file mode 100644
--- /dev/null
+++ b/services/sync/tests/unit/test_history_engine.js
@@ -0,0 +1,143 @@
+Cu.import("resource://services-sync/base_records/wbo.js");
+Cu.import("resource://services-sync/base_records/crypto.js");
+Cu.import("resource://services-sync/engines/history.js");
+Cu.import("resource://services-sync/type_records/history.js");
+Cu.import("resource://services-sync/constants.js");
+Cu.import("resource://services-sync/engines.js");
+Cu.import("resource://services-sync/identity.js");
+Cu.import("resource://services-sync/util.js");
+
+function makeSteamEngine() {
+  return new SteamEngine();
+}
+
+var syncTesting = new SyncTestingInfrastructure(makeSteamEngine);
+
+function test_processIncoming_mobile_history_batched() {
+  _("SyncEngine._processIncoming works on history engine.");
+
+  let FAKE_DOWNLOAD_LIMIT = 100;
+  
+  Svc.Prefs.set("clusterURL", "http://localhost:8080/");
+  Svc.Prefs.set("username", "foo");
+  Svc.Prefs.set("client.type", "mobile");
+  Svc.History.removeAllPages();
+  Engines.register(HistoryEngine);
+
+  // A collection that logs each GET
+  let collection = new ServerCollection();
+  collection.get_log = [];
+  collection._get = collection.get;
+  collection.get = function (options) {
+    this.get_log.push(options);
+    return this._get(options);
+  };
+
+  // Let's create some 234 server side history records. They're all at least
+  // 10 minutes old.
+  let visitType = Ci.nsINavHistoryService.TRANSITION_LINK;
+  for (var i = 0; i < 234; i++) {
+    let id = 'record-no-' + i;
+    let modified = Date.now()/1000 - 60*(i+10);
+    let payload = encryptPayload({
+      id: id,
+      histUri: "http://foo/bar?" + id,
+        title: id,
+        sortindex: i,
+        visits: [{date: (modified - 5), type: visitType}],
+        deleted: false});
+    
+    let wbo = new ServerWBO(id, payload);
+    wbo.modified = modified;
+    collection.wbos[id] = wbo;
+  }
+  
+  let server = sync_httpd_setup({
+      "/1.0/foo/storage/history": collection.handler()
+  });
+  do_test_pending();
+
+  let engine = new HistoryEngine("history");
+  let meta_global = Records.set(engine.metaURL, new WBORecord(engine.metaURL));
+  meta_global.payload.engines = {history: {version: engine.version,
+                                           syncID: engine.syncID}};
+
+  try {
+
+    _("On a mobile client, we get new records from the server in batches of 50.");
+    engine._syncStartup();
+    
+    // Fake a lower limit.
+    engine.downloadLimit = FAKE_DOWNLOAD_LIMIT;
+    _("Last modified: " + engine.lastModified);
+    _("Processing...");
+    engine._processIncoming();
+    
+    _("Last modified: " + engine.lastModified);
+    engine._syncFinish();
+    
+    // Back to the normal limit.
+    _("Running again. Should fetch none, because of lastModified");
+    engine.downloadLimit = MAX_HISTORY_DOWNLOAD;
+    _("Processing...");
+    engine._processIncoming();
+    
+    _("Last modified: " + engine.lastModified);
+    _("Running again. Expecting to pull everything");
+    
+    engine.lastModified = undefined;
+    engine.lastSync     = 0;
+    _("Processing...");
+    engine._processIncoming();
+    
+    _("Last modified: " + engine.lastModified);
+
+    // Verify that the right number of GET requests with the right
+    // kind of parameters were made.
+    do_check_eq(collection.get_log.length,
+        // First try:
+        1 +    // First 50...
+        1 +    // 1 GUID fetch...
+               // 1 fetch...
+        Math.ceil((FAKE_DOWNLOAD_LIMIT - 50) / MOBILE_BATCH_SIZE) +
+        // Second try: none
+        // Third try:
+        1 +    // First 50...
+        1 +    // 1 GUID fetch...
+               // 4 fetch...
+        Math.ceil((234 - 50) / MOBILE_BATCH_SIZE));
+    
+    // Check the structure of each HTTP request.
+    do_check_eq(collection.get_log[0].full, 1);
+    do_check_eq(collection.get_log[0].limit, MOBILE_BATCH_SIZE);
+    do_check_eq(collection.get_log[1].full, undefined);
+    do_check_eq(collection.get_log[1].sort, "index");
+    do_check_eq(collection.get_log[1].limit, FAKE_DOWNLOAD_LIMIT);
+    do_check_eq(collection.get_log[2].full, 1);
+    do_check_eq(collection.get_log[3].full, 1);
+    do_check_eq(collection.get_log[3].limit, MOBILE_BATCH_SIZE);
+    do_check_eq(collection.get_log[4].full, undefined);
+    do_check_eq(collection.get_log[4].sort, "index");
+    do_check_eq(collection.get_log[4].limit, MAX_HISTORY_DOWNLOAD);
+    for (let i = 0; i <= Math.floor((234 - 50) / MOBILE_BATCH_SIZE); i++) {
+      let j = i + 5;
+      do_check_eq(collection.get_log[j].full, 1);
+      do_check_eq(collection.get_log[j].limit, undefined);
+      if (i < Math.floor((234 - 50) / MOBILE_BATCH_SIZE))
+        do_check_eq(collection.get_log[j].ids.length, MOBILE_BATCH_SIZE);
+      else
+        do_check_eq(collection.get_log[j].ids.length, 234 % MOBILE_BATCH_SIZE);
+    }
+
+  } finally {
+    server.stop(do_test_finished);
+    Svc.Prefs.resetBranch("");
+    Records.clearCache();
+  }
+}
+
+function run_test() {
+  CollectionKeys.generateNewKeys();
+
+  test_processIncoming_mobile_history_batched();
+}
--- a/services/sync/tests/unit/test_history_tracker.js
+++ b/services/sync/tests/unit/test_history_tracker.js
@@ -13,17 +13,17 @@ function run_test() {
   let _counter = 0;
   function addVisit() {
     Svc.History.addVisit(Utils.makeURI("http://getfirefox.com/" + _counter),
                          Date.now() * 1000, null, 1, false, 0);
     _counter += 1;
   }
 
   try {
-    _("Create bookmark. Won't show because we haven't started tracking yet");
+    _("Create history item. Won't show because we haven't started tracking yet");
     addVisit();
     do_check_eq([id for (id in tracker.changedIDs)].length, 0);
 
     _("Tell the tracker to start tracking changes.");
     Svc.Obs.notify("weave:engine:start-tracking");
     addVisit();
     do_check_eq([id for (id in tracker.changedIDs)].length, 1);
 
--- a/services/sync/tests/unit/test_syncengine_sync.js
+++ b/services/sync/tests/unit/test_syncengine_sync.js
@@ -97,40 +97,16 @@ SteamEngine.prototype = {
 
 
 function makeSteamEngine() {
   return new SteamEngine();
 }
 
 var syncTesting = new SyncTestingInfrastructure(makeSteamEngine);
 
-
-/*
- * Test setup helpers
- */
-
-function sync_httpd_setup(handlers) {
-  handlers["/1.0/foo/storage/meta/global"]
-      = (new ServerWBO('global', {})).handler();
-  return httpd_setup(handlers);
-}
-
-// Turn WBO cleartext into "encrypted" payload as it goes over the wire
-function encryptPayload(cleartext) {
-  if (typeof cleartext == "object") {
-    cleartext = JSON.stringify(cleartext);
-  }
-
-  return {encryption: "../crypto/steam",
-          ciphertext: cleartext, // ciphertext == cleartext with fake crypto
-          IV: "irrelevant",
-          hmac: Utils.sha256HMAC(cleartext, null)};
-}
-
-
 /*
  * Tests
  * 
  * SyncEngine._sync() is divided into four rather independent steps:
  *
  * - _syncStartup()
  * - _processIncoming()
  * - _uploadOutgoing()
@@ -517,17 +493,17 @@ function test_processIncoming_mobile_bat
 
   let engine = makeSteamEngine();
   let meta_global = Records.set(engine.metaURL, new WBORecord(engine.metaURL));
   meta_global.payload.engines = {steam: {version: engine.version,
                                          syncID: engine.syncID}};
 
   try {
 
-    // On a mobile client, we get new records from the server in batches of 50.
+    _("On a mobile client, we get new records from the server in batches of 50.");
     engine._syncStartup();
     engine._processIncoming();
     do_check_eq([id for (id in engine._store.items)].length, 234);
     do_check_true('record-no-0' in engine._store.items);
     do_check_true('record-no-49' in engine._store.items);
     do_check_true('record-no-50' in engine._store.items);
     do_check_true('record-no-233' in engine._store.items);
 
@@ -551,17 +527,16 @@ function test_processIncoming_mobile_bat
   } finally {
     server.stop(do_test_finished);
     Svc.Prefs.resetBranch("");
     Records.clearCache();
     syncTesting = new SyncTestingInfrastructure(makeSteamEngine);
   }
 }
 
-
 function test_uploadOutgoing_toEmptyServer() {
   _("SyncEngine._uploadOutgoing uploads new records to server");
 
   Svc.Prefs.set("clusterURL", "http://localhost:8080/");
   Svc.Prefs.set("username", "foo");
   let collection = new ServerCollection();
   collection.wbos.flying = new ServerWBO('flying');
   collection.wbos.scotsman = new ServerWBO('scotsman');
@@ -1012,16 +987,18 @@ function test_canDecrypt_true() {
   }
 }
 
 
 function run_test() {
   if (DISABLE_TESTS_BUG_604565)
     return;
 
+  CollectionKeys.generateNewKeys();
+
   test_syncStartup_emptyOrOutdatedGlobalsResetsSync();
   test_syncStartup_serverHasNewerVersion();
   test_syncStartup_syncIDMismatchResetsClient();
   test_processIncoming_emptyServer();
   test_processIncoming_createFromServer();
   test_processIncoming_reconcile();
   test_processIncoming_mobile_batchSize();
   test_uploadOutgoing_toEmptyServer();