Bug 1110487 P3 Cache should ensure Actions are finished before completing. r=ehsan
authorBen Kelly <ben@wanderview.com>
Mon, 23 Mar 2015 22:23:45 -0400
changeset 264062 1a468067441b53e56717df11f868125d6e144638
parent 264061 a13277e46eb96344cc4fb36d036d6b59ba287db4
child 264063 fa8fa3aa9ea63588dfad9dcaf70f0ba0a163758f
push id4718
push userraliiev@mozilla.com
push dateMon, 11 May 2015 18:39:53 +0000
treeherdermozilla-beta@c20c4ef55f08 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1110487
milestone39.0a1
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 1110487 P3 Cache should ensure Actions are finished before completing. r=ehsan
dom/cache/Action.cpp
dom/cache/Action.h
dom/cache/Context.cpp
dom/cache/Context.h
--- a/dom/cache/Action.cpp
+++ b/dom/cache/Action.cpp
@@ -5,18 +5,16 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/dom/cache/Action.h"
 
 namespace mozilla {
 namespace dom {
 namespace cache {
 
-NS_IMPL_ISUPPORTS0(mozilla::dom::cache::Action::Resolver);
-
 void
 Action::CancelOnInitiatingThread()
 {
   NS_ASSERT_OWNINGTHREAD(Action);
   MOZ_ASSERT(!mCanceled);
   mCanceled = true;
 }
 
--- a/dom/cache/Action.h
+++ b/dom/cache/Action.h
@@ -13,31 +13,29 @@
 
 namespace mozilla {
 namespace dom {
 namespace cache {
 
 class Action
 {
 public:
-  class Resolver : public nsISupports
+  class Resolver
   {
-  protected:
-    // virtual because deleted through base class pointer
-    virtual ~Resolver() { }
-
   public:
     // Note: Action must drop Resolver ref after calling Resolve()!
     // Note: Must be called on the same thread used to execute
     //       Action::RunOnTarget().
     virtual void Resolve(nsresult aRv) = 0;
 
-    // We must use ISUPPORTS for our refcounting here because sub-classes also
-    // want to inherit interfaces like nsIRunnable.
-    NS_DECL_THREADSAFE_ISUPPORTS
+    NS_IMETHOD_(MozExternalRefCountType)
+    AddRef(void) = 0;
+
+    NS_IMETHOD_(MozExternalRefCountType)
+    Release(void) = 0;
   };
 
   // Execute operations on the target thread.  Once complete call
   // Resolver::Resolve().  This can be done sync or async.
   // Note: Action should hold Resolver ref until its ready to call Resolve().
   // Note: The "target" thread is determined when the Action is scheduled on
   //       Context.  The Action should not assume any particular thread is used.
   virtual void RunOnTarget(Resolver* aResolver, const QuotaInfo& aQuotaInfo) = 0;
--- a/dom/cache/Context.cpp
+++ b/dom/cache/Context.cpp
@@ -1,16 +1,17 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/dom/cache/Context.h"
 
+#include "mozilla/AutoRestore.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/dom/cache/Action.h"
 #include "mozilla/dom/cache/Manager.h"
 #include "mozilla/dom/cache/ManagerId.h"
 #include "mozilla/dom/cache/OfflineStorage.h"
 #include "mozilla/dom/quota/OriginOrPatternString.h"
 #include "mozilla/dom/quota/QuotaManager.h"
 #include "nsIFile.h"
@@ -64,17 +65,16 @@ using mozilla::dom::quota::OriginOrPatte
 using mozilla::dom::quota::QuotaManager;
 using mozilla::dom::quota::PERSISTENCE_TYPE_DEFAULT;
 using mozilla::dom::quota::PersistenceType;
 
 // Executed to perform the complicated dance of steps necessary to initialize
 // the QuotaManager.  This must be performed for each origin before any disk
 // IO occurrs.
 class Context::QuotaInitRunnable final : public nsIRunnable
-                                       , public Action::Resolver
 {
 public:
   QuotaInitRunnable(Context* aContext,
                     Manager* aManager,
                     Action* aQuotaIOThreadAction)
     : mContext(aContext)
     , mThreadsafeHandle(aContext->CreateThreadsafeHandle())
     , mManager(aManager)
@@ -98,36 +98,45 @@ public:
     nsresult rv = NS_DispatchToMainThread(this, nsIThread::DISPATCH_NORMAL);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       mState = STATE_COMPLETE;
       Clear();
     }
     return rv;
   }
 
-  virtual void Resolve(nsresult aRv) override
+private:
+  class SyncResolver final : public Action::Resolver
   {
-    // Depending on the error or success path, this can run on either the
-    // main thread or the QuotaManager IO thread.  The IO thread is an
-    // idle thread which may be destroyed and recreated, so its hard to
-    // assert on.
-    MOZ_ASSERT(mState == STATE_RUNNING || NS_FAILED(aRv));
+  public:
+    SyncResolver()
+      : mResolved(false)
+      , mResult(NS_OK)
+    { }
 
-    mResult = aRv;
-    mState = STATE_COMPLETING;
+    virtual void
+    Resolve(nsresult aRv) override
+    {
+      MOZ_ASSERT(!mResolved);
+      mResolved = true;
+      mResult = aRv;
+    };
 
-    nsresult rv = mInitiatingThread->Dispatch(this, nsIThread::DISPATCH_NORMAL);
-    if (NS_FAILED(rv)) {
-      // Shutdown must be delayed until all Contexts are destroyed.  Crash for
-      // this invariant violation.
-      MOZ_CRASH("Failed to dispatch QuotaInitRunnable to initiating thread.");
-    }
-  }
+    bool Resolved() const { return mResolved; }
+    nsresult Result() const { return mResult; }
+
+  private:
+    ~SyncResolver() { }
 
-private:
+    bool mResolved;
+    nsresult mResult;
+
+    NS_INLINE_DECL_REFCOUNTING(Context::QuotaInitRunnable::SyncResolver, override)
+  };
+
   ~QuotaInitRunnable()
   {
     MOZ_ASSERT(mState == STATE_COMPLETE);
     MOZ_ASSERT(!mContext);
     MOZ_ASSERT(!mQuotaIOThreadAction);
   }
 
   enum State
@@ -157,22 +166,21 @@ private:
   nsCOMPtr<nsIThread> mInitiatingThread;
   nsresult mResult;
   QuotaInfo mQuotaInfo;
   nsMainThreadPtrHandle<OfflineStorage> mOfflineStorage;
   State mState;
   bool mNeedsQuotaRelease;
 
 public:
-  NS_DECL_ISUPPORTS_INHERITED
+  NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIRUNNABLE
 };
 
-NS_IMPL_ISUPPORTS_INHERITED(mozilla::dom::cache::Context::QuotaInitRunnable,
-                            Action::Resolver, nsIRunnable);
+NS_IMPL_ISUPPORTS(mozilla::dom::cache::Context::QuotaInitRunnable, nsIRunnable);
 
 // The QuotaManager init state machine is represented in the following diagram:
 //
 //    +---------------+
 //    |     Start     |      Resolve(error)
 //    | (Orig Thread) +---------------------+
 //    +-------+-------+                     |
 //            |                             |
@@ -187,71 +195,72 @@ NS_IMPL_ISUPPORTS_INHERITED(mozilla::dom
 //   +--------+---------+                   |
 //            |                             |
 // +----------v------------+                |
 // |EnsureOriginInitialized| Resolve(error) |
 // |   (Quota IO Thread)   +----------------+
 // +----------+------------+                |
 //            |                             |
 //  +---------v---------+            +------v------+
-//  |      Running      |  Resolve() |  Completing |
+//  |      Running      |            |  Completing |
 //  | (Quota IO Thread) +------------>(Orig Thread)|
 //  +-------------------+            +------+------+
 //                                          |
 //                                    +-----v----+
 //                                    | Complete |
 //                                    +----------+
 //
 // The initialization process proceeds through the main states.  If an error
-// occurs, then we transition back to Completing state back on the original
-// thread.
+// occurs, then we transition to Completing state back on the original thread.
 NS_IMETHODIMP
 Context::QuotaInitRunnable::Run()
 {
   // May run on different threads depending on the state.  See individual
   // state cases for thread assertions.
 
+  nsRefPtr<SyncResolver> resolver = new SyncResolver();
+
   switch(mState) {
     // -----------------------------------
     case STATE_CALL_WAIT_FOR_OPEN_ALLOWED:
     {
       MOZ_ASSERT(NS_IsMainThread());
       QuotaManager* qm = QuotaManager::GetOrCreate();
       if (!qm) {
-        Resolve(NS_ERROR_FAILURE);
-        return NS_OK;
+        resolver->Resolve(NS_ERROR_FAILURE);
+        break;
       }
 
       nsRefPtr<ManagerId> managerId = mManager->GetManagerId();
       nsCOMPtr<nsIPrincipal> principal = managerId->Principal();
       nsresult rv = qm->GetInfoFromPrincipal(principal,
                                              &mQuotaInfo.mGroup,
                                              &mQuotaInfo.mOrigin,
                                              &mQuotaInfo.mIsApp);
       if (NS_WARN_IF(NS_FAILED(rv))) {
-        Resolve(rv);
-        return NS_OK;
+        resolver->Resolve(rv);
+        break;
       }
 
       QuotaManager::GetStorageId(PERSISTENCE_TYPE_DEFAULT,
                                  mQuotaInfo.mOrigin,
                                  Client::DOMCACHE,
                                  NS_LITERAL_STRING("cache"),
                                  mQuotaInfo.mStorageId);
 
       // QuotaManager::WaitForOpenAllowed() will hold a reference to us as
       // a callback.  We will then get executed again on the main thread when
       // it is safe to open the quota directory.
       mState = STATE_WAIT_FOR_OPEN_ALLOWED;
       rv = qm->WaitForOpenAllowed(OriginOrPatternString::FromOrigin(mQuotaInfo.mOrigin),
                                   Nullable<PersistenceType>(PERSISTENCE_TYPE_DEFAULT),
                                   mQuotaInfo.mStorageId, this);
       if (NS_FAILED(rv)) {
-        Resolve(rv);
-        return NS_OK;
+        resolver->Resolve(rv);
+        break;
       }
       break;
     }
     // ------------------------------
     case STATE_WAIT_FOR_OPEN_ALLOWED:
     {
       MOZ_ASSERT(NS_IsMainThread());
 
@@ -262,18 +271,18 @@ Context::QuotaInitRunnable::Run()
 
       nsRefPtr<OfflineStorage> offlineStorage =
         OfflineStorage::Register(mThreadsafeHandle, mQuotaInfo);
       mOfflineStorage = new nsMainThreadPtrHolder<OfflineStorage>(offlineStorage);
 
       mState = STATE_ENSURE_ORIGIN_INITIALIZED;
       nsresult rv = qm->IOThread()->Dispatch(this, nsIThread::DISPATCH_NORMAL);
       if (NS_WARN_IF(NS_FAILED(rv))) {
-        Resolve(rv);
-        return NS_OK;
+        resolver->Resolve(rv);
+        break;
       }
       break;
     }
     // ----------------------------------
     case STATE_ENSURE_ORIGIN_INITIALIZED:
     {
       // Can't assert quota IO thread because its an idle thread that can get
       // recreated.  At least assert we're not on main thread or owning thread.
@@ -283,31 +292,31 @@ Context::QuotaInitRunnable::Run()
       QuotaManager* qm = QuotaManager::Get();
       MOZ_ASSERT(qm);
       nsresult rv = qm->EnsureOriginIsInitialized(PERSISTENCE_TYPE_DEFAULT,
                                                   mQuotaInfo.mGroup,
                                                   mQuotaInfo.mOrigin,
                                                   mQuotaInfo.mIsApp,
                                                   getter_AddRefs(mQuotaInfo.mDir));
       if (NS_FAILED(rv)) {
-        Resolve(rv);
-        return NS_OK;
+        resolver->Resolve(rv);
+        break;
       }
 
       mState = STATE_RUNNING;
 
       if (!mQuotaIOThreadAction) {
-        Resolve(NS_OK);
-        return NS_OK;
+        resolver->Resolve(NS_OK);
+        break;
       }
 
-      // Execute the provided initialization Action.  We pass ourselves as the
-      // Resolver.  The Action must either call Resolve() immediately or hold
-      // a ref to us and call Resolve() later.
-      mQuotaIOThreadAction->RunOnTarget(this, mQuotaInfo);
+      // Execute the provided initialization Action.  The Action must Resolve()
+      // before returning.
+      mQuotaIOThreadAction->RunOnTarget(resolver, mQuotaInfo);
+      MOZ_ASSERT(resolver->Resolved());
 
       break;
     }
     // -------------------
     case STATE_COMPLETING:
     {
       NS_ASSERT_OWNINGTHREAD(Action::Resolver);
       if (mQuotaIOThreadAction) {
@@ -326,20 +335,30 @@ Context::QuotaInitRunnable::Run()
       // the threads we have bounced through.
       Clear();
       break;
     }
     // -----
     default:
     {
       MOZ_CRASH("unexpected state in QuotaInitRunnable");
-      break;
     }
   }
 
+  if (resolver->Resolved()) {
+    MOZ_ASSERT(mState == STATE_RUNNING || NS_FAILED(resolver->Result()));
+
+    MOZ_ASSERT(NS_SUCCEEDED(mResult));
+    mResult = resolver->Result();
+
+    mState = STATE_COMPLETING;
+    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
+      mInitiatingThread->Dispatch(this, nsIThread::DISPATCH_NORMAL)));
+  }
+
   return NS_OK;
 }
 
 // Runnable wrapper around Action objects dispatched on the Context.  This
 // runnable executes the Action on the appropriate threads while the Context
 // is initialized.
 class Context::ActionRunnable final : public nsIRunnable
                                     , public Action::Resolver
@@ -350,16 +369,17 @@ public:
                  const QuotaInfo& aQuotaInfo)
     : mContext(aContext)
     , mTarget(aTarget)
     , mAction(aAction)
     , mQuotaInfo(aQuotaInfo)
     , mInitiatingThread(NS_GetCurrentThread())
     , mState(STATE_INIT)
     , mResult(NS_OK)
+    , mExecutingRunOnTarget(false)
   {
     MOZ_ASSERT(mContext);
     MOZ_ASSERT(mTarget);
     MOZ_ASSERT(mAction);
     MOZ_ASSERT(mQuotaInfo.mDir);
     MOZ_ASSERT(mInitiatingThread);
   }
 
@@ -390,24 +410,36 @@ public:
     NS_ASSERT_OWNINGTHREAD(Action::Resolver);
     mAction->CancelOnInitiatingThread();
   }
 
   virtual void Resolve(nsresult aRv) override
   {
     MOZ_ASSERT(mTarget == NS_GetCurrentThread());
     MOZ_ASSERT(mState == STATE_RUNNING);
+
     mResult = aRv;
-    mState = STATE_COMPLETING;
-    nsresult rv = mInitiatingThread->Dispatch(this, nsIThread::DISPATCH_NORMAL);
-    if (NS_FAILED(rv)) {
-      // Shutdown must be delayed until all Contexts are destroyed.  Crash
-      // for this invariant violation.
-      MOZ_CRASH("Failed to dispatch ActionRunnable to initiating thread.");
+
+    // We ultimately must complete on the initiating thread, but bounce through
+    // the current thread again to ensure that we don't destroy objects and
+    // state out from under the currently running action's stack.
+    mState = STATE_RESOLVING;
+
+    // If we were resolved synchronously within Action::RunOnTarget() then we
+    // can avoid a thread bounce and just resolve once RunOnTarget() returns.
+    // The Run() method will handle this by looking at mState after
+    // RunOnTarget() returns.
+    if (mExecutingRunOnTarget) {
+      return;
     }
+
+    // Otherwise we are in an asynchronous resolve.  And must perform a thread
+    // bounce to run on the target thread again.
+    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
+      mTarget->Dispatch(this, nsIThread::DISPATCH_NORMAL)));
   }
 
 private:
   ~ActionRunnable()
   {
     MOZ_ASSERT(mState == STATE_COMPLETE);
     MOZ_ASSERT(!mContext);
     MOZ_ASSERT(!mAction);
@@ -423,51 +455,60 @@ private:
     mAction = nullptr;
   }
 
   enum State
   {
     STATE_INIT,
     STATE_RUN_ON_TARGET,
     STATE_RUNNING,
+    STATE_RESOLVING,
     STATE_COMPLETING,
     STATE_COMPLETE
   };
 
   nsRefPtr<Context> mContext;
   nsCOMPtr<nsIEventTarget> mTarget;
   nsRefPtr<Action> mAction;
   const QuotaInfo mQuotaInfo;
   nsCOMPtr<nsIThread> mInitiatingThread;
   State mState;
   nsresult mResult;
 
+  // Only accessible on target thread;
+  bool mExecutingRunOnTarget;
+
 public:
-  NS_DECL_ISUPPORTS_INHERITED
+  NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIRUNNABLE
 };
 
