Bug 635482 - Crash [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AbortIfOffMainThreadIfCheckFast ] (was: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P]) from nsCycleCollectingAutoRefCnt::decr with mozilla::storage::BindingParams::Release
authorShawn Wilsher <me@shawnwilsher.com>
Mon, 28 Feb 2011 11:52:31 -0800
changeset 63171 0e4b5530151d4881b58cd2ba62821391492e6639
parent 63170 a1d3010eff92e6f1e43ac123bc370aa183edb339
child 63172 e2a82bfadc6778b96da4762c94a99dd06fb77c59
push id1
push userroot
push dateTue, 10 Dec 2013 15:46:25 +0000
bugs635482
milestone2.0b13pre
Bug 635482 - Crash [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | AbortIfOffMainThreadIfCheckFast ] (was: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P]) from nsCycleCollectingAutoRefCnt::decr with mozilla::storage::BindingParams::Release Be very explicit about what thread we release objects on. r=asuth a=blocking
storage/src/mozStorageAsyncStatementExecution.cpp
storage/src/mozStorageStatementData.h
--- a/storage/src/mozStorageAsyncStatementExecution.cpp
+++ b/storage/src/mozStorageAsyncStatementExecution.cpp
@@ -11,17 +11,17 @@
  * Software distributed under the License is distributed on an "AS IS" basis,
  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
  * for the specific language governing rights and limitations under the
  * License.
  *
  * The Original Code is mozilla.org code.
  *
  * The Initial Developer of the Original Code is
- * Mozilla Corporation. 
+ * the Mozilla Foundation.
  * Portions created by the Initial Developer are Copyright (C) 2008
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
  *   Shawn Wilsher <me@shawnwilsher.com> (Original Author)
  *
  * Alternatively, the contents of this file may be used under the terms of
  * either the GNU General Public License Version 2 or later (the "GPL"), or
@@ -71,16 +71,17 @@ namespace storage {
 #define MAX_ROWS_PER_RESULT 15
 
 ////////////////////////////////////////////////////////////////////////////////
 //// Local Classes
 
 namespace {
 
 typedef AsyncExecuteStatements::ExecutionState ExecutionState;
+typedef AsyncExecuteStatements::StatementDataArray StatementDataArray;
 
 /**
  * Notifies a callback with a result set.
  */
 class CallbackResultNotifier : public nsRunnable
 {
 public:
   CallbackResultNotifier(mozIStorageStatementCallback *aCallback,
@@ -133,48 +134,53 @@ public:
 
 private:
   mozIStorageStatementCallback *mCallback;
   nsCOMPtr<mozIStorageError> mErrorObj;
   nsRefPtr<AsyncExecuteStatements> mEventStatus;
 };
 
 /**
- * Notifies the calling thread that the statement has finished executing.  Keeps
- * the AsyncExecuteStatements instance alive long enough so that it does not
- * get destroyed on the async thread if there are no other references alive.
+ * Notifies the calling thread that the statement has finished executing.  Takes
+ * ownership of the StatementData so it is released on the proper thread.
  */
 class CompletionNotifier : public nsRunnable
 {
 public:
   /**
-   * This takes ownership of the callback.  It is released on the thread this is
-   * dispatched to (which should always be the calling thread).
+   * This takes ownership of the callback and the StatementData.  They are
+   * released on the thread this is dispatched to (which should always be the
+   * calling thread).
    */
   CompletionNotifier(mozIStorageStatementCallback *aCallback,
                      ExecutionState aReason,
-                     AsyncExecuteStatements *aKeepAsyncAlive)
-    : mKeepAsyncAlive(aKeepAsyncAlive)
-    , mCallback(aCallback)
+                     StatementDataArray &aStatementData)
+    : mCallback(aCallback)
     , mReason(aReason)
   {
+    mStatementData.SwapElements(aStatementData);
   }
 
   NS_IMETHOD Run()
   {
     if (mCallback) {
       (void)mCallback->HandleCompletion(mReason);
       NS_RELEASE(mCallback);
     }
 
+    // The async thread could still hold onto a reference to us, so we need to
+    // make sure we release our reference to the StatementData now in case our
+    // destructor happens in a different thread.
+    mStatementData.Clear();
+
     return NS_OK;
   }
 
 private:
-  nsRefPtr<AsyncExecuteStatements> mKeepAsyncAlive;
+  StatementDataArray mStatementData;
   mozIStorageStatementCallback *mCallback;
   ExecutionState mReason;
 };
 
 } // anonymous namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 //// AsyncExecuteStatements
@@ -443,18 +449,19 @@ AsyncExecuteStatements::notifyComplete()
     }
     delete mTransactionManager;
     mTransactionManager = nsnull;
   }
 
   // Always generate a completion notification; it is what guarantees that our
   // destruction does not happen here on the async thread.
   nsRefPtr<CompletionNotifier> completionEvent =
