Bug 1320301 - Add partial support to sqlite3_interrupt. r=asuth
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 31 Jul 2017 22:27:23 +0200
changeset 426142 34018fbd40961704c75af4ea446506658ffcaa43
parent 426141 95c4a9f2a723f59e207bfa461b274571a96311ad
child 426143 109578ac95f64f09218c6f1268b60ee0d1597641
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth
bugs1320301
milestone57.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 1320301 - Add partial support to sqlite3_interrupt. r=asuth MozReview-Commit-ID: V3ZjLEjqmT
dom/cache/Connection.cpp
storage/mozIStorageAsyncConnection.idl
storage/mozStorageAsyncStatementExecution.cpp
storage/mozStorageConnection.cpp
storage/test/unit/test_connection_interrupt.js
storage/test/unit/test_storage_connection.js
storage/test/unit/xpcshell.ini
toolkit/components/places/UnifiedComplete.js
toolkit/modules/Sqlite.jsm
toolkit/modules/tests/xpcshell/test_sqlite.js
--- a/dom/cache/Connection.cpp
+++ b/dom/cache/Connection.cpp
@@ -156,16 +156,22 @@ Connection::Clone(bool aReadOnly, mozISt
 
   nsCOMPtr<mozIStorageConnection> wrapped = new Connection(conn);
   wrapped.forget(aConnectionOut);
 
   return rv;
 }
 
 NS_IMETHODIMP
+Connection::Interrupt()
+{
+  return mBase->Interrupt();
+}
+
+NS_IMETHODIMP
 Connection::GetDefaultPageSize(int32_t* aSizeOut)
 {
   return mBase->GetDefaultPageSize(aSizeOut);
 }
 
 NS_IMETHODIMP
 Connection::GetConnectionReady(bool* aReadyOut)
 {
--- a/storage/mozIStorageAsyncConnection.idl
+++ b/storage/mozIStorageAsyncConnection.idl
@@ -75,16 +75,18 @@ interface mozIStorageAsyncConnection : n
    *
    * @throws NS_ERROR_NOT_SAME_THREAD
    *         If is called on a thread other than the one that opened it.
    * @throws NS_ERROR_UNEXPECTED
    *         If this connection is a memory database.
    *
    * @note If your connection is already read-only, you will get a read-only
    *       clone.
+   * @note The resulting connection will NOT implement mozIStorageConnection,
+   *       it will only implement mozIStorageAsyncConnection.
    * @note Due to a bug in SQLite, if you use the shared cache
    *       (see mozIStorageService), you end up with the same privileges as the
    *       first connection opened regardless of what is specified in aReadOnly.
    * @note The following pragmas are copied over to a read-only clone:
    *        - cache_size
    *        - temp_store
    *       The following pragmas are copied over to a writeable clone:
    *        - cache_size
@@ -98,16 +100,26 @@ interface mozIStorageAsyncConnection : n
                   in mozIStorageCompletionCallback aCallback);
 
   /**
    * The current database nsIFile.  Null if the database
    * connection refers to an in-memory database.
    */
   readonly attribute nsIFile databaseFile;
 
