General restructure for performance improvements (bug 441907, r=thunder)
authorAnant Narayanan <anant@kix.in>
Fri, 27 Jun 2008 20:16:43 -0700
changeset 44804 2041ae2539375465671949bd18688f44a8f2d8a1
parent 44801 6ea092ee96bd4398401d7856f321fe89997fe041
child 44805 cd53ad616b2e7fe8d7da0146ca39487ec0a6e898
push id14033
push useredward.lee@engineering.uiuc.edu
push dateWed, 23 Jun 2010 22:21:35 +0000
treeherderautoland@227db4ad8cdf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersthunder
bugs441907
General restructure for performance improvements (bug 441907, r=thunder)
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
services/sync/modules/engines/cookies.js
services/sync/modules/engines/forms.js
services/sync/modules/engines/history.js
services/sync/modules/engines/passwords.js
services/sync/modules/engines/tabs.js
services/sync/modules/stores.js
services/sync/modules/syncCores.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -125,30 +125,30 @@ Engine.prototype = {
   get _json() {
     if (!this.__json)
       this.__json = Cc["@mozilla.org/dom/json;1"].
         createInstance(Ci.nsIJSON);
     return this.__json;
   },
 
   // _core, _store and _tracker need to be overridden in subclasses
-  __core: null,
-  get _core() {
-    if (!this.__core)
-      this.__core = new SyncCore();
-    return this.__core;
-  },
-
   __store: null,
   get _store() {
     if (!this.__store)
       this.__store = new Store();
     return this.__store;
   },
 
+  __core: null,
+  get _core() {
+    if (!this.__core)
+      this.__core = new SyncCore(this._store);
+    return this.__core;
+  },
+
   __tracker: null,
   get _tracker() {
     if (!this.__tracker)
       this.__tracker = new Tracker();
     return this.__tracker;
   },
 
   __snapshot: null,
@@ -259,16 +259,17 @@ Engine.prototype = {
 
     this._log.info("Local snapshot version: " + this._snapshot.version);
     this._log.info("Server maxVersion: " + this._remote.status.data.maxVersion);
 
     if ("none" != Utils.prefs.getCharPref("encryption"))
       yield this._remote.keys.getKeyAndIV(self.cb, this.engineId);
 
     // 1) Fetch server deltas
+    
     let server = {};
     let serverSnap = yield this._remote.wrap(self.cb, this._snapshot);
     server.snapshot = serverSnap.data;
     this._core.detectUpdates(self.cb, this._snapshot.data, server.snapshot);
     server.updates = yield;
 
     // 2) Generate local deltas from snapshot -> current client status
 
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -623,30 +623,30 @@ BookmarksSharingManager.prototype = {
 function BookmarksEngine(pbeId) {
   this._init(pbeId);
 }
 BookmarksEngine.prototype = {
   get name() { return "bookmarks"; },
   get logName() { return "BmkEngine"; },
   get serverPrefix() { return "user-data/bookmarks/"; },
 
-  __core: null,
-  get _core() {
-    if (!this.__core)
-      this.__core = new BookmarksSyncCore();
-    return this.__core;
-  },
-
   __store: null,
   get _store() {
     if (!this.__store)
       this.__store = new BookmarksStore();
     return this.__store;
   },
 
+  __core: null,
+  get _core() {
+    if (!this.__core)
+      this.__core = new BookmarksSyncCore(this._store);
+    return this.__core;
+  },
+
   __tracker: null,
   get _tracker() {
     if (!this.__tracker)
       this.__tracker = new BookmarksTracker();
     return this.__tracker;
   },
 
   __sharing: null,
@@ -680,33 +680,23 @@ BookmarksEngine.prototype = {
     let self = yield;
     this._sharing._stopSharing.async( this._sharing, self.cb, guid, username);
     yield;
     self.done();
   }
 };
 BookmarksEngine.prototype.__proto__ = new Engine();
 
-function BookmarksSyncCore() {
+function BookmarksSyncCore(store) {
+  this._store = store;
   this._init();
 }
 BookmarksSyncCore.prototype = {
   _logName: "BMSync",
-
-  __bms: null,
-  get _bms() {
-    if (!this.__bms)
-      this.__bms = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
-                   getService(Ci.nsINavBookmarksService);
-    return this.__bms;
-  },
-
-  _itemExists: function BSC__itemExists(GUID) {
-    return this._bms.getItemIdForGUID(GUID) >= 0;
-  },
+  _store: null,
 
   _getEdits: function BSC__getEdits(a, b) {
     // NOTE: we do not increment ret.numProps, as that would cause
     // edit commands to always get generated
     let ret = SyncCore.prototype._getEdits.call(this, a, b);
     ret.props.type = a.type;
     return ret;
   },
@@ -783,16 +773,17 @@ BookmarksSyncCore.prototype = {
 };
 BookmarksSyncCore.prototype.__proto__ = new SyncCore();
 
 function BookmarksStore() {
   this._init();
 }
 BookmarksStore.prototype = {
   _logName: "BStore",
+  _lookup: null,
 
   __bms: null,
   get _bms() {
     if (!this.__bms)
       this.__bms = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
                    getService(Ci.nsINavBookmarksService);
     return this.__bms;
   },
@@ -845,17 +836,17 @@ BookmarksStore.prototype = {
       return this._bms.toolbarFolder;
     case "unfiled":
       return this._bms.unfiledBookmarksFolder;
     default:
       return this._bms.getItemIdForGUID(GUID);
     }
     return null;
   },
-
+  
   _createCommand: function BStore__createCommand(command) {
     let newId;
     let parentId = this._getItemIdForGUID(command.data.parentGUID);
 
     if (parentId < 0) {
       this._log.warn("Creating node with unknown parent -> reparenting to root");
       parentId = this._bms.bookmarksMenuFolder;
     }
@@ -1247,16 +1238,17 @@ BookmarksStore.prototype = {
     return ret;
   },
 
   wrap: function BStore_wrap() {
     var items = {};
     this._wrap(this._getNode(this._bms.bookmarksMenuFolder), items, "menu");
     this._wrap(this._getNode(this._bms.toolbarFolder), items, "toolbar");
     this._wrap(this._getNode(this._bms.unfiledBookmarksFolder), items, "unfiled");
+    this._lookup = items;
     return items;
   },
 
   wipe: function BStore_wipe() {
     this._bms.removeFolderChildren(this._bms.bookmarksMenuFolder);
     this._bms.removeFolderChildren(this._bms.toolbarFolder);
     this._bms.removeFolderChildren(this._bms.unfiledBookmarksFolder);
   },
--- a/services/sync/modules/engines/cookies.js
+++ b/services/sync/modules/engines/cookies.js
@@ -49,94 +49,46 @@ Cu.import("resource://weave/trackers.js"
 function CookieEngine(pbeId) {
   this._init(pbeId);
 }
 CookieEngine.prototype = {
   get name() { return "cookies"; },
   get logName() { return "CookieEngine"; },
   get serverPrefix() { return "user-data/cookies/"; },
 
-  __core: null,
-  get _core() {
-    if (!this.__core)
-      this.__core = new CookieSyncCore();
-    return this.__core;
-  },
-
   __store: null,
   get _store() {
     if (!this.__store)
       this.__store = new CookieStore();
     return this.__store;
   },
 
+  __core: null,
+  get _core() {
+    if (!this.__core)
+      this.__core = new CookieSyncCore(this._store);
+    return this.__core;
+  },
+
   __tracker: null,
   get _tracker() {
     if (!this.__tracker)
       this.__tracker = new CookieTracker();
     return this.__tracker;
   }
 };
 CookieEngine.prototype.__proto__ = new Engine();
 
-function CookieSyncCore() {
+function CookieSyncCore(store) {
+  this._store = store;
   this._init();
 }
 CookieSyncCore.prototype = {
   _logName: "CookieSync",
-
-  __cookieManager: null,
-  get _cookieManager() {
-    if (!this.__cookieManager)
-      this.__cookieManager = Cc["@mozilla.org/cookiemanager;1"].
-                             getService(Ci.nsICookieManager2);
-    /* need the 2nd revision of the ICookieManager interface
-       because it supports add() and the 1st one doesn't. */
-    return this.__cookieManager;
-  },
-
-
-  _itemExists: function CSC__itemExists(GUID) {
-    /* true if a cookie with the given GUID exists.
-       The GUID that we are passed should correspond to the keys
-       that we define in the JSON returned by CookieStore.wrap()
-       That is, it will be a string of the form
-       "host:path:name". */
-
-    /* TODO verify that colons can't normally appear in any of
-       the fields -- if they did it then we can't rely on .split(":")
-       to parse correctly.*/
-
-    let cookieArray = GUID.split( ":" );
-    let cookieHost = cookieArray[0];
-    let cookiePath = cookieArray[1];
-    let cookieName = cookieArray[2];
-
-    /* alternate implementation would be to instantiate a cookie from
-       cookieHost, cookiePath, and cookieName, then call
-       cookieManager.cookieExists(). Maybe that would have better
-       performance?  This implementation seems pretty slow.*/
-    let enumerator = this._cookieManager.enumerator;
-    while (enumerator.hasMoreElements())
-      {
-	let aCookie = enumerator.getNext();
-	if (aCookie.host == cookieHost &&
-	    aCookie.path == cookiePath &&
-	    aCookie.name == cookieName ) {
-	  return true;
-	}
-      }
-    return false;
-    /* Note: We can't just call cookieManager.cookieExists() with a generic
-       javascript object with .host, .path, and .name attributes attatched.
-       cookieExists is implemented in C and does a hard static_cast to an
-       nsCookie object, so duck typing doesn't work (and in fact makes
-       Firefox hard-crash as the static_cast returns null and is not checked.)
-    */
-  },
+  _store: null,
 
   _commandLike: function CSC_commandLike(a, b) {
     /* Method required to be overridden.
        a and b each have a .data and a .GUID
        If this function returns true, an editCommand will be
        generated to try to resolve the thing.
        but are a and b objects of the type in the Store or
        are they "commands"?? */
@@ -150,17 +102,17 @@ function CookieStore( cookieManagerStub 
      Mozilla cookie manager component.  This is the normal way to use this
      class in production code.  But for unit-testing purposes, you can pass
      in a stub object that will be used in place of the cookieManager. */
   this._init();
   this._cookieManagerStub = cookieManagerStub;
 }
 CookieStore.prototype = {
   _logName: "CookieStore",
-
+  _lookup: null,
 
   // Documentation of the nsICookie interface says:
   // name 	ACString 	The name of the cookie. Read only.
   // value 	ACString 	The cookie value. Read only.
   // isDomain 	boolean 	True if the cookie is a domain cookie, false otherwise. Read only.
   // host 	AUTF8String 	The host (possibly fully qualified) of the cookie. Read only.
   // path 	AUTF8String 	The path pertaining to the cookie. Read only.
   // isSecure 	boolean 	True if the cookie was transmitted over ssl, false otherwise. Read only.
@@ -181,17 +133,17 @@ CookieStore.prototype = {
     // otherwise, use the real one
     if (!this.__cookieManager)
       this.__cookieManager = Cc["@mozilla.org/cookiemanager;1"].
                              getService(Ci.nsICookieManager2);
     // need the 2nd revision of the ICookieManager interface
     // because it supports add() and the 1st one doesn't.
     return this.__cookieManager;
   },
-
+  
   _createCommand: function CookieStore__createCommand(command) {
     /* we got a command to create a cookie in the local browser
        in order to sync with the server. */
 
     this._log.info("CookieStore got createCommand: " + command );
     // this assumes command.data fits the nsICookie2 interface
     if ( !command.data.isSession ) {
       // Add only persistent cookies ( not session cookies )
@@ -270,49 +222,49 @@ CookieStore.prototype = {
     // Also, there's an exception raised because
     // this._data[comand.GUID] is undefined
   },
 
   wrap: function CookieStore_wrap() {
     /* Return contents of this store, as JSON.
        A dictionary of cookies where the keys are GUIDs and the
        values are sub-dictionaries containing all cookie fields. */
-
     let items = {};
     var iter = this._cookieManager.enumerator;
-    while (iter.hasMoreElements()){
+    while (iter.hasMoreElements()) {
       var cookie = iter.getNext();
-      if (cookie.QueryInterface( Ci.nsICookie )){
-	// String used to identify cookies is
-	// host:path:name
-	if ( cookie.isSession ) {
-	  /* Skip session-only cookies, sync only persistent cookies. */
-	  continue;
-	}
+      if (cookie.QueryInterface( Ci.nsICookie )) {
+	      // String used to identify cookies is
+	      // host:path:name
+	      if ( cookie.isSession ) {
+	        /* Skip session-only cookies, sync only persistent cookies. */
+	        continue;
+	      }
 
-	let key = cookie.host + ":" + cookie.path + ":" + cookie.name;
-	items[ key ] = { parentGUID: '',
-			 name: cookie.name,
-			 value: cookie.value,
-			 isDomain: cookie.isDomain,
-			 host: cookie.host,
-			 path: cookie.path,
-			 isSecure: cookie.isSecure,
-			 // nsICookie2 values:
-			 rawHost: cookie.rawHost,
-			 isSession: cookie.isSession,
-			 expiry: cookie.expiry,
-			 isHttpOnly: cookie.isHttpOnly };
+	      let key = cookie.host + ":" + cookie.path + ":" + cookie.name;
+	      items[ key ] = { parentGUID: '',
+			                    name: cookie.name,
+			                    value: cookie.value,
+			                    isDomain: cookie.isDomain,
+			                    host: cookie.host,
+			                    path: cookie.path,
+			                    isSecure: cookie.isSecure,
+			                    // nsICookie2 values:
+			                    rawHost: cookie.rawHost,
+			                    isSession: cookie.isSession,
+			                    expiry: cookie.expiry,
+			                    isHttpOnly: cookie.isHttpOnly };
 
-	/* See http://developer.mozilla.org/en/docs/nsICookie
-	   Note: not syncing "expires", "status", or "policy"
-	   since they're deprecated. */
+	      /* See http://developer.mozilla.org/en/docs/nsICookie
+	         Note: not syncing "expires", "status", or "policy"
+	         since they're deprecated. */
 
       }
     }
+    this._lookup = items;
     return items;
   },
 
   wipe: function CookieStore_wipe() {
     /* Remove everything from the store.  Return nothing.
        TODO are the semantics of this just wiping out an internal
        buffer, or am I supposed to wipe out all cookies from
        the browser itself for reals? */
--- a/services/sync/modules/engines/forms.js
+++ b/services/sync/modules/engines/forms.js
@@ -42,150 +42,69 @@ const Cu = Components.utils;
 
 Cu.import("resource://weave/log4moz.js");
 Cu.import("resource://weave/util.js");
 Cu.import("resource://weave/engines.js");
 Cu.import("resource://weave/syncCores.js");
 Cu.import("resource://weave/stores.js");
 Cu.import("resource://weave/trackers.js");
 
-/*
- * Generate GUID from a name,value pair.
- * If the concatenated length is less than 40, we just Base64 the JSON.
- * Otherwise, we Base64 the JSON of the name and SHA1 of the value.
- * The first character of the key determines which method we used:
- * '0' for full Base64, '1' for SHA-1'ed val.
- */
-function _generateFormGUID(nam, val) {
-  var key;
-  var con = nam + val;
-  
-  var jso = Cc["@mozilla.org/dom/json;1"].
-            createInstance(Ci.nsIJSON);
-
-  if (con.length <= 40) {
-    key = '0' + btoa(jso.encode([nam, val]));
-  } else {
-    val = Utils.sha1(val);
-    key = '1' + btoa(jso.encode([nam, val]));
-  }
-  
-  return key;
-}
-
-/*
- * Unwrap a name,value pair from a GUID.
- * Return an array [sha1ed, name, value]
- * sha1ed is a boolean determining if the value is SHA-1'ed or not.
- */
-function _unwrapFormGUID(guid) {
-  var jso = Cc["@mozilla.org/dom/json;1"].
-            createInstance(Ci.nsIJSON);
-  
-  var ret;
-  var dec = atob(guid.slice(1));
-  var obj = jso.decode(dec);
-  
-  switch (guid[0]) {
-    case '0':
-      ret = [false, obj[0], obj[1]];
-      break;
-    case '1':
-      ret = [true, obj[0], obj[1]];
-      break;
-    default:
-      this._log.warn("Unexpected GUID header: " + guid[0] + ", aborting!");
-      return false;
-  }
-  
-  return ret;
-}
-
 function FormEngine(pbeId) {
   this._init(pbeId);
 }
 FormEngine.prototype = {
   get name() { return "forms"; },
   get logName() { return "FormEngine"; },
   get serverPrefix() { return "user-data/forms/"; },
 
-  __core: null,
-  get _core() {
-    if (!this.__core)
-      this.__core = new FormSyncCore();
-    return this.__core;
-  },
-
   __store: null,
   get _store() {
     if (!this.__store)
       this.__store = new FormStore();
     return this.__store;
   },
 
+  __core: null,
+  get _core() {
+    if (!this.__core)
+      this.__core = new FormSyncCore(this._store);
+    return this.__core;
+  },
+
   __tracker: null,
   get _tracker() {
     if (!this.__tracker)
       this.__tracker = new FormsTracker();
     return this.__tracker;
   }
 };
 FormEngine.prototype.__proto__ = new Engine();
 
-function FormSyncCore() {
+function FormSyncCore(store) {
+  this._store = store;
   this._init();
 }
 FormSyncCore.prototype = {
   _logName: "FormSync",
-
-  __formDB: null,
-  get _formDB() {
-    if (!this.__formDB) {
-      var file = Cc["@mozilla.org/file/directory_service;1"].
-                 getService(Ci.nsIProperties).
-                 get("ProfD", Ci.nsIFile);
-      file.append("formhistory.sqlite");
-      var stor = Cc["@mozilla.org/storage/service;1"].
-                 getService(Ci.mozIStorageService);
-      this.__formDB = stor.openDatabase(file);
-    }
-    return this.__formDB;
-  },
-
-  _itemExists: function FSC__itemExists(GUID) {
-    var found = false;
-    var stmnt = this._formDB.createStatement("SELECT * FROM moz_formhistory");
-
-    /* Same performance restrictions as PasswordSyncCore apply here:
-       caching required */
-    while (stmnt.executeStep()) {
-      var nam = stmnt.getUTF8String(1);
-      var val = stmnt.getUTF8String(2);
-      var key = _generateFormGUID(nam, val);
-
-      if (key == GUID)
-        found = true;
-    }
-
-    return found;
-  },
+  _store: null,
 
   _commandLike: function FSC_commandLike(a, b) {
     /* Not required as GUIDs for similar data sets will be the same */
     return false;
   }
 };
 FormSyncCore.prototype.__proto__ = new SyncCore();
 
 function FormStore() {
   this._init();
 }
 FormStore.prototype = {
   _logName: "FormStore",
-
+  _lookup: null,
+  
   __formDB: null,
   get _formDB() {
     if (!this.__formDB) {
       var file = Cc["@mozilla.org/file/directory_service;1"].
                  getService(Ci.nsIProperties).
                  get("ProfD", Ci.nsIFile);
       file.append("formhistory.sqlite");
       var stor = Cc["@mozilla.org/storage/service;1"].
@@ -198,76 +117,57 @@ FormStore.prototype = {
   __formHistory: null,
   get _formHistory() {
     if (!this.__formHistory)
       this.__formHistory = Cc["@mozilla.org/satchel/form-history;1"].
                            getService(Ci.nsIFormHistory2);
     return this.__formHistory;
   },
 
-  _getValueFromSHA1: function FormStore__getValueFromSHA1(name, sha) {
-    var query = "SELECT value FROM moz_formhistory WHERE fieldname = '" + name + "'";
-    var stmnt = this._formDB.createStatement(query);
-    var found = false;
-    
-    while (stmnt.executeStep()) {
-      var val = stmnt.getUTF8String(0);
-      if (Utils.sha1(val) == sha) {
-        found = val;
-        break;
-      }
-    }
-    return found;
-  },
-  
   _createCommand: function FormStore__createCommand(command) {
     this._log.info("FormStore got createCommand: " + command);
     this._formHistory.addEntry(command.data.name, command.data.value);
   },
 
   _removeCommand: function FormStore__removeCommand(command) {
     this._log.info("FormStore got removeCommand: " + command);
     
-    var data = _unwrapFormGUID(command.GUID);
-    if (!data) {
+    var data;
+    if (command.GUID in this._lookup) {
+      data = this._lookup[command.GUID];
+    } else {
       this._log.warn("Invalid GUID found, ignoring remove request.");
       return;
     }
     
-    var nam = data[1];
-    var val = data[2];
-    if (data[0]) {
-      val = this._getValueFromSHA1(nam, val);
-    }
+    var nam = data.name;
+    var val = data.value;
+    this._formHistory.removeEntry(nam, val);
     
-    if (val) {
-      this._formHistory.removeEntry(nam, val);
-    } else {
-      this._log.warn("Form value not found from GUID, ignoring remove request.");
-    }
+    delete this._lookup[command.GUID];
   },
 
   _editCommand: function FormStore__editCommand(command) {
     this._log.info("FormStore got editCommand: " + command);
     this._log.warn("Form syncs are expected to only be create/remove!");
   },
 
   wrap: function FormStore_wrap() {
-    var items = [];
+    this._lookup = {};
     var stmnt = this._formDB.createStatement("SELECT * FROM moz_formhistory");
 
     while (stmnt.executeStep()) {
       var nam = stmnt.getUTF8String(1);
       var val = stmnt.getUTF8String(2);
-      var key = _generateFormGUID(nam, val);
+      var key = Utils.sha1(nam + val);
 
-      items[key] = { name: nam, value: val };
+      this._lookup[key] = { name: nam, value: val };
     }
-
-    return items;
+    
+    return this._lookup;
   },
 
   wipe: function FormStore_wipe() {
     this._formHistory.removeAllEntries();
   },
 
   resetGUIDs: function FormStore_resetGUIDs() {
     // Not needed.
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -53,49 +53,46 @@ Function.prototype.async = Async.sugar;
 function HistoryEngine(pbeId) {
   this._init(pbeId);
 }
 HistoryEngine.prototype = {
   get name() { return "history"; },
   get logName() { return "HistEngine"; },
   get serverPrefix() { return "user-data/history/"; },
 
-  __core: null,
-  get _core() {
-    if (!this.__core)
-      this.__core = new HistorySyncCore();
-    return this.__core;
-  },
-
   __store: null,
   get _store() {
     if (!this.__store)
       this.__store = new HistoryStore();
     return this.__store;
   },
 
+  __core: null,
+  get _core() {
+    if (!this.__core)
+      this.__core = new HistorySyncCore(this._store);
+    return this.__core;
+  },
+
   __tracker: null,
   get _tracker() {
     if (!this.__tracker)
       this.__tracker = new HistoryTracker();
     return this.__tracker;
   }
 };
 HistoryEngine.prototype.__proto__ = new Engine();
 
-function HistorySyncCore() {
+function HistorySyncCore(store) {
+  this._store = store;
   this._init();
 }
 HistorySyncCore.prototype = {
   _logName: "HistSync",
-
-  _itemExists: function HSC__itemExists(GUID) {
-    // we don't care about already-existing items; just try to re-add them
-    return false;
-  },
+  _store: null,
 
   _commandLike: function HSC_commandLike(a, b) {
     // History commands never qualify for likeness.  We will always
     // take the union of all client/server items.  We use the URL as
     // the GUID, so the same sites will map to the same item (same
     // GUID), without our intervention.
     return false;
   },
@@ -129,16 +126,21 @@ HistoryStore.prototype = {
       this.__hsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
                     getService(Ci.nsINavHistoryService);
       this.__hsvc.QueryInterface(Ci.nsIGlobalHistory2);
       this.__hsvc.QueryInterface(Ci.nsIBrowserHistory);
     }
     return this.__hsvc;
   },
 
+  _itemExists: function HistStore__itemExists(GUID) {
+    // we don't care about already-existing items; just try to re-add them
+    return false;
+  },
+
   _createCommand: function HistStore__createCommand(command) {
     this._log.debug("  -> creating history entry: " + command.GUID);
     try {
       let uri = Utils.makeURI(command.data.URI);
       this._hsvc.addVisit(uri, command.data.time, null,
                           this._hsvc.TRANSITION_TYPED, false, null);
       this._hsvc.setPageTitle(uri, command.data.title);
     } catch (e) {
--- a/services/sync/modules/engines/passwords.js
+++ b/services/sync/modules/engines/passwords.js
@@ -38,116 +38,96 @@ const EXPORTED_SYMBOLS = ['PasswordEngin
 
 const Cu = Components.utils;
 
 Cu.import("resource://weave/util.js");
 Cu.import("resource://weave/engines.js");
 Cu.import("resource://weave/syncCores.js");
 Cu.import("resource://weave/stores.js");
 
-/*
- * _hashLoginInfo
- *
- * nsILoginInfo objects don't have a unique GUID, so we need to generate one
- * on the fly. This is done by taking a hash of every field in the object.
- * Note that the resulting GUID could potentiually reveal passwords via
- * dictionary attacks or brute force. But GUIDs shouldn't be obtainable by
- * anyone, so this should generally be safe.
- */
-function _hashLoginInfo(aLogin) {
-  var loginKey = aLogin.hostname      + ":" +
-                 aLogin.formSubmitURL + ":" +
-                 aLogin.httpRealm     + ":" +
-                 aLogin.username      + ":" +
-                 aLogin.password      + ":" +
-                 aLogin.usernameField + ":" +
-                 aLogin.passwordField;
-
-  return Utils.sha1(loginKey);
-}
-
 function PasswordEngine() {
   this._init();
 }
 PasswordEngine.prototype = {
   get name() { return "passwords"; },
   get logName() { return "PasswordEngine"; },
   get serverPrefix() { return "user-data/passwords/"; },
 
-  __core: null,
-  get _core() {
-    if (!this.__core)
-      this.__core = new PasswordSyncCore();
-    return this.__core;
-  },
-
   __store: null,
   get _store() {
     if (!this.__store)
       this.__store = new PasswordStore();
     return this.__store;
+  },
+  
+  __core: null,
+  get _core() {
+    if (!this.__core)
+      this.__core = new PasswordSyncCore(this._store);
+    return this.__core;
   }
 };
 PasswordEngine.prototype.__proto__ = new Engine();
 
-function PasswordSyncCore() {
+function PasswordSyncCore(store) {
+  this._store = store;
   this._init();
 }
 PasswordSyncCore.prototype = {
   _logName: "PasswordSync",
-
-  __loginManager : null,
-  get _loginManager() {
-    if (!this.__loginManager)
-      this.__loginManager = Utils.getLoginManager();
-    return this.__loginManager;
-  },
-
-  _itemExists: function PSC__itemExists(GUID) {
-    var found = false;
-    var logins = this._loginManager.getAllLogins({});
-
-    // XXX It would be more efficient to compute all the hashes in one shot,
-    // cache the results, and check the cache here. That would need to happen
-    // once per sync -- not sure how to invalidate cache after current sync?
-    for (var i = 0; i < logins.length && !found; i++) {
-        var hash = _hashLoginInfo(logins[i]);
-        if (hash == GUID)
-            found = true;;
-    }
-
-    return found;
-  },
+  _store: null,
 
   _commandLike: function PSC_commandLike(a, b) {
     // Not used.
     return false;
   }
 };
 PasswordSyncCore.prototype.__proto__ = new SyncCore();
 
 function PasswordStore() {
   this._init();
 }
 PasswordStore.prototype = {
   _logName: "PasswordStore",
+  _lookup: null,
 
-  __loginManager : null,
+  __loginManager: null,
   get _loginManager() {
     if (!this.__loginManager)
       this.__loginManager = Utils.getLoginManager();
     return this.__loginManager;
   },
 
-  __nsLoginInfo : null,
+  __nsLoginInfo: null,
   get _nsLoginInfo() {
     if (!this.__nsLoginInfo)
       this.__nsLoginInfo = Utils.makeNewLoginInfo();
     return this.__nsLoginInfo;
   },
+  
+  /*
+   * _hashLoginInfo
+   *
+   * nsILoginInfo objects don't have a unique GUID, so we need to generate one
+   * on the fly. This is done by taking a hash of every field in the object.
+   * Note that the resulting GUID could potentiually reveal passwords via
+   * dictionary attacks or brute force. But GUIDs shouldn't be obtainable by
+   * anyone, so this should generally be safe.
+   */
+   _hashLoginInfo: function PasswordStore__hashLoginInfo(aLogin) {
+    var loginKey = aLogin.hostname      + ":" +
+                   aLogin.formSubmitURL + ":" +
+                   aLogin.httpRealm     + ":" +
+                   aLogin.username      + ":" +
+                   aLogin.password      + ":" +
+                   aLogin.usernameField + ":" +
+                   aLogin.passwordField;
+
+    return Utils.sha1(loginKey);
+  },
 
   _createCommand: function PasswordStore__createCommand(command) {
     this._log.info("PasswordStore got createCommand: " + command );
 
     var login = new this._nsLoginInfo(command.data.hostname,
                                       command.data.formSubmitURL,
                                       command.data.httpRealm,
                                       command.data.username,
@@ -175,33 +155,33 @@ PasswordStore.prototype = {
   _editCommand: function PasswordStore__editCommand(command) {
     this._log.info("PasswordStore got editCommand: " + command );
     throw "Password syncs are expected to only be create/remove!";
   },
 
   wrap: function PasswordStore_wrap() {
     /* Return contents of this store, as JSON. */
     var items = {};
-
     var logins = this._loginManager.getAllLogins({});
 
     for (var i = 0; i < logins.length; i++) {
       var login = logins[i];
 
-      var key = _hashLoginInfo(login);
+      var key = this._hashLoginInfo(login);
 
       items[key] = { hostname      : login.hostname,
                      formSubmitURL : login.formSubmitURL,
                      httpRealm     : login.httpRealm,
                      username      : login.username,
                      password      : login.password,
                      usernameField : login.usernameField,
                      passwordField : login.passwordField };
     }
 
+    this._lookup = items;
     return items;
   },
 
   wipe: function PasswordStore_wipe() {
     this._loginManager.removeAllLogins();
   },
 
   resetGUIDs: function PasswordStore_resetGUIDs() {
--- a/services/sync/modules/engines/tabs.js
+++ b/services/sync/modules/engines/tabs.js
@@ -57,75 +57,53 @@ function TabEngine(pbeId) {
 TabEngine.prototype = {
   __proto__: new Engine(),
 
   get name() "tabs",
   get logName() "TabEngine",
   get serverPrefix() "user-data/tabs/",
   get store() this._store,
 
-  get _core() {
-    let core = new TabSyncCore(this);
-    this.__defineGetter__("_core", function() core);
-    return this._core;
-  },
-
   get _store() {
     let store = new TabStore();
     this.__defineGetter__("_store", function() store);
     return this._store;
   },
+  
+  get _core() {
+    let core = new TabSyncCore(this._store);
+    this.__defineGetter__("_core", function() core);
+    return this._core;
+  },
 
   get _tracker() {
     let tracker = new TabTracker(this);
     this.__defineGetter__("_tracker", function() tracker);
     return this._tracker;
   }
 
 };
 
-function TabSyncCore(engine) {
-  this._engine = engine;
+function TabSyncCore(store) {
+  this._store = store;
   this._init();
 }
 TabSyncCore.prototype = {
   __proto__: new SyncCore(),
 
   _logName: "TabSync",
-
-  _engine: null,
+  _store: null,
 
   get _sessionStore() {
     let sessionStore = Cc["@mozilla.org/browser/sessionstore;1"].
 		       getService(Ci.nsISessionStore);
     this.__defineGetter__("_sessionStore", function() sessionStore);
     return this._sessionStore;
   },
 
-  _itemExists: function TSC__itemExists(GUID) {
-    // Note: this method returns true if the tab exists in any window, not just
-    // the window from which the tab came.  In the future, if we care about
-    // windows, we might need to make this more specific, although in that case
-    // we'll have to identify tabs by something other than URL, since even
-    // window-specific tabs look the same when identified by URL.
-
-    // Get the set of all real and virtual tabs.
-    let tabs = this._engine.store.wrap();
-
-    // XXX Should we convert both to nsIURIs and then use nsIURI::equals
-    // to compare them?
-    if (GUID in tabs) {
-      this._log.trace("_itemExists: " + GUID + " exists");
-      return true;
-    }
-
-    this._log.trace("_itemExists: " + GUID + " doesn't exist");
-    return false;
-  },
-
   _commandLike: function TSC_commandLike(a, b) {
     // Not implemented.
     return false;
   }
 };
 
 function TabStore() {
   this._virtualTabs = {};
@@ -258,16 +236,37 @@ TabStore.prototype = {
     this.__proto__.__proto__.applyCommands.async(this, self.cb, commandList);
     yield;
 
     this._saveVirtualTabs();
 
     self.done();
   },
 
+  _itemExists: function TabStore__itemExists(GUID) {
+    // Note: this method returns true if the tab exists in any window, not just
+    // the window from which the tab came.  In the future, if we care about
+    // windows, we might need to make this more specific, although in that case
+    // we'll have to identify tabs by something other than URL, since even
+    // window-specific tabs look the same when identified by URL.
+
+    // Get the set of all real and virtual tabs.
+    let tabs = this.wrap();
+
+    // XXX Should we convert both to nsIURIs and then use nsIURI::equals
+    // to compare them?
+    if (GUID in tabs) {
+      this._log.trace("_itemExists: " + GUID + " exists");
+      return true;
+    }
+
+    this._log.trace("_itemExists: " + GUID + " doesn't exist");
+    return false;
+  },
+
   _createCommand: function TabStore__createCommand(command) {
     this._log.debug("_createCommand: " + command.GUID);
 
     if (command.GUID in this._virtualTabs || command.GUID in this._wrapRealTabs())
       throw "trying to create a tab that already exists; id: " + command.GUID;
 
     // Cache the tab and notify the UI to prompt the user to open it.
     this._virtualTabs[command.GUID] = command.data;
--- a/services/sync/modules/stores.js
+++ b/services/sync/modules/stores.js
@@ -57,16 +57,19 @@ Function.prototype.async = Async.sugar;
 
 function Store() {
   this._init();
 }
 Store.prototype = {
   _logName: "Store",
   _yieldDuringApply: true,
 
+  // set this property in child object's wrap()!
+  _lookup: null,
+
   __json: null,
   get _json() {
     if (!this.__json)
       this.__json = Cc["@mozilla.org/dom/json;1"].
         createInstance(Ci.nsIJSON);
     return this.__json;
   },
 
@@ -97,20 +100,38 @@ Store.prototype = {
       default:
         this._log.error("unknown action in command: " + command["action"]);
         break;
       }
     }
     self.done();
   },
 
+  // override only if neccessary
+  _itemExists: function Store__itemExists(GUID) {
+    if (GUID in this._lookup)
+      return true;
+    else
+      return false;
+  },
+  
   // override these in derived objects
-  wrap: function Store_wrap() {},
-  wipe: function Store_wipe() {},
-  resetGUIDs: function Store_resetGUIDs() {}
+  
+  // wrap MUST save the wrapped store in the _lookup property!
+  wrap: function Store_wrap() {
+    throw "wrap needs to be subclassed";
+  },
+  
+  wipe: function Store_wipe() {
+    throw "wipe needs to be subclassed";
+  },
+  
+  resetGUIDs: function Store_resetGUIDs() {
+    throw "resetGUIDs needs to be subclassed";
+  }
 };
 
 function SnapshotStore(name) {
   this._init(name);
 }
 SnapshotStore.prototype = {
   _logName: "SnapStore",
 
--- a/services/sync/modules/syncCores.js
+++ b/services/sync/modules/syncCores.js
@@ -57,17 +57,20 @@ Function.prototype.async = Async.sugar;
  * itemExists.
  */
 
 function SyncCore() {
   this._init();
 }
 SyncCore.prototype = {
   _logName: "Sync",
-
+  
+  // Set this property in child objects!
+  _store: null,
+  
   _init: function SC__init() {
     this._log = Log4Moz.Service.getLogger("Service." + this._logName);
   },
 
   // FIXME: this won't work for deep objects, or objects with optional
   // properties
   _getEdits: function SC__getEdits(a, b) {
     let ret = {numProps: 0, props: {}};
@@ -204,21 +207,16 @@ SyncCore.prototype = {
       let cmdConflicts = function(elt) {
         return elt.GUID == commands[i].GUID;
       };
       if (!conflicts.some(cmdConflicts))
         propagations.push(commands[i]);
     }
   },
 
-  _itemExists: function SC__itemExists(GUID) {
-    this._log.error("itemExists needs to be subclassed");
-    return false;
-  },
-
   _reconcile: function SC__reconcile(listA, listB) {
     let self = yield;
 
     let propagations = [[], []];
     let conflicts = [[], []];
     let ret = {propagations: propagations, conflicts: conflicts};
     this._log.debug("Reconciling " + listA.length +
 		    " against " + listB.length + " commands");
@@ -248,17 +246,17 @@ SyncCore.prototype = {
       		      GUID: a.GUID,
       		      data: {GUID: b.GUID}});
           delete listA[i]; // a
           skip = true;
           return false; // b, but we add it back from guidChanges
         }
 
         // watch out for create commands with GUIDs that already exist
-        if (b.action == "create" && this._itemExists(b.GUID)) {
+        if (b.action == "create" && this._store._itemExists(b.GUID)) {
           this._log.error("Remote command has GUID that already exists " +
                           "locally. Dropping command.");
           return false; // delete b
         }
         return true; // keep b
       }, this);
     }