Backed out changeset af815754d58a (bug 1635489) for perma failures on…

… test_statement_executeAsync.js. CLOSED TREE
jbstans · Aug 10, 2020 · 550d472 · 550d472
1 parent ced6b30
commit 550d472
Show file tree

Hide file tree

Showing 20 changed files with 82 additions and 595 deletions.
diff --git a/dom/cache/DBAction.cpp b/dom/cache/DBAction.cpp
@@ -226,7 +226,7 @@ nsresult OpenDBConnection(const QuotaInfo& aQuotaInfo, nsIFile* aDBFile,
   }
 
   nsCOMPtr<mozIStorageConnection> conn;
-  rv = ss->OpenDatabaseWithFileURL(dbFileUrl, EmptyCString(), getter_AddRefs(conn));
+  rv = ss->OpenDatabaseWithFileURL(dbFileUrl, getter_AddRefs(conn));
   if (rv == NS_ERROR_FILE_CORRUPTED) {
     NS_WARNING("Cache database corrupted. Recreating empty database.");
 
@@ -239,7 +239,7 @@ nsresult OpenDBConnection(const QuotaInfo& aQuotaInfo, nsIFile* aDBFile,
       return rv;
     }
 
-    rv = ss->OpenDatabaseWithFileURL(dbFileUrl, EmptyCString(), getter_AddRefs(conn));
+    rv = ss->OpenDatabaseWithFileURL(dbFileUrl, getter_AddRefs(conn));
   }
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
@@ -258,7 +258,7 @@ nsresult OpenDBConnection(const QuotaInfo& aQuotaInfo, nsIFile* aDBFile,
       return rv;
     }
 
-    rv = ss->OpenDatabaseWithFileURL(dbFileUrl, EmptyCString(), getter_AddRefs(conn));
+    rv = ss->OpenDatabaseWithFileURL(dbFileUrl, getter_AddRefs(conn));
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }

