Bug 976940 - FormHistory.update() should call handleError if form history is disabled and the operation is not a removal. r=markh a=lizzard
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 22 Feb 2016 18:17:43 +0100
changeset 317187 6c5820c96c4a30cd3c8d248603fc4ed54bea29ac
parent 317186 9bd0b3869ad80501b2dedcfe4c7b3e00145b6d03
child 317188 0ecf23735bfa0709403f10d0697b77cef7ab5e1d
push id5703
push userraliiev@mozilla.com
push dateMon, 07 Mar 2016 14:18:41 +0000
treeherdermozilla-beta@31e373ad5b5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, lizzard
bugs976940
milestone46.0a2
Bug 976940 - FormHistory.update() should call handleError if form history is disabled and the operation is not a removal. r=markh a=lizzard MozReview-Commit-ID: 94FRqGofGsP
browser/components/search/content/search.xml
browser/modules/ContentSearch.jsm
toolkit/components/satchel/FormHistory.jsm
toolkit/components/satchel/test/unit/test_history_api.js
toolkit/content/finddialog.js
--- a/browser/components/search/content/search.xml
+++ b/browser/components/search/content/search.xml
@@ -399,17 +399,17 @@
       <method name="doSearch">
         <parameter name="aData"/>
         <parameter name="aWhere"/>
         <parameter name="aEngine"/>
         <body><![CDATA[
           var textBox = this._textbox;
 
           // Save the current value in the form history
-          if (aData && !PrivateBrowsingUtils.isWindowPrivate(window)) {
+          if (aData && !PrivateBrowsingUtils.isWindowPrivate(window) && this.FormHistory.enabled) {
             this.FormHistory.update(
               { op : "bump",
                 fieldname : textBox.getAttribute("autocompletesearchparam"),
                 value : aData },
               { handleError : function(aError) {
                   Components.utils.reportError("Saving search to form history failed: " + aError.message);
               }});
           }
--- a/browser/modules/ContentSearch.jsm
+++ b/browser/modules/ContentSearch.jsm
@@ -370,26 +370,28 @@ this.ContentSearch = {
       // properties, which won't be true if the browser has been destroyed.
       // That may be the case here due to the asynchronous nature of messaging.
       isPrivate = PrivateBrowsingUtils.isBrowserPrivate(msg.target);
     } catch (err) {}
     if (isPrivate || entry === "") {
       return Promise.resolve();
     }
     let browserData = this._suggestionDataForBrowser(msg.target, true);
-    FormHistory.update({
-      op: "bump",
-      fieldname: browserData.controller.formHistoryParam,
-      value: entry,
-    }, {
-      handleCompletion: () => {},
-      handleError: err => {
-        Cu.reportError("Error adding form history entry: " + err);
-      },
-    });
+    if (FormHistory.enabled) {
+      FormHistory.update({
+        op: "bump",
+        fieldname: browserData.controller.formHistoryParam,
+        value: entry,
+      }, {
+        handleCompletion: () => {},
+        handleError: err => {
+          Cu.reportError("Error adding form history entry: " + err);
+        },
+      });
+    }
     return Promise.resolve();
   },
 
   _onMessageRemoveFormHistoryEntry: function (msg, entry) {
     let browserData = this._suggestionDataForBrowser(msg.target);
     if (browserData && browserData.previousFormHistoryResult) {
       let { previousFormHistoryResult } = browserData;
       for (let i = 0; i < previousFormHistoryResult.matchCount; i++) {
--- a/toolkit/components/satchel/FormHistory.jsm
+++ b/toolkit/components/satchel/FormHistory.jsm
@@ -837,34 +837,44 @@ this.FormHistory = {
         }
       }
     };
 
     stmt.executeAsync(handlers);
   },
 
   update : function formHistoryUpdate(aChanges, aCallbacks) {
-    if (!Prefs.enabled) {
-      return;
-    }
-
     // Used to keep track of how many searches have been started. When that number
     // are finished, updateFormHistoryWrite can be called.
     let numSearches = 0;
     let completedSearches = 0;
     let searchFailed = false;
 
     function validIdentifier(change) {
       // The identifier is only valid if one of either the guid or the (fieldname/value) are set
       return Boolean(change.guid) != Boolean(change.fieldname && change.value);
     }
 
     if (!("length" in aChanges))
       aChanges = [aChanges];
 
+    let isRemoveOperation = aChanges.every(change => change && change.op && change.op == "remove");
+    if (!Prefs.enabled && !isRemoveOperation) {
+      if (aCallbacks && aCallbacks.handleError) {
+        aCallbacks.handleError({
+          message: "Form history is disabled, only remove operations are allowed",
+          result: Ci.mozIStorageError.MISUSE
+        });
+      }
+      if (aCallbacks && aCallbacks.handleCompletion) {
+        aCallbacks.handleCompletion(1);
+      }
+      return;
+    }
+
     for (let change of aChanges) {
       switch (change.op) {
         case "remove":
           validateSearchData(change, "Remove");
           continue;
         case "update":
           if (validIdentifier(change)) {
             validateOpData(change, "Update");
--- a/toolkit/components/satchel/test/unit/test_history_api.js
+++ b/toolkit/components/satchel/test/unit/test_history_api.js
@@ -53,27 +53,31 @@ function promiseUpdateEntry(op, name, va
   var change = { op: op };
   if (name !== null)
     change.fieldname = name;
   if (value !== null)
     change.value = value;
   return promiseUpdate(change);
 }
 
-function promiseUpdate(change)
-{
-  let deferred = Promise.defer();
-  FormHistory.update(change,
-                     { handleError: function (error) {
-                         do_throw("Error occurred updating form history: " + error);
-                         deferred.reject(error);
-                       },
-                       handleCompletion: function (reason) { if (!reason) deferred.resolve(); }
-                     });
-  return deferred.promise;
+function promiseUpdate(change) {
+  return new Promise((resolve, reject) => {
+    FormHistory.update(change, {
+      handleError(error) {
+        this._error = error;
+      },
+      handleCompletion(reason) {
+        if (reason) {
+          reject(this._error);
+        } else {
+          resolve();
+        }
+      }
+    });
+  });
 }
 
 function promiseSearchEntries(terms, params)
 {
   let deferred = Promise.defer();
   let results = [];
   FormHistory.search(terms, params,
                      { handleResult: result => results.push(result),
@@ -400,16 +404,49 @@ add_task(function* ()
   do_check_eq(13, results[3].timesUsed);
   do_check_eq(230, results[2].firstUsed);
   do_check_eq(430, results[3].firstUsed);
   do_check_true(results[2].lastUsed > 600);
   do_check_true(results[3].lastUsed > 700);
 
   yield promiseCountEntries(null, null, num => do_check_eq(num, 4));
 
+  // ===== 21 =====
+  // Check update fails if form history is disabled and the operation is not a
+  // pure removal.
+  testnum++;
+  Services.prefs.setBoolPref("browser.formfill.enable", false);
+
+  // Cannot use arrow functions, see bug 1237961.
+  Assert.rejects(promiseUpdate(
+                   { op : "bump", fieldname: "field5", value: "value5" }),
+                 function(err) { return err.result == Ci.mozIStorageError.MISUSE; },
+                 "bumping when form history is disabled should fail");
+  Assert.rejects(promiseUpdate(
+                   { op : "add", fieldname: "field5", value: "value5" }),
+                 function(err) { return err.result == Ci.mozIStorageError.MISUSE; },
+                 "Adding when form history is disabled should fail");
+  Assert.rejects(promiseUpdate([
+                     { op : "update", fieldname: "field5", value: "value5" },
+                     { op : "remove", fieldname: "field5", value: "value5" }
+                   ]),
+                 function(err) { return err.result == Ci.mozIStorageError.MISUSE; },
+                 "mixed operations when form history is disabled should fail");
+  Assert.rejects(promiseUpdate([
+                     null, undefined, "", 1, {},
+                     { op : "remove", fieldname: "field5", value: "value5" }
+                   ]),
+                 function(err) { return err.result == Ci.mozIStorageError.MISUSE; },
+                 "Invalid entries when form history is disabled should fail");
+
+  // Remove should work though.
+  yield promiseUpdate([{ op: "remove", fieldname: "field5", value: null },
+                       { op: "remove", fieldname: null, value: null }]);
+  Services.prefs.clearUserPref("browser.formfill.enable");
+
   } catch (e) {
     throw "FAILED in test #" + testnum + " -- " + e;
   }
   finally {
     FormHistory._supportsDeletedTable = oldSupportsDeletedTable;
     dbConnection.asyncClose(do_test_finished);
   }
 });
--- a/toolkit/content/finddialog.js
+++ b/toolkit/content/finddialog.js
@@ -131,19 +131,21 @@ function doEnabling()
 
 function updateFormHistory()
 {
   if (window.opener.PrivateBrowsingUtils &&
       window.opener.PrivateBrowsingUtils.isWindowPrivate(window.opener) ||
       !dialog.findKey.value)
     return;
 
-  FormHistory.update({
-    op: "bump",
-    fieldname: "find-dialog",
-    value: dialog.findKey.value
-  }, {
-    handleError: function(aError) {
-      Components.utils.reportError("Saving find to form history failed: " +
-                                   aError.message);
-    }
-  });
+  if (FormHistory.enabled) {
+    FormHistory.update({
+      op: "bump",
+      fieldname: "find-dialog",
+      value: dialog.findKey.value
+    }, {
+      handleError: function(aError) {
+        Components.utils.reportError("Saving find to form history failed: " +
+                                     aError.message);
+      }
+    });
+  }
 }