-NS_IMPL_ISUPPORTS_INHERITED(mozilla::dom::cache::Context::ActionRunnable,
-                            Action::Resolver, nsIRunnable);
+NS_IMPL_ISUPPORTS(mozilla::dom::cache::Context::ActionRunnable, nsIRunnable);
 
 // The ActionRunnable has a simpler state machine.  It basically needs to run
 // the action on the target thread and then complete on the original thread.
 //
 //   +-------------+
 //   |    Start    |
 //   |(Orig Thread)|
 //   +-----+-------+
 //         |
 // +-------v---------+
 // |  RunOnTarget    |
-// |Target IO Thread)+-------------------------------+
-// +-------+---------+                               |
-//         |                                         |
-// +-------v----------+ Resolve()            +-------v-----+
-// |     Running      |                      |  Completing |
+// |Target IO Thread)+---+ Resolve()
+// +-------+---------+   |
+//         |             |
+// +-------v----------+  |
+// |     Running      |  |
+// |(Target IO Thread)|  |
+// +------------------+  |
+//         | Resolve()   |
+// +-------v----------+  |
+// |     Resolving    <--+                   +-------------+
+// |                  |                      |  Completing |
 // |(Target IO Thread)+---------------------->(Orig Thread)|
 // +------------------+                      +-------+-----+
 //                                                   |
 //                                                   |
 //                                              +----v---+
 //                                              |Complete|
 //                                              +--------+
 //