+  /**
+   * Causes any pending database operation to abort and return at the first
+   * opportunity.
+   * This can only be used on read-only connections that don't implement
+   * the mozIStorageConnection interface.
+   * @note operations that are nearly complete may still be able to complete.
+   * @throws if used on an unsupported connection type, or a closed connection.
+   */
+  void interrupt();
+
   //////////////////////////////////////////////////////////////////////////////
   //// Statement creation
 
   /**
    * Create an asynchronous statement for the given SQL. An
    * asynchronous statement can only be used to dispatch asynchronous
    * requests to the asynchronous execution thread and cannot be used
    * to take any synchronous actions on the database.
--- a/storage/mozStorageAsyncStatementExecution.cpp
+++ b/storage/mozStorageAsyncStatementExecution.cpp
@@ -174,17 +174,17 @@ AsyncExecuteStatements::executeAndProces
   mMutex.AssertNotCurrentThreadOwns();
 
   // Execute our statement
   bool hasResults;
   do {
     hasResults = executeStatement(aStatement);
 
     // If we had an error, bail.
-    if (mState == ERROR)
+    if (mState == ERROR || mState == CANCELED)
       return false;
 
     // If we have been canceled, there is no point in going on...
     {
       MutexAutoLock lockedScope(mMutex);
       if (mCancelRequested) {
         mState = CANCELED;
         return false;
@@ -252,16 +252,21 @@ AsyncExecuteStatements::executeStatement
       // Don't hold the lock while we call outside our module.
       SQLiteMutexAutoUnlock unlockedScope(mDBMutex);
 
       // Yield, and try again
       (void)::PR_Sleep(PR_INTERVAL_NO_WAIT);
       continue;
     }
 
+    if (rc == SQLITE_INTERRUPT) {
+      mState = CANCELED;
+      return false;
+    }
+
     // Set an error state.
     mState = ERROR;
     Telemetry::Accumulate(Telemetry::MOZ_STORAGE_ASYNC_REQUESTS_SUCCESS, false);
 
     // Construct the error message before giving up the mutex (which we cannot
     // hold during the call to notifyError).
     nsCOMPtr<mozIStorageError> errorObj(
       new Error(rc, ::sqlite3_errmsg(mNativeConnection))
--- a/storage/mozStorageConnection.cpp
+++ b/storage/mozStorageConnection.cpp
@@ -1446,18 +1446,18 @@ Connection::AsyncClone(bool aReadOnly,
   int flags = mFlags;
   if (aReadOnly) {
     // Turn off SQLITE_OPEN_READWRITE, and set SQLITE_OPEN_READONLY.
     flags = (~SQLITE_OPEN_READWRITE & flags) | SQLITE_OPEN_READONLY;
     // Turn off SQLITE_OPEN_CREATE.
     flags = (~SQLITE_OPEN_CREATE & flags);
   }
 
-  RefPtr<Connection> clone = new Connection(mStorageService, flags,
-                                              mAsyncOnly);
+  // Force the cloned connection to only implement the async connection API.
+  RefPtr<Connection> clone = new Connection(mStorageService, flags, true);
 
   RefPtr<AsyncInitializeClone> initEvent =
     new AsyncInitializeClone(this, clone, aReadOnly, aCallback);
   // Dispatch to our async thread, since the originating connection must remain
   // valid and open for the whole cloning process.  This also ensures we are
   // properly serialized with a `close` operation, rather than race with it.
   nsCOMPtr<nsIEventTarget> target = getAsyncExecutionTarget();
   if (!target) {
@@ -1586,29 +1586,42 @@ Connection::Clone(bool aReadOnly,
   int flags = mFlags;
   if (aReadOnly) {
     // Turn off SQLITE_OPEN_READWRITE, and set SQLITE_OPEN_READONLY.
     flags = (~SQLITE_OPEN_READWRITE & flags) | SQLITE_OPEN_READONLY;
     // Turn off SQLITE_OPEN_CREATE.
     flags = (~SQLITE_OPEN_CREATE & flags);
   }
 
-  RefPtr<Connection> clone = new Connection(mStorageService, flags,
-                                              mAsyncOnly);
+  RefPtr<Connection> clone = new Connection(mStorageService, flags, mAsyncOnly);
 
   nsresult rv = initializeClone(clone, aReadOnly);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   NS_IF_ADDREF(*_connection = clone);
   return NS_OK;
 }
 
 NS_IMETHODIMP
+Connection::Interrupt()
+{
+  MOZ_ASSERT(threadOpenedOn == NS_GetCurrentThread());
+  if (!mDBConn) {
+    return NS_ERROR_NOT_INITIALIZED;
+  }
+  if (!mAsyncOnly || !(mFlags & SQLITE_OPEN_READONLY)) {
+    return NS_ERROR_INVALID_ARG;
+  }
+  ::sqlite3_interrupt(mDBConn);
+  return NS_OK;
+}
+
+NS_IMETHODIMP
 Connection::GetDefaultPageSize(int32_t *_defaultPageSize)
 {
   *_defaultPageSize = Service::getDefaultPageSize();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 Connection::GetConnectionReady(bool *_ready)
new file mode 100644
--- /dev/null
+++ b/storage/test/unit/test_connection_interrupt.js
@@ -0,0 +1,76 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// This file tests the functionality of mozIStorageAsyncConnection::interrupt.
+
+add_task(async function test_sync_conn() {
+  // Interrupt can only be used on async connections.
+  let db = getOpenedDatabase();
+  Assert.throws(() => db.interrupt(),
+                /NS_ERROR_ILLEGAL_VALUE/,
+                "interrupt() should throw if invoked on a synchronous connection");
+  db.close();
+});
+
+add_task(async function test_wr_async_conn() {
+  // Interrupt cannot be used on R/W async connections.
+  let db = await openAsyncDatabase(getTestDB());
+  Assert.throws(() => db.interrupt(),
+                /NS_ERROR_ILLEGAL_VALUE/,
+                "interrupt() should throw if invoked on a R/W connection");
+  await asyncClose(db);
+});
+
+add_task(async function test_closed_conn() {
+  let db = await openAsyncDatabase(getTestDB(), {readOnly: true});
+  await asyncClose(db);
+  Assert.throws(() => db.interrupt(),
+                /NS_ERROR_NOT_INITIALIZED/,
+                "interrupt() should throw if invoked on a closed connection");
+});
+
+add_task({
+  // We use a timeout in the test that may be insufficient on Android emulators.
+  // We don't really need the Android coverage, so skip on Android.
+  skip_if: () => AppConstants.platform == "android"
+}, async function test_async_conn() {
+  let db = await openAsyncDatabase(getTestDB(), {readOnly: true});
+  // This query is built to hang forever.
+  let stmt = db.createAsyncStatement(`
+    WITH RECURSIVE test(n) AS (
+      VALUES(1)
+      UNION ALL
+      SELECT n + 1 FROM test
+    )
+    SELECT t.n
+    FROM test,test AS t`);
+
+  let completePromise = new Promise((resolve, reject) => {
+    let listener = {
+      handleResult(aResultSet) {
+        reject();
+      },
+      handleError(aError) {
+        reject();
+      },
+      handleCompletion(aReason) {
+        resolve(aReason);
+      }
+    };
+    stmt.executeAsync(listener);
+    stmt.finalize();
+  });
+
+  // Wait for the statement to be executing.
+  // This is not rock-solid, see the discussion in bug 1320301. A better
+  // approach will be evaluated in a separate bug.
+  await new Promise(resolve => do_timeout(500, resolve));
+
+  db.interrupt();
+
+  Assert.equal(await completePromise,
+               Ci.mozIStorageStatementCallback.REASON_CANCELED,
+               "Should have been canceled");
+
+  await asyncClose(db);
+});
--- a/storage/test/unit/test_storage_connection.js
+++ b/storage/test/unit/test_storage_connection.js
@@ -426,27 +426,29 @@ add_task(async function test_async_open_
 
 add_task(async function test_clone_trivial_async() {
   do_print("Open connection");
   let db = getService().openDatabase(getTestDB());
   do_check_true(db instanceof Ci.mozIStorageAsyncConnection);
   do_print("AsyncClone connection");
   let clone = await asyncClone(db, true);
   do_check_true(clone instanceof Ci.mozIStorageAsyncConnection);
+  do_check_false(clone instanceof Ci.mozIStorageConnection);
   do_print("Close connection");
   await asyncClose(db);
   do_print("Close clone");
   await asyncClose(clone);
 });
 
 add_task(async function test_clone_no_optional_param_async() {
   "use strict";
   do_print("Testing async cloning");
   let adb1 = await openAsyncDatabase(getTestDB(), null);
   do_check_true(adb1 instanceof Ci.mozIStorageAsyncConnection);
+  do_check_false(adb1 instanceof Ci.mozIStorageConnection);
 
   do_print("Cloning database");
 
   let adb2 = await asyncClone(adb1);
   do_print("Testing that the cloned db is a mozIStorageAsyncConnection " +
            "and not a mozIStorageConnection");
   do_check_true(adb2 instanceof Ci.mozIStorageAsyncConnection);
   do_check_false(adb2 instanceof Ci.mozIStorageConnection);
--- a/storage/test/unit/xpcshell.ini
+++ b/storage/test/unit/xpcshell.ini
@@ -13,16 +13,17 @@ support-files =
 [test_bug-444233.js]
 [test_cache_size.js]
 [test_chunk_growth.js]
 # Bug 676981: test fails consistently on Android
 fail-if = os == "android"
 [test_connection_asyncClose.js]
 [test_connection_executeAsync.js]
 [test_connection_executeSimpleSQLAsync.js]
+[test_connection_interrupt.js]
 [test_js_helpers.js]
 [test_levenshtein.js]
 [test_like.js]
 [test_like_escape.js]
 [test_locale_collation.js]
 [test_page_size_is_32k.js]
 [test_sqlite_secure_delete.js]
 [test_statement_executeAsync.js]
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -338,16 +338,21 @@ function convertBucketsCharPrefToArray(s
  *   add(uri): adds a given nsIURI to the store
  *   delete(uri): removes a given nsIURI from the store
  *   shutdown(): stops storing data to Sqlite
  */
 XPCOMUtils.defineLazyGetter(this, "SwitchToTabStorage", () => Object.seal({
   _conn: null,
   // Temporary queue used while the database connection is not available.
   _queue: new Map(),
+  // Whether we are in the process of updating the temp table.
+  _updatingLevel: 0,
+  get updating() {
+    return this._updatingLevel > 0;
+  },
   async initDatabase(conn) {
     // To reduce IO use an in-memory table for switch-to-tab tracking.
     // Note: this should be kept up-to-date with the definition in
     //       nsPlacesTables.h.
     await conn.execute(
       `CREATE TEMP TABLE moz_openpages_temp (
          url TEXT,
          userContextId INTEGER,
@@ -367,65 +372,74 @@ XPCOMUtils.defineLazyGetter(this, "Switc
            AND userContextId = NEW.userContextId;
        END`);
 
     this._conn = conn;
 
     // Populate the table with the current cache contents...
     for (let [userContextId, uris] of this._queue) {
       for (let uri of uris) {
-        this.add(uri, userContextId);
+        this.add(uri, userContextId).catch(Cu.reportError);
       }
     }
 
     // ...then clear it to avoid double additions.
     this._queue.clear();
   },
 
-  add(uri, userContextId) {
+  async add(uri, userContextId) {
     if (!this._conn) {
       if (!this._queue.has(userContextId)) {
         this._queue.set(userContextId, new Set());
       }
       this._queue.get(userContextId).add(uri);
       return;
     }
-    this._conn.executeCached(
-      `INSERT OR REPLACE INTO moz_openpages_temp (url, userContextId, open_count)
-         VALUES ( :url,
-                  :userContextId,
-                  IFNULL( ( SELECT open_count + 1
-                            FROM moz_openpages_temp
-                            WHERE url = :url
-                            AND userContextId = :userContextId ),
-                          1
-                        )
-                )`
-    , { url: uri.spec, userContextId });
+    try {
+      this._updatingLevel++;
+      await this._conn.executeCached(
+        `INSERT OR REPLACE INTO moz_openpages_temp (url, userContextId, open_count)
+          VALUES ( :url,
+                    :userContextId,
+                    IFNULL( ( SELECT open_count + 1
+                              FROM moz_openpages_temp
+                              WHERE url = :url
+                              AND userContextId = :userContextId ),
+                            1
+                          )
+                  )
+        `, { url: uri.spec, userContextId });
+    } finally {
+      this._updatingLevel--;
+    }
   },
 
-  delete(uri, userContextId) {
+  async delete(uri, userContextId) {
     if (!this._conn) {
-      // This should not happen.
       if (!this._queue.has(userContextId)) {
         throw new Error("Unknown userContextId!");
       }
 
       this._queue.get(userContextId).delete(uri);
       if (this._queue.get(userContextId).size == 0) {
         this._queue.delete(userContextId);
       }
       return;
     }
-    this._conn.executeCached(
-      `UPDATE moz_openpages_temp
-       SET open_count = open_count - 1
-       WHERE url = :url
-       AND userContextId = :userContextId`
-    , { url: uri.spec, userContextId });
+    try {
+      this._updatingLevel++;
+      await this._conn.executeCached(
+        `UPDATE moz_openpages_temp
+         SET open_count = open_count - 1
+         WHERE url = :url
+           AND userContextId = :userContextId
+        `, { url: uri.spec, userContextId });
+    } finally {
+      this._updatingLevel--;
+    }
   },
 
   shutdown() {
     this._conn = null;
     this._queue.clear();
   }
 }));
 
@@ -942,16 +956,19 @@ Search.prototype = {
     if (this._sleepResolve) {
       this._sleepResolve();
       this._sleepResolve = null;
     }
     if (this._searchSuggestionController) {
       this._searchSuggestionController.stop();
       this._searchSuggestionController = null;
     }
+    if (typeof this.interrupt == "function") {
+      this.interrupt();
+    }
     this.pending = false;
   },
 
   /**
    * Whether this search is active.
    */
   pending: true,
 
@@ -960,16 +977,24 @@ Search.prototype = {
    * @param conn
    *        The Sqlite connection.
    */
   async execute(conn) {
     // A search might be canceled before it starts.
     if (!this.pending)
       return;
 
+    // Used by stop() to interrupt an eventual running statement.
+    this.interrupt = () => {
+      // Interrupt any ongoing statement to run the search sooner.
+      if (!SwitchToTabStorage.updating) {
+        conn.interrupt();
+      }
+    }
+
     TelemetryStopwatch.start(TELEMETRY_1ST_RESULT, this);
     if (this._searchString)
       TelemetryStopwatch.start(TELEMETRY_6_FIRST_RESULTS, this);
 
     // Since we call the synchronous parseSubmissionURL function later, we must
     // wait for the initialization of PlacesSearchAutocompleteProvider first.
     await PlacesSearchAutocompleteProvider.ensureInitialized();
     if (!this.pending)
@@ -2284,21 +2309,21 @@ UnifiedComplete.prototype = {
       });
     }
     return this._promiseDatabase;
   },
 
   // mozIPlacesAutoComplete
 
   registerOpenPage(uri, userContextId) {
-    SwitchToTabStorage.add(uri, userContextId);
+    SwitchToTabStorage.add(uri, userContextId).catch(Cu.reportError);
   },
 
   unregisterOpenPage(uri, userContextId) {
-    SwitchToTabStorage.delete(uri, userContextId);
+    SwitchToTabStorage.delete(uri, userContextId).catch(Cu.reportError);
   },
 
   populatePreloadedSiteStorage(json) {
     PreloadedSiteStorage.populate(json);
   },
 
   // nsIAutoCompleteSearch
 
--- a/toolkit/modules/Sqlite.jsm
+++ b/toolkit/modules/Sqlite.jsm
@@ -665,16 +665,22 @@ ConnectionData.prototype = Object.freeze
       ++count;
       statement.finalize();
     }
     this._cachedStatements.clear();
     this._log.debug("Discarded " + count + " cached statements.");
     return count;
   },
 