diff --git a/dom/indexedDB/ActorsParent.cpp b/dom/indexedDB/ActorsParent.cpp
@@ -3806,7 +3806,8 @@ nsresult UpgradeSchemaFrom25_0To26_0(mozIStorageConnection& aConnection) {
 }
 
 Result<nsCOMPtr<nsIFileURL>, nsresult> GetDatabaseFileURL(
-    nsIFile& aDatabaseFile, const int64_t aDirectoryLockId) {
+    nsIFile& aDatabaseFile, const int64_t aDirectoryLockId,
+    const uint32_t aTelemetryId) {
   MOZ_ASSERT(aDirectoryLockId >= -1);
 
   nsresult rv;
@@ -3839,9 +3840,17 @@ Result<nsCOMPtr<nsIFileURL>, nsresult> GetDatabaseFileURL(
           ? "&directoryLockId="_ns + IntCString(aDirectoryLockId)
           : EmptyCString();
 
+  nsAutoCString telemetryFilenameClause;
+  if (aTelemetryId) {
+    telemetryFilenameClause.AssignLiteral("&telemetryFilename=indexedDB-");
+    telemetryFilenameClause.AppendInt(aTelemetryId);
+    telemetryFilenameClause.Append(NS_ConvertUTF16toUTF8(kSQLiteSuffix));
+  }
+
   nsCOMPtr<nsIFileURL> result;
   rv = NS_MutateURI(mutator)
-           .SetQuery("cache=private"_ns + directoryLockIdClause)
+           .SetQuery("cache=private"_ns + directoryLockIdClause +
+                     telemetryFilenameClause)
            .Finalize(result);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return Err(rv);
@@ -3975,19 +3984,10 @@ struct StorageOpenTraits;
 template <>
 struct StorageOpenTraits<nsIFileURL> {
   static Result<MovingNotNull<nsCOMPtr<mozIStorageConnection>>, nsresult> Open(
-      mozIStorageService& aStorageService, nsIFileURL& aFileURL,
-      const uint32_t aTelemetryId = 0) {
-
-    nsAutoCString telemetryFilename;
-    if (aTelemetryId) {
-      telemetryFilename.AssignLiteral("indexedDB-");
-      telemetryFilename.AppendInt(aTelemetryId);
-      telemetryFilename.Append(NS_ConvertUTF16toUTF8(kSQLiteSuffix));
-    }
-
+      mozIStorageService& aStorageService, nsIFileURL& aFileURL) {
     nsCOMPtr<mozIStorageConnection> connection;
     nsresult rv = aStorageService.OpenDatabaseWithFileURL(
-        &aFileURL, telemetryFilename, getter_AddRefs(connection));
+        &aFileURL, getter_AddRefs(connection));
     return ValOrErr(std::move(connection), rv);
   }
 
@@ -4001,8 +4001,7 @@ struct StorageOpenTraits<nsIFileURL> {
 template <>
 struct StorageOpenTraits<nsIFile> {
   static Result<MovingNotNull<nsCOMPtr<mozIStorageConnection>>, nsresult> Open(
-      mozIStorageService& aStorageService, nsIFile& aFile,
-      const uint32_t aTelemetryId = 0) {
+      mozIStorageService& aStorageService, nsIFile& aFile) {
     nsCOMPtr<mozIStorageConnection> connection;
     nsresult rv = aStorageService.OpenUnsharedDatabase(
         &aFile, getter_AddRefs(connection));
@@ -4022,13 +4021,12 @@ struct StorageOpenTraits<nsIFile> {
 template <class FileOrURLType>
 Result<MovingNotNull<nsCOMPtr<mozIStorageConnection>>, nsresult>
 OpenDatabaseAndHandleBusy(mozIStorageService& aStorageService,
-                          FileOrURLType& aFileOrURL,
-                          const uint32_t aTelemetryId = 0) {
+                          FileOrURLType& aFileOrURL) {
   MOZ_ASSERT(!NS_IsMainThread());
   MOZ_ASSERT(!IsOnBackgroundThread());
 
   auto connectionOrErr =
-      StorageOpenTraits<FileOrURLType>::Open(aStorageService, aFileOrURL, aTelemetryId);
+      StorageOpenTraits<FileOrURLType>::Open(aStorageService, aFileOrURL);
 
   if (connectionOrErr.isErr() &&
       connectionOrErr.inspectErr() == NS_ERROR_STORAGE_BUSY) {
@@ -4054,7 +4052,7 @@ OpenDatabaseAndHandleBusy(mozIStorageService& aStorageService,
       PR_Sleep(PR_MillisecondsToInterval(100));
 
       connectionOrErr =
-          StorageOpenTraits<FileOrURLType>::Open(aStorageService, aFileOrURL, aTelemetryId);
+          StorageOpenTraits<FileOrURLType>::Open(aStorageService, aFileOrURL);
       if (!connectionOrErr.isErr() ||
           connectionOrErr.inspectErr() != NS_ERROR_STORAGE_BUSY ||
           TimeStamp::NowLoRes() - start > TimeDuration::FromSeconds(10)) {
@@ -4083,7 +4081,7 @@ CreateStorageConnection(nsIFile& aDBFile, nsIFile& aFMDirectory,
   bool exists;
 
   auto dbFileUrlOrErr =
-      GetDatabaseFileURL(aDBFile, aDirectoryLockId);
+      GetDatabaseFileURL(aDBFile, aDirectoryLockId, aTelemetryId);
   if (NS_WARN_IF(dbFileUrlOrErr.isErr())) {
     return dbFileUrlOrErr.propagateErr();
   }
@@ -4097,7 +4095,7 @@ CreateStorageConnection(nsIFile& aDBFile, nsIFile& aFMDirectory,
     return Err(rv);
   }
 
-  auto connectionOrErr = OpenDatabaseAndHandleBusy(*ss, *dbFileUrl, aTelemetryId);
+  auto connectionOrErr = OpenDatabaseAndHandleBusy(*ss, *dbFileUrl);
   if (connectionOrErr.isErr()) {
     if (connectionOrErr.inspectErr() == NS_ERROR_FILE_CORRUPTED) {
       // If we're just opening the database during origin initialization, then
@@ -4135,7 +4133,7 @@ CreateStorageConnection(nsIFile& aDBFile, nsIFile& aFMDirectory,
         }
       }
 
-      connectionOrErr = OpenDatabaseAndHandleBusy(*ss, *dbFileUrl, aTelemetryId);
+      connectionOrErr = OpenDatabaseAndHandleBusy(*ss, *dbFileUrl);
     }
 
     if (NS_WARN_IF(connectionOrErr.isErr())) {
@@ -4550,7 +4548,7 @@ GetStorageConnection(nsIFile& aDatabaseFile, const int64_t aDirectoryLockId,
   }
 
   auto dbFileUrlOrErr =
-      GetDatabaseFileURL(aDatabaseFile, aDirectoryLockId);
+      GetDatabaseFileURL(aDatabaseFile, aDirectoryLockId, aTelemetryId);
   if (NS_WARN_IF(dbFileUrlOrErr.isErr())) {
     return dbFileUrlOrErr.propagateErr();
   }
@@ -4562,7 +4560,7 @@ GetStorageConnection(nsIFile& aDatabaseFile, const int64_t aDirectoryLockId,
   }
 
   auto connectionOrErr =
-      OpenDatabaseAndHandleBusy(*ss, *dbFileUrlOrErr.inspect(), aTelemetryId);
+      OpenDatabaseAndHandleBusy(*ss, *dbFileUrlOrErr.inspect());
   if (NS_WARN_IF(connectionOrErr.isErr())) {
     return connectionOrErr.propagateErr();
   }

diff --git a/storage/mozIStorageService.idl b/storage/mozIStorageService.idl
@@ -161,12 +161,8 @@ interface mozIStorageService : nsISupports {
    *
    * @param aURL
    *        A nsIFileURL that represents the database that is to be opened.
-   * @param [optional] aTelemetryFilename
-   *        The name to use for the database in telemetry. Only needed if the
-   *        actual filename can contain sensitive information.
    */
-  mozIStorageConnection openDatabaseWithFileURL(in nsIFileURL aFileURL,
-                                                [optional] in ACString aTelemetryFilename);
+  mozIStorageConnection openDatabaseWithFileURL(in nsIFileURL aFileURL);
 
   /*
    * Utilities

diff --git a/storage/mozStorageAsyncStatementExecution.cpp b/storage/mozStorageAsyncStatementExecution.cpp
@@ -122,8 +122,8 @@ bool AsyncExecuteStatements::bindExecuteAndProcessStatement(
 
   sqlite3_stmt* aStatement = nullptr;
   // This cannot fail; we are only called if it's available.
-  Unused << aData.getSqliteStatement(&aStatement);
-  MOZ_DIAGNOSTIC_ASSERT(aStatement, "bindExecuteAndProcessStatement called without an initialized statement");
+  (void)aData.getSqliteStatement(&aStatement);
+  NS_ASSERTION(aStatement, "You broke the code; do not call here like that!");
   BindingParamsArray* paramsArray(aData);
 
   // Iterate through all of our parameters, bind them, and execute.
@@ -147,7 +147,7 @@ bool AsyncExecuteStatements::bindExecuteAndProcessStatement(
     // Advance our iterator, execute, and then process the statement.
     itr++;
     bool lastStatement = aLastStatement && itr == end;
-    continueProcessing = executeAndProcessStatement(aData, lastStatement);
+    continueProcessing = executeAndProcessStatement(aStatement, lastStatement);
 
     // Always reset our statement.
     (void)::sqlite3_reset(aStatement);
@@ -157,18 +157,13 @@ bool AsyncExecuteStatements::bindExecuteAndProcessStatement(
 }
 
 bool AsyncExecuteStatements::executeAndProcessStatement(
-    StatementData& aData, bool aLastStatement) {
+    sqlite3_stmt* aStatement, bool aLastStatement) {
   mMutex.AssertNotCurrentThreadOwns();
 
-  sqlite3_stmt* aStatement = nullptr;
-  // This cannot fail; we are only called if it's available.
-  Unused << aData.getSqliteStatement(&aStatement);
-  MOZ_DIAGNOSTIC_ASSERT(aStatement, "executeAndProcessStatement called without an initialized statement");
-
   // Execute our statement
   bool hasResults;
   do {
-    hasResults = executeStatement(aData);
+    hasResults = executeStatement(aStatement);
 
     // If we had an error, bail.
     if (mState == ERROR || mState == CANCELED) return false;
@@ -213,16 +208,11 @@ bool AsyncExecuteStatements::executeAndProcessStatement(
   return true;
 }
 
-bool AsyncExecuteStatements::executeStatement(StatementData& aData) {
+bool AsyncExecuteStatements::executeStatement(sqlite3_stmt* aStatement) {
   mMutex.AssertNotCurrentThreadOwns();
   Telemetry::AutoTimer<Telemetry::MOZ_STORAGE_ASYNC_REQUESTS_MS>
       finallySendExecutionDuration(mRequestStartDate);
 
-  sqlite3_stmt* aStatement = nullptr;
-  // This cannot fail; we are only called if it's available.
-  Unused << aData.getSqliteStatement(&aStatement);
-  MOZ_DIAGNOSTIC_ASSERT(aStatement, "executeStatement called without an initialized statement");
-
   bool busyRetry = false;
   while (true) {
     if (busyRetry) {
@@ -245,16 +235,6 @@ bool AsyncExecuteStatements::executeStatement(StatementData& aData) {
     SQLiteMutexAutoLock lockedScope(mDBMutex);
 
     int rc = mConnection->stepStatement(mNativeConnection, aStatement);
-
-    // Some errors are not fatal, and we can handle them and continue.
-    if (rc == SQLITE_BUSY) {
-      ::sqlite3_reset(aStatement);
-      busyRetry = true;
-      continue;
-    }
-
-    aData.MaybeRecordQueryStatus(rc);
-
     // Stop if we have no more results.
     if (rc == SQLITE_DONE) {
       Telemetry::Accumulate(Telemetry::MOZ_STORAGE_ASYNC_REQUESTS_SUCCESS,
@@ -269,6 +249,13 @@ bool AsyncExecuteStatements::executeStatement(StatementData& aData) {
       return true;
     }
 
+    // Some errors are not fatal, and we can handle them and continue.
+    if (rc == SQLITE_BUSY) {
+      ::sqlite3_reset(aStatement);
+      busyRetry = true;
+      continue;
+    }
+
     if (rc == SQLITE_INTERRUPT) {
       mState = CANCELED;
       return false;
@@ -561,7 +548,7 @@ AsyncExecuteStatements::Run() {
       if (!bindExecuteAndProcessStatement(mStatements[i], finished)) break;
     }
     // Otherwise, just execute and process the statement.
-    else if (!executeAndProcessStatement(mStatements[i], finished)) {
+    else if (!executeAndProcessStatement(stmt, finished)) {
       break;
     }
   }

diff --git a/storage/mozStorageAsyncStatementExecution.h b/storage/mozStorageAsyncStatementExecution.h
@@ -119,26 +119,26 @@ class AsyncExecuteStatements final : public Runnable,
    *
    * @pre mMutex is not held
    *
-   * @param aData
-   *        The StatementData to execute, and then process.
+   * @param aStatement
+   *        The statement to execute and then process.
    * @param aLastStatement
    *        Indicates if this is the last statement or not.  If it is, we have
    *        to set the proper state.
    * @returns true if we should continue to process statements, false otherwise.
    */
-  bool executeAndProcessStatement(StatementData& aData,
+  bool executeAndProcessStatement(sqlite3_stmt* aStatement,
                                   bool aLastStatement);
 
   /**
    * Executes a statement to completion, properly handling any error conditions.
    *
    * @pre mMutex is not held
    *
-   * @param aData
-   *        The StatementData to execute to completion.
+   * @param aStatement
+   *        The statement to execute to completion.
    * @returns true if results were obtained, false otherwise.
    */
-  bool executeStatement(StatementData& aData);
+  bool executeStatement(sqlite3_stmt* aStatement);
 
   /**
    * Builds a result set up with a row from a given statement.  If we meet the