Bug 1914723 - Remove no longer necessary proxy release for Storage variants. r=asuth

`mParamsArray` doesn't need to be released on the main-thread as it doesn't
contain anymore instances of XPCVariant. The variants used are derived from
thread-safe Storage Variant_base.

Differential Revision: https://phabricator.services.mozilla.com/D220100

storage/mozStorageBindingParams.cpp

@@ -130,8 +130,9 @@ already_AddRefed<mozIStorageError> BindingParams::bind(
       // object with the right message.  Note that we special case
       // SQLITE_MISMATCH, but otherwise get the message from SQLite.
       const char* msg = "Could not covert nsIVariant to SQLite type.";
-      if (rc != SQLITE_MISMATCH)
+      if (rc != SQLITE_MISMATCH) {
         msg = ::sqlite3_errmsg(::sqlite3_db_handle(aStatement));
+      }
 
       nsCOMPtr<mozIStorageError> err(new Error(rc, msg));
       return err.forget();
@@ -143,10 +144,6 @@ already_AddRefed<mozIStorageError> BindingParams::bind(
 
 already_AddRefed<mozIStorageError> AsyncBindingParams::bind(
     sqlite3_stmt* aStatement) {
-  // We should bind by index using the super-class if there is nothing in our
-  // hashtable.
-  if (!mNamedParameters.Count()) return BindingParams::bind(aStatement);
-
   nsCOMPtr<mozIStorageError> err;
 
   for (const auto& entry : mNamedParameters) {
@@ -162,29 +159,19 @@ already_AddRefed<mozIStorageError> AsyncBindingParams::bind(
       nsAutoCString errMsg(key);
       errMsg.AppendLiteral(" is not a valid named parameter.");
       err = new Error(SQLITE_RANGE, errMsg.get());
-      break;
-    }
-
-    // XPCVariant's AddRef and Release are not thread-safe and so we must not
-    // do anything that would invoke them here on the async thread.  As such we
-    // can't cram aValue into mParameters using ReplaceObjectAt so that
-    // we can freeload off of the BindingParams::Bind implementation.
-    int rc = variantToSQLiteT(BindingColumnData(aStatement, oneIdx - 1),
-                              entry.GetWeak());
-    if (rc != SQLITE_OK) {
-      // We had an error while trying to bind.  Now we need to create an error
-      // object with the right message.  Note that we special case
-      // SQLITE_MISMATCH, but otherwise get the message from SQLite.
-      const char* msg = "Could not covert nsIVariant to SQLite type.";
-      if (rc != SQLITE_MISMATCH) {
-        msg = ::sqlite3_errmsg(::sqlite3_db_handle(aStatement));
-      }
-      err = new Error(rc, msg);
-      break;
-    }
-  }
-
       return err.forget();
+    }
+
+    if (mParameters.Length() <= static_cast<uint32_t>(oneIdx - 1)) {
+      mParameters.SetLength(oneIdx - 1);
+      mParameters.AppendElement(entry.GetWeak());
+    } else {
+      mParameters.ReplaceElementAt(oneIdx - 1, entry.GetWeak());
+    }
+  }
+
+  // Now bind using the super class.
+  return BindingParams::bind(aStatement);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -208,8 +195,7 @@ AsyncBindingParams::BindByName(const nsACString& aName, nsIVariant* aValue) {
 
   RefPtr<Variant_base> variant = convertVariantToStorageVariant(aValue);
   if (!variant) return NS_ERROR_UNEXPECTED;
-
-  mNamedParameters.InsertOrUpdate(aName, nsCOMPtr<nsIVariant>{variant});
+  mNamedParameters.InsertOrUpdate(aName, std::move(variant));
   return NS_OK;
 }
 

storage/mozStorageBindingParams.h

@@ -7,20 +7,16 @@
 #ifndef mozStorageBindingParams_h
 #define mozStorageBindingParams_h
 
-#include "nsCOMArray.h"
 #include "nsIVariant.h"
-#include "nsInterfaceHashtable.h"
 
-#include "mozStorageBindingParamsArray.h"
 #include "mozStorageStatement.h"
 #include "mozStorageAsyncStatement.h"
 #include "Variant.h"
-
+#include "nsTHashMap.h"
 #include "mozIStorageBindingParams.h"
 #include "IStorageBindingParamsInternal.h"
 
-namespace mozilla {
-namespace storage {
+namespace mozilla::storage {
 
 class BindingParams : public mozIStorageBindingParams,
                       public IStorageBindingParamsInternal {
@@ -58,9 +54,13 @@ class BindingParams : public mozIStorageBindingParams,
   virtual ~BindingParams() {}
 
   explicit BindingParams(mozIStorageBindingParamsArray* aOwningArray);
+
   // Note that this is managed as a sparse array, so particular caution should
   // be used for out-of-bounds usage.
-  nsTArray<RefPtr<Variant_base> > mParameters;
+  // Variants added to this array must be thread-safe, thus you should not use
+  // XPCVariant or similar main-thread only implementations.
+  nsTArray<RefPtr<Variant_base>> mParameters;
+
   bool mLocked;
 
  private:
@@ -102,10 +102,11 @@ class AsyncBindingParams : public BindingParams {
   virtual ~AsyncBindingParams() {}
 
  private:
-  nsInterfaceHashtable<nsCStringHashKey, nsIVariant> mNamedParameters;
+  // Variants added to this array must be thread-safe, thus you should not use
+  // XPCVariant or similar main-thread only implementations.
+  nsTHashMap<nsCStringHashKey, RefPtr<Variant_base>> mNamedParameters;
 };
 
-}  // namespace storage
-}  // namespace mozilla
+}  // namespace mozilla::storage
 
 #endif  // mozStorageBindingParams_h

storage/mozStorageStatementData.h

@@ -10,7 +10,6 @@
 #include "sqlite3.h"
 
 #include "nsTArray.h"
-#include "MainThreadUtils.h"
 
 #include "mozStorageBindingParamsArray.h"
 #include "mozStorageConnection.h"
@@ -19,8 +18,7 @@
 
 struct sqlite3_stmt;
 
-namespace mozilla {
-namespace storage {
+namespace mozilla::storage {
 
 class StatementData {
  public:
@@ -41,13 +39,6 @@ class StatementData {
     MOZ_ASSERT(mStatementOwner, "Must have a statement owner!");
   }
   StatementData() : mStatement(nullptr), mQueryStatusRecorded(false) {}
-  ~StatementData() {
-    // We need to ensure that mParamsArray is released on the main thread,
-    // as the binding arguments may be XPConnect values, which are safe
-    // to release only on the main thread.
-    NS_ReleaseOnMainThread("StatementData::mParamsArray",
-                           mParamsArray.forget());
-  }
 
   /**
    * Return the sqlite statement, fetching it from the storage statement.  In
@@ -127,7 +118,13 @@ class StatementData {
 
  private:
   sqlite3_stmt* mStatement;
+
+  /**
+   * It's safe to release this on any thread, as it holds `BindingParams` that
+   * can only contain thread-safe derivates of `Variant_base`.
+   */
   RefPtr<BindingParamsArray> mParamsArray;
+
   bool mQueryStatusRecorded;
 
   /**
@@ -137,7 +134,6 @@ class StatementData {
   nsCOMPtr<StorageBaseStatementInternal> mStatementOwner;
 };
 
-}  // namespace storage
-}  // namespace mozilla
+}  // namespace mozilla::storage
 
 #endif  // mozStorageStatementData_h