+  interrupt() {
+    this._log.info("Trying to interrupt.");
+    this.ensureOpen();
+    this._dbConn.interrupt();
+  },
+
   /**
    * Helper method to bind parameters of various kinds through
    * reflection.
    */
   _bindParameters(statement, params) {
     if (!params) {
       return;
     }
@@ -788,34 +794,23 @@ ConnectionData.prototype = Object.freeze
       },
 
       handleCompletion(reason) {
         self._log.debug("Stmt #" + index + " finished.");
         self._pendingStatements.delete(index);
 
         switch (reason) {
           case Ci.mozIStorageStatementCallback.REASON_FINISHED:
+          case Ci.mozIStorageStatementCallback.REASON_CANCELED:
             // If there is an onRow handler, we always instead resolve to a
             // boolean indicating whether the onRow handler was called or not.
             let result = onRow ? handledRow : rows;
             deferred.resolve(result);
             break;
 
-          case Ci.mozIStorageStatementCallback.REASON_CANCELED:
-            // It is not an error if the user explicitly requested cancel via
-            // the onRow handler.
-            if (userCancelled) {
-              let result = onRow ? handledRow : rows;
-              deferred.resolve(result);
-            } else {
-              deferred.reject(new Error("Statement was cancelled."));
-            }
-
-            break;
-
           case Ci.mozIStorageStatementCallback.REASON_ERROR:
             let error = new Error("Error(s) encountered during statement execution: " + errors.map(e => e.message).join(", "));
             error.errors = errors;
             deferred.reject(error);
             break;
 
           default:
             deferred.reject(new Error("Unknown completion reason code: " +
@@ -1444,16 +1439,25 @@ OpenedConnection.prototype = Object.free
    * execution: we finalize all statements, which is only safe if
    * they will not be executed again.
    *
    * @return (integer) the number of statements discarded.
    */
   discardCachedStatements() {
     return this._connectionData.discardCachedStatements();
   },
+
+  /**
+   * Interrupts pending database operations returning at the first opportunity.
+   * Statement execution will throw an NS_ERROR_ABORT failure.
+   * Can only be used on read-only connections.
+   */
+  interrupt() {
+    this._connectionData.interrupt();
+  },
 });
 
 this.Sqlite = {
   openConnection,
   cloneStorageConnection,
   wrapStorageConnection,
   /**
    * Shutdown barrier client. May be used by clients to perform last-minute
--- a/toolkit/modules/tests/xpcshell/test_sqlite.js
+++ b/toolkit/modules/tests/xpcshell/test_sqlite.js
@@ -42,27 +42,28 @@ function getConnection(dbName, extraOpti
   for (let [k, v] of Object.entries(extraOptions)) {
     options[k] = v;
   }
 
   return Sqlite.openConnection(options);
 }
 
 async function getDummyDatabase(name, extraOptions = {}) {
-  const TABLES = {
-    dirs: "id INTEGER PRIMARY KEY AUTOINCREMENT, path TEXT",
-    files: "id INTEGER PRIMARY KEY AUTOINCREMENT, dir_id INTEGER, path TEXT",
-  };
-
   let c = await getConnection(name, extraOptions);
   c._initialStatementCount = 0;
 
-  for (let [k, v] of Object.entries(TABLES)) {
-    await c.execute("CREATE TABLE " + k + "(" + v + ")");
-    c._initialStatementCount++;
+  if (!extraOptions.readOnly) {
+    const TABLES = new Map([
+      ["dirs", "id INTEGER PRIMARY KEY AUTOINCREMENT, path TEXT"],
+      ["files", "id INTEGER PRIMARY KEY AUTOINCREMENT, dir_id INTEGER, path TEXT"],
+    ]);
+    for (let [k, v] of TABLES) {
+      await c.execute("CREATE TABLE " + k + "(" + v + ")");
+      c._initialStatementCount++;
+    }
   }
 
   return c;
 }
 
 async function getDummyTempDatabase(name, extraOptions = {}) {
   const TABLES = {
     dirs: "id INTEGER PRIMARY KEY AUTOINCREMENT, path TEXT",
@@ -1135,8 +1136,21 @@ add_task(async function test_datatypes()
       // In Sqlite bool is stored and then retrieved as numeric.
       let val = typeof binding[colName] == "boolean" ? +binding[colName]
                                                        : binding[colName];
       Assert.deepEqual(val, row.getResultByName(colName));
     }
   }
   await c.close();
 });
+
+add_task(async function test_interrupt() {
+  // Testing the interrupt functionality is left to mozStorage unit tests, here
+  // we'll just test error conditions.
+  let c = await getDummyDatabase("interrupt");
+  Assert.throws(() => c.interrupt(),
+                /NS_ERROR_ILLEGAL_VALUE/,
+                "Sqlite.interrupt() should throw on a writable connection");
+  await c.close();
+  Assert.throws(() => c.interrupt(),
+                /Connection is not open/,
+                "Sqlite.interrupt() should throw on a closed connection");
+});