-    new CompletionNotifier(mCallback, mState, this);
-  NS_ENSURE_TRUE(completionEvent, NS_ERROR_OUT_OF_MEMORY);
+    new CompletionNotifier(mCallback, mState, mStatements);
+  NS_ASSERTION(mStatements.IsEmpty(),
+               "Should have given up ownership of mStatements!");
 
   // We no longer own mCallback (the CompletionNotifier takes ownership).
   mCallback = nsnull;
 
   (void)mCallingThread->Dispatch(completionEvent, NS_DISPATCH_NORMAL);
 
   return NS_OK;
 }
--- a/storage/src/mozStorageStatementData.h
+++ b/storage/src/mozStorageStatementData.h
@@ -11,17 +11,17 @@
  * Software distributed under the License is distributed on an "AS IS" basis,
  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
  * for the specific language governing rights and limitations under the
  * License.
  *
  * The Original Code is mozilla.org code.
  *
  * The Initial Developer of the Original Code is
- * Mozilla Corporation.
+ * the Mozilla Foundation.
  * Portions created by the Initial Developer are Copyright (C) 2009
  * the Initial Developer. All Rights Reserved.
  *
  * Contributor(s):
  *   Shawn Wilsher <me@shawnwilsher.com> (Original Author)
  *
  * Alternatively, the contents of this file may be used under the terms of
  * either the GNU General Public License Version 2 or later (the "GPL"), or
@@ -39,16 +39,17 @@
 
 #ifndef mozStorageStatementData_h
 #define mozStorageStatementData_h
 
 #include "sqlite3.h"
 
 #include "nsAutoPtr.h"
 #include "nsTArray.h"
+#include "nsIEventTarget.h"
 
 #include "mozStorageBindingParamsArray.h"
 #include "mozIStorageBaseStatement.h"
 #include "mozStorageConnection.h"
 #include "StorageBaseStatementInternal.h"
 
 struct sqlite3_stmt;
 
@@ -60,22 +61,24 @@ class StatementData
 public:
   StatementData(sqlite3_stmt *aStatement,
                 already_AddRefed<BindingParamsArray> aParamsArray,
                 StorageBaseStatementInternal *aStatementOwner)
   : mStatement(aStatement)
   , mParamsArray(aParamsArray)
   , mStatementOwner(aStatementOwner)
   {
+    NS_PRECONDITION(mStatementOwner, "Must have a statement owner!");
   }
   StatementData(const StatementData &aSource)
   : mStatement(aSource.mStatement)
   , mParamsArray(aSource.mParamsArray)
   , mStatementOwner(aSource.mStatementOwner)
   {
+    NS_PRECONDITION(mStatementOwner, "Must have a statement owner!");
   }
   StatementData()
   {
   }
 
   /**
    * Return the sqlite statement, fetching it from the storage statement.  In
    * the case of AsyncStatements this may actually create the statement 
@@ -99,24 +102,33 @@ public:
   operator sqlite3 *() const
   {
     return mStatementOwner->getOwner()->GetNativeConnection();
   }
 
   /**
    * NULLs out our sqlite3_stmt (it is held by the owner) after reseting it and
    * clear all bindings to it.  This is expected to occur on the async thread.
-   *
-   * We do not clear mParamsArray out because we only want to release
-   * mParamsArray on the calling thread because of XPCVariant addref/release
-   * thread-safety issues.  The same holds for mStatementOwner which can be
-   * holding such a reference chain as well.
    */
   inline void finalize()
   {
+    NS_PRECONDITION(mStatementOwner, "Must have a statement owner!");
+#ifdef DEBUG
+    {
+      nsCOMPtr<nsIEventTarget> asyncThread =
+        mStatementOwner->getOwner()->getAsyncExecutionTarget();
+      // It's possible that we are shutting down the async thread, and this
+      // method would return NULL as a result.
+      if (asyncThread) {
+        PRBool onAsyncThread;
+        NS_ASSERTION(NS_SUCCEEDED(asyncThread->IsOnCurrentThread(&onAsyncThread)) && onAsyncThread,
+                     "This should only be running on the async thread!");
+      }
+    }
+#endif
     // In the AsyncStatement case we may never have populated mStatement if the
     // AsyncExecuteStatements got canceled or a failure occurred in constructing
     // the statement.
     if (mStatement) {
       (void)::sqlite3_reset(mStatement);
       (void)::sqlite3_clear_bindings(mStatement);
       mStatement = NULL;
     }