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 idunknown
push userunknown
push dateunknown
bugs635482
milestone2.0b13pre
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 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;
     }