Bug 1320301 - Add partial support to sqlite3_interrupt. r=asuth draft
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 31 Jul 2017 22:27:23 +0200
changeset 652050 6f559b473c32dfcfb2bfe919c53199e202f7d636
parent 651207 b911a4c97fde5d8bdeebfd5d0266ee9f7b9e59b2
child 727957 40a83c8f8cfb9daccbb02e0dd844adfa6d283a66
push id75914
push usermak77@bonardo.net
push dateThu, 24 Aug 2017 10:08:52 +0000
reviewersasuth
bugs1320301
milestone57.0a1
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");
+});