Bug 888784 - Make FormHistory.update use Sqlite.jsm database backend. r=mak
☠☠ backed out by 9fc7e71752fd ☠ ☠
authorMike Conley <mconley@mozilla.com>
Thu, 30 Nov 2017 18:09:54 -0500
changeset 450176 73ad820d09ecafbfab3e3d2979f64677009b3ed6
parent 450175 18d185fa362e6a3018560cb414df06e0731b9558
child 450177 46fb8f82f2bffe2bbd27ee8bcec815e3f59d7697
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs888784
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 888784 - Make FormHistory.update use Sqlite.jsm database backend. r=mak MozReview-Commit-ID: 7Ku1kFtTYZK
toolkit/components/satchel/FormHistory.jsm
--- a/toolkit/components/satchel/FormHistory.jsm
+++ b/toolkit/components/satchel/FormHistory.jsm
@@ -617,150 +617,218 @@ function dbClose(aShutdown) {
   _dbConnection.asyncClose(() => closed = true);
 
   if (!aShutdown) {
     Services.tm.spinEventLoopUntil(() => closed);
   }
 }
 
 /**
+ * @typedef {Object} InsertQueryData
+ * @property {Object} updatedChange
+ *           A change requested by FormHistory.
+ * @property {String} query
+ *           The insert query string.
+ */
+
+/**
+ * Prepares a query and some default parameters when inserting an entry
+ * to the database.
+ *
+ * @param {Object} change
+ *        The change requested by FormHistory.
+ * @param {number} now
+ *        The current timestamp in microseconds.
+ * @returns {InsertQueryData}
+ *          The query information needed to pass along to the database.
+ */
+function prepareInsertQuery(change, now) {
+  let updatedChange = Object.assign({}, change);
+  let query = "INSERT INTO moz_formhistory " +
+              "(fieldname, value, timesUsed, firstUsed, lastUsed, guid) " +
+              "VALUES (:fieldname, :value, :timesUsed, :firstUsed, :lastUsed, :guid)";
+  updatedChange.timesUsed = updatedChange.timesUsed || 1;
+  updatedChange.firstUsed = updatedChange.firstUsed || now;
+  updatedChange.lastUsed = updatedChange.lastUsed || now;
+
+  return {
+    updatedChange,
+    query,
+  };
+}
+
+/**
  * Constructs and executes database statements from a pre-processed list of
  * inputted changes.
  *
  * @param {Array.<Object>} aChanges changes to form history
- * @param {Object} aCallbacks
+ * @param {Object} aPreparedHandlers
  */