@@ -478,18 +519,49 @@ NS_IMPL_ISUPPORTS_INHERITED(mozilla::dom
 NS_IMETHODIMP
 Context::ActionRunnable::Run()
 {
   switch(mState) {
     // ----------------------
     case STATE_RUN_ON_TARGET:
     {
       MOZ_ASSERT(NS_GetCurrentThread() == mTarget);
+      MOZ_ASSERT(!mExecutingRunOnTarget);
+
+      // Note that we are calling RunOnTarget().  This lets us detect
+      // if Resolve() is called synchronously.
+      AutoRestore<bool> executingRunOnTarget(mExecutingRunOnTarget);
+      mExecutingRunOnTarget = true;
+
       mState = STATE_RUNNING;
       mAction->RunOnTarget(this, mQuotaInfo);
+
+      // Resolve was called synchronously from RunOnTarget().  We can
+      // immediately move to completing now since we are sure RunOnTarget()
+      // completed.
+      if (mState == STATE_RESOLVING) {
+        // Use recursion instead of switch case fall-through...  Seems slightly
+        // easier to understand.
+        Run();
+      }
+
+      break;
+    }
+    // -----------------
+    case STATE_RESOLVING:
+    {
+      MOZ_ASSERT(NS_GetCurrentThread() == mTarget);
+      // The call to Action::RunOnTarget() must have returned now if we
+      // are running on the target thread again.  We may now proceed
+      // with completion.
+      mState = STATE_COMPLETING;
+      // Shutdown must be delayed until all Contexts are destroyed.  Crash
+      // for this invariant violation.
+      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(
+        mInitiatingThread->Dispatch(this, nsIThread::DISPATCH_NORMAL)));
       break;
     }
     // -------------------
     case STATE_COMPLETING:
     {
       NS_ASSERT_OWNINGTHREAD(Action::Resolver);
       mAction->CompleteOnInitiatingThread(mResult);
       mState = STATE_COMPLETE;
--- a/dom/cache/Context.h
+++ b/dom/cache/Context.h
@@ -102,16 +102,19 @@ public:
   // cancel any outstanding Activity work when the Context is cancelled.
   class Activity
   {
   public:
     virtual void Cancel() = 0;
     virtual bool MatchesCacheId(CacheId aCacheId) const = 0;
   };
 
+  // Create a Context attached to the given Manager.  The given Action
+  // will run on the QuotaManager IO thread.  Note, this Action must
+  // be execute synchronously.
   static already_AddRefed<Context>
   Create(Manager* aManager, Action* aQuotaIOThreadAction);
 
   // Execute given action on the target once the quota manager has been
   // initialized.
   //
   // Only callable from the thread that created the Context.
   void Dispatch(nsIEventTarget* aTarget, Action* aAction);