Bug 1303945 - Avoid having TPS write directly to the form history database and remove private-browsing tests. r=tcsc

MozReview-Commit-ID: JsPGSJpQ7b1

--HG--
extra : rebase_source : 42011ef085f1d119d43b14c2ac0619777fcb2c68

services/sync/modules/engines.js

@@ -107,7 +107,7 @@ Tracker.prototype = {
     Utils.jsonLoad("changes/" + this.file, this, function(json) {
       if (json && (typeof(json) == "object")) {
         this.changedIDs = json;
-      } else {
+      } else if (json !== null) {
         this._log.warn("Changed IDs file " + this.file + " contains non-object value.");
         json = null;
       }

services/sync/modules/util.js

@@ -95,7 +95,7 @@ this.Utils = {
         return func.call(thisArg);
       }
       catch(ex) {
-        thisArg._log.debug("Exception", ex);
+        thisArg._log.debug("Exception calling " + (func.name || "anonymous function"), ex);
         if (exceptionCallback) {
           return exceptionCallback.call(thisArg, ex);
         }
@@ -358,7 +358,8 @@ this.Utils = {
       json = yield CommonUtils.readJSON(path);
     } catch (e) {
       if (e instanceof OS.File.Error && e.becauseNoSuchFile) {
-        // Ignore non-existent files.
+        // Ignore non-existent files, but explicitly return null.
+        json = null;
       } else {
         if (that._log) {
           that._log.debug("Failed to load json", e);

services/sync/tests/tps/all_tests.json

@@ -16,7 +16,6 @@
     "test_bug575423.js",
     "test_bug546807.js",
     "test_history_collision.js",
-    "test_privbrw_formdata.js",
     "test_privbrw_passwords.js",
     "test_privbrw_tabs.js",
     "test_bookmarks_in_same_named_folder.js",

services/sync/tests/tps/test_formdata.js

@@ -31,6 +31,11 @@ var formdata1 = [
   }
 ];
 
+// This is currently pointless - it *looks* like it is trying to check that
+// one of the entries in formdata1 has been removed, but (a) the delete code
+// isn't active (see comments below), and (b) the way the verification works
+// means it would never do the right thing - it only checks all the entries
+// here exist, but not that they are the only entries in the DB.
 var formdata2 = [
   { fieldname: "testing",
     value: "success",
@@ -47,6 +52,11 @@ var formdata_delete = [
   }
 ];
 
+var formdata_new = [
+  { fieldname: "new-field",
+    value: "new-value"
+  }
+]
 /*
  * Test phases
  */
@@ -72,12 +82,15 @@ Phase('phase3', [
   [Formdata.delete, formdata_delete],
 //[Formdata.verifyNot, formdata_delete],
   [Formdata.verify, formdata2],
+  // add new data after the first Sync, ensuring the tracker works.
+  [Formdata.add, formdata_new],
   [Sync],
 ]);
 
 Phase('phase4', [
   [Sync],
   [Formdata.verify, formdata2],
+  [Formdata.verify, formdata_new],
 //[Formdata.verifyNot, formdata_delete]
 ]);
 

services/sync/tests/tps/test_privbrw_formdata.js

@@ -1,73 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-/*
- * The list of phases mapped to their corresponding profiles.  The object
- * here must be in strict JSON format, as it will get parsed by the Python
- * testrunner (no single quotes, extra comma's, etc).
- */
-EnableEngines(["forms"]);
-
-var phases = { "phase1": "profile1",
-               "phase2": "profile2",
-               "phase3": "profile1",
-               "phase4": "profile2" };
-
-/*
- * Form data
- */
-
-// the form data to add to the browser
-var formdata1 = [
-   { fieldname: "name",
-     value: "xyz",
-     date: -1
-   },
-   { fieldname: "email",
-     value: "abc@gmail.com",
-     date: -2
-   },
-   { fieldname: "username",
-     value: "joe"
-   }
-];
-
-// the form data to add in private browsing mode
-var formdata2 = [
-   { fieldname: "password",
-     value: "secret",
-     date: -1
-   },
-   { fieldname: "city",
-     value: "mtview"
-   }
-];
-
-/*
- * Test phases
- */
-
-Phase('phase1', [
-  [Formdata.add, formdata1],
-  [Formdata.verify, formdata1],
-  [Sync]
-]);
-
-Phase('phase2', [
-  [Sync],
-  [Formdata.verify, formdata1]
-]);
-
-Phase('phase3', [
-  [Sync],
-  [Windows.add, { private: true }],
-  [Formdata.add, formdata2],
-  [Formdata.verify, formdata2],
-  [Sync],
-]);
-
-Phase('phase4', [
-  [Sync],
-  [Formdata.verify, formdata1],
-  [Formdata.verifyNot, formdata2]
-]);

services/sync/tests/unit/test_service_sync_locked.js

@@ -31,7 +31,7 @@ function run_test() {
   Service.sync();
   Service._locked = false;
 
-  do_check_true(debug[debug.length - 2].startsWith("Exception: Could not acquire lock. Label: \"service.js: login\"."));
+  do_check_true(debug[debug.length - 2].startsWith("Exception calling WrappedLock: Could not acquire lock. Label: \"service.js: login\"."));
   do_check_eq(info[info.length - 1], "Cannot start sync: already syncing?");
 }
 

services/sync/tps/extensions/tps/resource/modules/forms.jsm

@@ -13,74 +13,45 @@ const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 Cu.import("resource://tps/logger.jsm");
 
-var formService = Cc["@mozilla.org/satchel/form-history;1"]
+Cu.import("resource://gre/modules/FormHistory.jsm");
-                  .getService(Ci.nsIFormHistory2);
+Cu.import("resource://gre/modules/Log.jsm");
 
 /**
  * FormDB
  *
- * Helper object containing methods to interact with the moz_formhistory
+ * Helper object containing methods to interact with the FormHistory module.
- * SQLite table.
  */
 var FormDB = {
-  /**
+  _update(data) {
-   * makeGUID
+    return new Promise((resolve, reject) => {
-   *
+      let handlers = {
-   * Generates a brand-new globally unique identifier (GUID).  Borrowed
+        handleError(error) {
-   * from Weave's utils.js.
+          Logger.logError("Error occurred updating form history: " + Log.exceptionStr(error));
-   *
+          reject(error);
-   * @return the new guid
+        },
-   */
+        handleCompletion(reason) {
-  makeGUID: function makeGUID() {
+          resolve();
-    // 70 characters that are not-escaped URL-friendly
+        }
-    const code =
+      }
-      "!()*-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz~";
+      FormHistory.update(data, handlers);
-
+    });
-    let guid = "";
-    let num = 0;
-    let val;
-
-    // Generate ten 70-value characters for a 70^10 (~61.29-bit) GUID
-    for (let i = 0; i < 10; i++) {
-      // Refresh the number source after using it a few times
-      if (i == 0 || i == 5)
-        num = Math.random();
-
-      // Figure out which code to use for the next GUID character
-      num *= 70;
-      val = Math.floor(num);
-      guid += code[val];
-      num -= val;
-    }
-
-    return guid;
   },
 
   /**
    * insertValue
    *
-   * Inserts the specified value for the specified fieldname into the
+   * Adds the specified value for the specified fieldname into form history.
-   * moz_formhistory table.
    *
    * @param fieldname The form fieldname to insert
    * @param value The form value to insert
    * @param us The time, in microseconds, to use for the lastUsed
    *        and firstUsed columns
-   * @return nothing
+   * @return Promise<undefined>
    */
-  insertValue: function (fieldname, value, us) {
+  insertValue(fieldname, value, us) {
-    let query = this.createStatement(
+    let data = { op: "add", fieldname, value, timesUsed: 1,
-      "INSERT INTO moz_formhistory " +
+                 firstUsed: us, lastUsed: us }
-      "(fieldname, value, timesUsed, firstUsed, lastUsed, guid) VALUES " +
+    return this._update(data);
-      "(:fieldname, :value, :timesUsed, :firstUsed, :lastUsed, :guid)");
-    query.params.fieldname = fieldname;
-    query.params.value = value;
-    query.params.timesUsed = 1;
-    query.params.firstUsed = us;
-    query.params.lastUsed = us;
-    query.params.guid = this.makeGUID();
-    query.execute();
-    query.reset();
   },
 
   /**
@@ -90,15 +61,10 @@ var FormDB = {
    *
    * @param id The id of the row to update
    * @param newvalue The new value to set
-   * @return nothing
+   * @return Promise<undefined>
    */
-  updateValue: function (id, newvalue) {
+  updateValue(id, newvalue) {
-    let query = this.createStatement(
+    return this._update({ op: "update", guid: id, value: newvalue });
-      "UPDATE moz_formhistory SET value = :value WHERE id = :id");
-    query.params.id = id;
-    query.params.value = newvalue;
-    query.execute();
-    query.reset();
   },
 
   /**
@@ -109,52 +75,44 @@ var FormDB = {
    *
    * @param fieldname The fieldname of the row to query
    * @param value The value of the row to query
-   * @return null if no row is found with the specified fieldname and value,
+   * @return Promise<null if no row is found with the specified fieldname and value,
-   *         or an object containing the row's id, lastUsed, and firstUsed
+   *         or an object containing the row's guid, lastUsed, and firstUsed
-   *         values
+   *         values>
    */
-  getDataForValue: function (fieldname, value) {
+  getDataForValue(fieldname, value) {
-    let query = this.createStatement(
+    return new Promise((resolve, reject) => {
-      "SELECT id, lastUsed, firstUsed FROM moz_formhistory WHERE " +
+      let result = null;
-      "fieldname = :fieldname AND value = :value");
+      let handlers = {
-    query.params.fieldname = fieldname;
+        handleResult(oneResult) {
-    query.params.value = value;
+          if (result != null) {
-    if (!query.executeStep())
+            reject("more than 1 result for this query");
-      return null;
+            return;
-
+          }
-    return {
+          result = oneResult;
-      id: query.row.id,
+        },
-      lastUsed: query.row.lastUsed,
+        handleError(error) {
-      firstUsed: query.row.firstUsed
+          Logger.logError("Error occurred updating form history: " + Log.exceptionStr(error));
-    };
+          reject(error);
+        },
+        handleCompletion(reason) {
+          resolve(result);
+        }
+      }
+      FormHistory.search(["guid", "lastUsed", "firstUsed"], { fieldname }, handlers);
+    });
   },
 
   /**
-   * createStatement
+   * remove
    *
-   * Creates a statement from a SQL string.  This function is borrowed
+   * Removes the specified GUID from the database.
-   * from Weave's forms.js.
    *
-   * @param query The SQL query string
+   * @param guid The guid of the item to delete
-   * @return the mozIStorageStatement created from the specified SQL
+   * @return Promise<>
    */
-  createStatement: function createStatement(query) {
+   remove(guid) {
-    try {
+    return this._update({ op: "remove", guid });
-      // Just return the statement right away if it's okay
+  },
-      return formService.DBConnection.createStatement(query);
-    }
-    catch(ex) {
-      // Assume guid column must not exist yet, so add it with an index
-      formService.DBConnection.executeSimpleSQL(
-        "ALTER TABLE moz_formhistory ADD COLUMN guid TEXT");
-      formService.DBConnection.executeSimpleSQL(
-        "CREATE INDEX IF NOT EXISTS moz_formhistory_guid_index " +
-        "ON moz_formhistory (guid)");
-    }
-
-    // Try creating the query now that the column exists
-    return formService.DBConnection.createStatement(query);
-  }
 };
 
 /**
@@ -204,18 +162,18 @@ FormData.prototype = {
     Logger.AssertTrue(this.fieldname != null && this.value != null,
       "Must specify both fieldname and value");
 
-    let formdata = FormDB.getDataForValue(this.fieldname, this.value);
+    return FormDB.getDataForValue(this.fieldname, this.value).then(formdata => {
-    if (!formdata) {
+      if (!formdata) {
-      // this item doesn't exist yet in the db, so we need to insert it
+        // this item doesn't exist yet in the db, so we need to insert it
-      FormDB.insertValue(this.fieldname, this.value,
+        return FormDB.insertValue(this.fieldname, this.value,
-                         this.hours_to_us(this.date));
+                                  this.hours_to_us(this.date));
-    }
+      } else {
-    else {
+        /* Right now, we ignore this case.  If bug 552531 is ever fixed,
-      /* Right now, we ignore this case.  If bug 552531 is ever fixed,
+           we might need to add code here to update the firstUsed or
-         we might need to add code here to update the firstUsed or
+           lastUsed fields, as appropriate.
-         lastUsed fields, as appropriate.
+         */
-       */
+      }
-    }
+    });
   },
 
   /**
@@ -227,21 +185,22 @@ FormData.prototype = {
    * @return true if this entry exists in the database, otherwise false
    */
   Find: function() {
-    let formdata = FormDB.getDataForValue(this.fieldname, this.value);
+    return FormDB.getDataForValue(this.fieldname, this.value).then(formdata => {
-    let status = formdata != null;
+      let status = formdata != null;
-    if (status) {
+      if (status) {
-      /*
+        /*
-      //form history dates currently not synced!  bug 552531
+        //form history dates currently not synced!  bug 552531
-      let us = this.hours_to_us(this.date);
+        let us = this.hours_to_us(this.date);
-      status = Logger.AssertTrue(
+        status = Logger.AssertTrue(
-        us >= formdata.firstUsed && us <= formdata.lastUsed,
+          us >= formdata.firstUsed && us <= formdata.lastUsed,
-        "No match for with that date value");
+          "No match for with that date value");
 
-      if (status)
+        if (status)
-      */
+        */
-        this.id = formdata.id;
+          this.id = formdata.guid;
-    }
+      }
-    return status;
+      return status;
+    });
   },
 
   /**
@@ -255,7 +214,6 @@ FormData.prototype = {
   Remove: function() {
     /* Right now Weave doesn't handle this correctly, see bug 568363.
      */
-    formService.removeEntry(this.fieldname, this.value);
+    return FormDB.remove(this.id);
-    return true;
   },
 };