-async function updateFormHistoryWrite(aChanges, aCallbacks) {
+async function updateFormHistoryWrite(aChanges, aPreparedHandlers) {
   log("updateFormHistoryWrite  " + aChanges.length);
 
   // pass 'now' down so that every entry in the batch has the same timestamp
   let now = Date.now() * 1000;
 
   // for each change, we either create and append a new storage statement to
   // stmts or bind a new set of parameters to an existing storage statement.
   // stmts and bindingArrays are updated when makeXXXStatement eventually
   // calls dbCreateAsyncStatement.
-  let stmts = [];
+  let queries = [];
   let notifications = [];
-  let bindingArrays = new Map();
+  let conn = await FormHistory.db;
 
   for (let change of aChanges) {
     let operation = change.op;
     delete change.op;
-    let stmt;
     switch (operation) {
-      case "remove":
+      case "remove": {
         log("Remove from form history  " + change);
-        let delStmt = makeMoveToDeletedStatement(change.guid, now, change, bindingArrays);
-        if (delStmt && !stmts.includes(delStmt)) {
-          stmts.push(delStmt);
-        }
-        if ("timeDeleted" in change) {
-          delete change.timeDeleted;
-        }
-        stmt = makeRemoveStatement(change, bindingArrays);
+        let queryTerms = makeQueryPredicates(change);
 
         // Fetch the GUIDs we are going to delete.
         try {
-          await new Promise((res, rej) => {
-            let selectStmt = makeSearchStatement(change, ["guid"]);
-            let selectHandlers = {
-              handleCompletion() {
-                res();
-              },
-              handleError() {
-                log("remove select guids failure");
-              },
-              handleResult(aResultSet) {
-                for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow()) {
-                  notifications.push(["formhistory-remove", row.getResultByName("guid")]);
-                }
-              },
-            };
-            dbConnection.executeAsync([selectStmt], 1, selectHandlers);
+          let query = "SELECT guid FROM moz_formhistory";
+          if (queryTerms) {
+            query += " WHERE " + queryTerms;
+          }
+
+          await conn.executeCached(query, change, row => {
+            notifications.push(["formhistory-remove", row.getResultByName("guid")]);
           });
         } catch (e) {
-          log("Error in select statement: " + e);
+          log("Error getting guids from moz_formhistory: " + e);
         }
 
+        if (supportsDeletedTable) {
+          log("Moving to deleted table " + change);
+          let query = "INSERT INTO moz_deleted_formhistory (guid, timeDeleted)";
+
+          // TODO: Add these items to the deleted items table once we've sorted
+          //       out the issues from bug 756701
+          if (change.guid || queryTerms) {
+            query +=
+              change.guid ? " VALUES (:guid, :timeDeleted)"
+                          : " SELECT guid, :timeDeleted FROM moz_formhistory WHERE " + queryTerms;
+            change.timeDeleted = now;
+            queries.push({ query, params: Object.assign({}, change) });
+          }
+
+          if ("timeDeleted" in change) {
+            delete change.timeDeleted;
+          }
+        }
+
+        let query = "DELETE FROM moz_formhistory";
+        if (queryTerms) {
+          log("removeEntries");
+          query += " WHERE " + queryTerms;
+        } else {
+          log("removeAllEntries");
+          // Not specifying any fields means we should remove all entries. We
+          // won't need to modify the query in this case.
+        }
+
+        queries.push({ query, params: change });
         break;
-      case "update":
+      }
+      case "update": {
         log("Update form history " + change);
         let guid = change.guid;
         delete change.guid;
         // a special case for updating the GUID - the new value can be
         // specified in newGuid.
         if (change.newGuid) {
           change.guid = change.newGuid;
           delete change.newGuid;
         }
-        stmt = makeUpdateStatement(guid, change, bindingArrays);
+
+        let query = "UPDATE moz_formhistory SET ";
+        let queryTerms = makeQueryPredicates(change, ", ");
+        if (!queryTerms) {
+          throw Components.Exception("Update query must define fields to modify.",
+                                     Cr.NS_ERROR_ILLEGAL_VALUE);
+        }
+        query += queryTerms + " WHERE guid = :existing_guid";
+        change.existing_guid = guid;
+        queries.push({ query, params: change });
         notifications.push(["formhistory-update", guid]);
         break;
-      case "bump":
+      }
+      case "bump": {
         log("Bump form history " + change);
         if (change.guid) {
-          stmt = makeBumpStatement(change.guid, now, bindingArrays);
+          let query = "UPDATE moz_formhistory " +
+                      "SET timesUsed = timesUsed + 1, lastUsed = :lastUsed WHERE guid = :guid";
+          let queryParams = {
+            lastUsed: now,
+            guid: change.guid,
+          };
+
+          queries.push({ query, params: queryParams });
           notifications.push(["formhistory-update", change.guid]);
         } else {
           change.guid = generateGUID();
-          stmt = makeAddStatement(change, now, bindingArrays);
-          notifications.push(["formhistory-add", change.guid]);
+          let { query, updatedChange } = prepareInsertQuery(change, now);
+          queries.push({ query, params: updatedChange });
+          notifications.push(["formhistory-add", updatedChange.guid]);
         }
         break;
-      case "add":
+      }
+      case "add": {
         log("Add to form history " + change);
         if (!change.guid) {
           change.guid = generateGUID();
         }
-        stmt = makeAddStatement(change, now, bindingArrays);
-        notifications.push(["formhistory-add", change.guid]);
+
+        let { query, updatedChange } = prepareInsertQuery(change, now);
+        queries.push({ query, params: updatedChange });
+        notifications.push(["formhistory-add", updatedChange.guid]);
         break;
-      default:
+      }
+      default: {
         // We should've already guaranteed that change.op is one of the above
         throw Components.Exception("Invalid operation " + operation,
                                    Cr.NS_ERROR_ILLEGAL_VALUE);
-    }
-
-    // As identical statements are reused, only add statements if they aren't already present.
-    if (stmt && !stmts.includes(stmt)) {
-      stmts.push(stmt);
+      }
     }
   }
 
-  for (let stmt of stmts) {
-    stmt.bindParameters(bindingArrays.get(stmt));
+  try {
+    await runUpdateQueries(conn, queries);
+  } catch (e) {
+    aPreparedHandlers.handleError(e);
+    aPreparedHandlers.handleCompletion(1);
+    return;
+  }
+
+  for (let [notification, param] of notifications) {
+    // We're either sending a GUID or nothing at all.
+    sendNotification(notification, param);
   }
 
-  let handlers = {
-    handleCompletion(aReason) {
-      if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) {
-        for (let [notification, param] of notifications) {
-          // We're either sending a GUID or nothing at all.
-          sendNotification(notification, param);
-        }
-      }
+  aPreparedHandlers.handleCompletion(0);
+}
 
-      if (aCallbacks && aCallbacks.handleCompletion) {
-        aCallbacks.handleCompletion(
-          aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED ?
-            0 :
-            1
-        );
-      }
-    },
-    handleError(aError) {
-      if (aCallbacks && aCallbacks.handleError) {
-        aCallbacks.handleError(aError);
-      }
-    },
-    handleResult: NOOP,
-  };
-
-  dbConnection.executeAsync(stmts, stmts.length, handlers);
+/**
+ * Runs queries for an update operation to the database. This
+ * is separated out from updateFormHistoryWrite to avoid shutdown
+ * leaks where the handlers passed to updateFormHistoryWrite would
+ * leak from the closure around the executeTransaction function.
+ *
+ * @param {SqliteConnection} conn the database connection
+ * @param {Object} queries query string and param pairs generated
+ *                 by updateFormHistoryWrite
+ */
+async function runUpdateQueries(conn, queries) {
+  await conn.executeTransaction(async () => {
+    for (let { query, params } of queries) {
+      await conn.executeCached(query, params);
+    }
+  });
 }
 
 /**
  * Functions that expire entries in form history and shrinks database
  * afterwards as necessary initiated by expireOldEntries.
  */
 
 /**
@@ -1165,44 +1233,42 @@ this.FormHistory = {
           handlers.handleError(e);
           handlers.handleCompletion(1);
           reject(e);
         }
       });
     });
   },
 
-  update(aChanges, aCallbacks) {
+  update(aChanges, aHandlers) {
     // 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 (so an X-OR)
       return Boolean(change.guid) != Boolean(change.fieldname && change.value);
     }
 
     if (!("length" in aChanges)) {
       aChanges = [aChanges];
     }
 
+    let handlers = this._prepareHandlers(aHandlers);
+
     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);
-      }
+      handlers.handleError({
+        message: "Form history is disabled, only remove operations are allowed",
+        result: Ci.mozIStorageError.MISUSE,
+      });
+      handlers.handleCompletion(1);
       return;
     }
 
     for (let change of aChanges) {
       switch (change.op) {
         case "remove":
           validateSearchData(change, "Remove");
           continue;
@@ -1252,54 +1318,50 @@ this.FormHistory = {
         {
           fieldname: change.fieldname,
           value: change.value,
         }, {
           foundResult: false,
           handleResult(aResult) {
             if (this.foundResult) {
               log("Database contains multiple entries with the same fieldname/value pair.");
-              if (aCallbacks && aCallbacks.handleError) {
-                aCallbacks.handleError({
-                  message:
-                    "Database contains multiple entries with the same fieldname/value pair.",
-                  result: 19, // Constraint violation
-                });
-              }
+              handlers.handleError({
+                message:
+                  "Database contains multiple entries with the same fieldname/value pair.",
+                result: 19, // Constraint violation
+              });
 
               searchFailed = true;
               return;
             }
 
             this.foundResult = true;
             changeToUpdate.guid = aResult.guid;
           },
 
           handleError(aError) {
-            if (aCallbacks && aCallbacks.handleError) {
-              aCallbacks.handleError(aError);
-            }
+            handlers.handleError(aError);
           },
 
           handleCompletion(aReason) {
             completedSearches++;
             if (completedSearches == numSearches) {
               if (!aReason && !searchFailed) {
-                updateFormHistoryWrite(aChanges, aCallbacks);
-              } else if (aCallbacks && aCallbacks.handleCompletion) {
-                aCallbacks.handleCompletion(1);
+                updateFormHistoryWrite(aChanges, handlers);
+              } else {
+                handlers.handleCompletion(1);
               }
             }
           },
         });
     }
 
     if (numSearches == 0) {
       // We don't have to wait for any statements to return.
-      updateFormHistoryWrite(aChanges, aCallbacks);
+      updateFormHistoryWrite(aChanges, handlers);
     }
   },
 
   getAutoCompleteResults(searchString, params, aCallbacks) {
     // only do substring matching when the search string contains more than one character
     let searchTokens;
     let where = "";
     let boundaryCalc = "";