services/sync/tps/extensions/tps/resource/tps.jsm

@@ -82,7 +82,7 @@ const ACTIONS = [
 const OBSERVER_TOPICS = ["fxaccounts:onlogin",
                          "fxaccounts:onlogout",
                          "private-browsing",
-                         "quit-application-requested",
+                         "profile-before-change",
                          "sessionstore-windows-restored",
                          "weave:engine:start-tracking",
                          "weave:engine:stop-tracking",
@@ -166,7 +166,7 @@ var TPS = {
           Logger.logInfo("private browsing " + data);
           break;
 
-        case "quit-application-requested":
+        case "profile-before-change":
           OBSERVER_TOPICS.forEach(function(topic) {
             Services.obs.removeObserver(this, topic);
           }, this);
@@ -367,17 +367,18 @@ var TPS = {
       let formdata = new FormData(datum, this._usSinceEpoch);
       switch(action) {
         case ACTION_ADD:
-          formdata.Create();
+          Async.promiseSpinningly(formdata.Create());
           break;
         case ACTION_DELETE:
-          formdata.Remove();
+          Async.promiseSpinningly(formdata.Remove());
           break;
         case ACTION_VERIFY:
-          Logger.AssertTrue(formdata.Find(), "form data not found");
+          Logger.AssertTrue(Async.promiseSpinningly(formdata.Find()),
+                            "form data not found");
           break;
         case ACTION_VERIFY_NOT:
-          Logger.AssertTrue(!formdata.Find(),
+          Logger.AssertTrue(!Async.promiseSpinningly(formdata.Find()),
-            "form data found, but it shouldn't be present");
+                            "form data found, but it shouldn't be present");
           break;
         default:
           Logger.AssertTrue(false, "invalid action: " + action);
@@ -591,7 +592,14 @@ var TPS = {
       Logger.logError("Failed to wipe server: " + Log.exceptionStr(ex));
     }
     try {
-      Authentication.signOut();
+      if (Authentication.isLoggedIn) {
+        // signout and wait for Sync to completely reset itself.
+        Logger.logInfo("signing out");
+        let waiter = this.createEventWaiter("weave:service:start-over:finish");
+        Authentication.signOut();
+        waiter();
+        Logger.logInfo("signout complete");
+      }
     } catch (e) {
       Logger.logError("Failed to sign out: " + Log.exceptionStr(e));
     }
@@ -701,8 +709,8 @@ var TPS = {
       let validator = new FormValidator();
       let serverRecords = validator.getServerItems(engine);
       let clientRecords = Async.promiseSpinningly(validator.getClientItems());
-      clientRecordDumpStr = JSON.stringify(clientRecords);
+      clientRecordDumpStr = JSON.stringify(clientRecords, undefined, 2);
-      serverRecordDumpStr = JSON.stringify(serverRecords);
+      serverRecordDumpStr = JSON.stringify(serverRecords, undefined, 2);
       let { problemData } = validator.compareClientWithServer(clientRecords, serverRecords);
       for (let { name, count } of problemData.getSummary()) {
         if (count) {
@@ -975,25 +983,57 @@ var TPS = {
     frame.runTestFile(mozmillfile.path, null);
   },
 
+  /**
+   * Return an object that when called, will block until the named event
+   * is observed. This is similar to waitForEvent, although is typically safer
+   * if you need to do some other work that may make the event fire.
+   *
+   * eg:
+   *    doSomething(); // causes the event to be fired.
+   *    waitForEvent("something");
+   * is risky as the call to doSomething may trigger the event before the
+   * waitForEvent call is made. Contrast with:
+   *
+   *   let waiter = createEventWaiter("something"); // does *not* block.
+   *   doSomething(); // causes the event to be fired.
+   *   waiter(); // will return as soon as the event fires, even if it fires
+   *             // before this function is called.
+   *
+   * @param aEventName
+   *        String event to wait for.
+   */
+  createEventWaiter(aEventName) {
+    Logger.logInfo("Setting up wait for " + aEventName + "...");
+    let cb = Async.makeSpinningCallback();
+    Svc.Obs.add(aEventName, cb);
+    return function() {
+      try {
+        cb.wait();
+      } finally {
+        Svc.Obs.remove(aEventName, cb);
+        Logger.logInfo(aEventName + " observed!");
+      }
+    }
+  },
+
+
   /**
    * Synchronously wait for the named event to be observed.
    *
    * When the event is observed, the function will wait an extra tick before
    * returning.
    *
+   * Note that in general, you should probably use createEventWaiter unless you
+   * are 100% sure that the event being waited on can only be sent after this
+   * call adds the listener.
+   *
    * @param aEventName
    *        String event to wait for.
    */
   waitForEvent: function waitForEvent(aEventName) {
-    Logger.logInfo("Waiting for " + aEventName + "...");
+    this.createEventWaiter(aEventName)();
-    let cb = Async.makeSpinningCallback();
-    Svc.Obs.add(aEventName, cb);
-    cb.wait();
-    Svc.Obs.remove(aEventName, cb);
-    Logger.logInfo(aEventName + " observed!");
   },
 
-
   /**
    * Waits for Sync to logged in before returning
    */