Bug 1151892 Refactor Cache Manager Context usage to be more sane and fix shutdown assert. r=ehsan

dom/cache/Context.cpp

@@ -732,6 +732,7 @@ Context::Dispatch(nsIEventTarget* aTarget, Action* aAction)
   MOZ_ASSERT(aTarget);
   MOZ_ASSERT(aAction);
 
+  MOZ_ASSERT(mState != STATE_CONTEXT_CANCELED);
   if (mState == STATE_CONTEXT_CANCELED) {
     return;
   } else if (mState == STATE_CONTEXT_INIT) {
@@ -766,11 +767,18 @@ Context::CancelAll()
   AllowToClose();
 }
 
+bool
+Context::IsCanceled() const
+{
+  NS_ASSERT_OWNINGTHREAD(Context);
+  return mState == STATE_CONTEXT_CANCELED;
+}
+
 void
 Context::Invalidate()
 {
   NS_ASSERT_OWNINGTHREAD(Context);
-  mManager->Invalidate();
+  mManager->NoteClosing();
   CancelAll();
 }
 

dom/cache/Context.h

@@ -126,6 +126,9 @@ public:
   // Only callable from the thread that created the Context.
   void CancelAll();
 
+  // True if CancelAll() has been called.
+  bool IsCanceled() const;
+
   // Like CancelAll(), but also marks the Manager as "invalid".
   void Invalidate();
 

dom/cache/Manager.cpp

@@ -157,6 +157,7 @@ public:
       if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
 
       ref = new Manager(aManagerId, ioThread);
+      ref->Init();
 
       MOZ_ASSERT(!sFactory->mManagerList.Contains(ref));
       sFactory->mManagerList.AppendElement(ref);
@@ -181,7 +182,7 @@ public:
       // If there is an invalid Manager finishing up and a new Manager
       // is created for the same origin, then the new Manager will
       // be blocked until QuotaManager finishes clearing the origin.
-      if (manager->IsValid() && *manager->mManagerId == *aManagerId) {
+      if (!manager->IsClosing() && *manager->mManagerId == *aManagerId) {
         return manager.forget();
       }
     }
@@ -1292,11 +1293,15 @@ public:
       if (!mManager->SetCacheIdOrphanedIfRefed(mCacheId)) {
 
         // no outstanding references, delete immediately
-        nsRefPtr<Context> context = mManager->CurrentContext();
-        context->CancelForCacheId(mCacheId);
-        nsRefPtr<Action> action =
-          new DeleteOrphanedCacheAction(mManager, mCacheId);
-        context->Dispatch(mManager->mIOThread, action);
+        nsRefPtr<Context> context = mManager->mContext;
+
+        // TODO: note that we need to check this cache for staleness on startup (bug 1110446)
+        if (!context->IsCanceled()) {
+          context->CancelForCacheId(mCacheId);
+          nsRefPtr<Action> action =
+            new DeleteOrphanedCacheAction(mManager, mCacheId);
+          context->Dispatch(mManager->mIOThread, action);
+        }
       }
     }
 
@@ -1443,32 +1448,31 @@ Manager::RemoveContext(Context* aContext)
   MOZ_ASSERT(mContext == aContext);
 
   // Whether the Context destruction was triggered from the Manager going
-  // idle or the underlying storage being invalidated, we should be invalid
-  // when the context is destroyed.
-  MOZ_ASSERT(!mValid);
+  // idle or the underlying storage being invalidated, we should know we
+  // are closing before the Conext is destroyed.
+  MOZ_ASSERT(mClosing);
 
   mContext = nullptr;
 
-  // If we're trying to shutdown, then note that we're done.  This is the
-  // delayed case from Manager::Shutdown().
-  if (mShuttingDown) {
-    Factory::Remove(this);
-  }
+  // Once the context is gone, we can immediately remove ourself from the
+  // Factory list.  We don't need to block shutdown by staying in the list
+  // any more.
+  Factory::Remove(this);
 }
 
 void
-Manager::Invalidate()
+Manager::NoteClosing()
 {
   NS_ASSERT_OWNINGTHREAD(Manager);
-  // QuotaManager can trigger this more than once.
-  mValid = false;
+  // This can be called more than once legitimately through different paths.
+  mClosing = true;
 }
 
 bool
-Manager::IsValid() const
+Manager::IsClosing() const
 {
   NS_ASSERT_OWNINGTHREAD(Manager);
-  return mValid;
+  return mClosing;
 }
 
 void
@@ -1500,8 +1504,8 @@ Manager::ReleaseCacheId(CacheId aCacheId)
         bool orphaned = mCacheIdRefs[i].mOrphaned;
         mCacheIdRefs.RemoveElementAt(i);
         // TODO: note that we need to check this cache for staleness on startup (bug 1110446)
-        if (orphaned && !mShuttingDown && mValid) {
-          nsRefPtr<Context> context = CurrentContext();
+        nsRefPtr<Context> context = mContext;
+        if (orphaned && context && !context->IsCanceled()) {
           context->CancelForCacheId(aCacheId);
           nsRefPtr<Action> action = new DeleteOrphanedCacheAction(this,
                                                                   aCacheId);
@@ -1544,9 +1548,9 @@ Manager::ReleaseBodyId(const nsID& aBodyId)
         bool orphaned = mBodyIdRefs[i].mOrphaned;
         mBodyIdRefs.RemoveElementAt(i);
         // TODO: note that we need to check this body for staleness on startup (bug 1110446)
-        if (orphaned && !mShuttingDown && mValid) {
+        nsRefPtr<Context> context = mContext;
+        if (orphaned && context && !context->IsCanceled()) {
           nsRefPtr<Action> action = new DeleteOrphanedBodyAction(aBodyId);
-          nsRefPtr<Context> context = CurrentContext();
           context->Dispatch(mIOThread, action);
         }
       }
@@ -1589,12 +1593,14 @@ Manager::ExecuteCacheOp(Listener* aListener, CacheId aCacheId,
   MOZ_ASSERT(aOpArgs.type() != CacheOpArgs::TCacheAddAllArgs);
   MOZ_ASSERT(aOpArgs.type() != CacheOpArgs::TCachePutAllArgs);
 
-  if (mShuttingDown || !mValid) {
+  if (mClosing) {
     aListener->OnOpComplete(ErrorResult(NS_ERROR_FAILURE), void_t());
     return;
   }
 
-  nsRefPtr<Context> context = CurrentContext();
+  nsRefPtr<Context> context = mContext;
+  MOZ_ASSERT(!context->IsCanceled());
+
   nsRefPtr<StreamList> streamList = new StreamList(this, context);
   ListenerId listenerId = SaveListener(aListener);
 
@@ -1631,12 +1637,14 @@ Manager::ExecuteStorageOp(Listener* aListener, Namespace aNamespace,
   NS_ASSERT_OWNINGTHREAD(Manager);
   MOZ_ASSERT(aListener);
 
-  if (mShuttingDown || !mValid) {
+  if (mClosing) {
     aListener->OnOpComplete(ErrorResult(NS_ERROR_FAILURE), void_t());
     return;
   }
 
-  nsRefPtr<Context> context = CurrentContext();
+  nsRefPtr<Context> context = mContext;
+  MOZ_ASSERT(!context->IsCanceled());
+
   nsRefPtr<StreamList> streamList = new StreamList(this, context);
   ListenerId listenerId = SaveListener(aListener);
 
@@ -1678,12 +1686,14 @@ Manager::ExecutePutAll(Listener* aListener, CacheId aCacheId,
   NS_ASSERT_OWNINGTHREAD(Manager);
   MOZ_ASSERT(aListener);
 
-  if (mShuttingDown || !mValid) {
+  if (mClosing) {
     aListener->OnOpComplete(ErrorResult(NS_ERROR_FAILURE), CachePutAllResult());
     return;
   }
 
-  nsRefPtr<Context> context = CurrentContext();
+  nsRefPtr<Context> context = mContext;
+  MOZ_ASSERT(!context->IsCanceled());
+
   ListenerId listenerId = SaveListener(aListener);
 
   nsRefPtr<Action> action = new CachePutAllAction(this, listenerId, aCacheId,
@@ -1698,7 +1708,7 @@ Manager::Manager(ManagerId* aManagerId, nsIThread* aIOThread)
   , mIOThread(aIOThread)
   , mContext(nullptr)
   , mShuttingDown(false)
-  , mValid(true)
+  , mClosing(false)
 {
   MOZ_ASSERT(mManagerId);
   MOZ_ASSERT(mIOThread);
@@ -1707,8 +1717,8 @@ Manager::Manager(ManagerId* aManagerId, nsIThread* aIOThread)
 Manager::~Manager()
 {
   NS_ASSERT_OWNINGTHREAD(Manager);
+  MOZ_ASSERT(mClosing);
   MOZ_ASSERT(!mContext);
-  Shutdown();
 
   nsCOMPtr<nsIThread> ioThread;
   mIOThread.swap(ioThread);
@@ -1720,6 +1730,19 @@ Manager::~Manager()
   MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(runnable)));
 }
 
+void
+Manager::Init()
+{
+  NS_ASSERT_OWNINGTHREAD(Manager);
+
+  // Create the context immediately.  Since there can at most be one Context
+  // per Manager now, this lets us cleanly call Factory::Remove() once the
+  // Context goes away.
+  nsRefPtr<Action> setupAction = new SetupAction();
+  nsRefPtr<Context> ref = Context::Create(this, setupAction);
+  mContext = ref;
+}
+
 void
 Manager::Shutdown()
 {
@@ -1732,36 +1755,20 @@ Manager::Shutdown()
     return;
   }
 
-  // Set a flag to prevent any new requests from coming in and creating
-  // a new Context.  We must ensure all Contexts and IO operations are
-  // complete before shutdown proceeds.
   mShuttingDown = true;
 
-  // If there is a context, then we must wait for it to complete.  Cancel and
-  // only note that we are done after its cleaned up.
+  // Note that we are closing to prevent any new requests from coming in and
+  // creating a new Context.  We must ensure all Contexts and IO operations are
+  // complete before shutdown proceeds.
+  NoteClosing();
+
+  // If there is a context, then cancel and only note that we are done after
+  // its cleaned up.
   if (mContext) {
     nsRefPtr<Context> context = mContext;
     context->CancelAll();
     return;
   }
-
-  // Otherwise, note that we are complete immediately
-  Factory::Remove(this);
-}
-
-already_AddRefed<Context>
-Manager::CurrentContext()
-{
-  NS_ASSERT_OWNINGTHREAD(Manager);
-  nsRefPtr<Context> ref = mContext;
-  if (!ref) {
-    MOZ_ASSERT(!mShuttingDown);
-    MOZ_ASSERT(mValid);
-    nsRefPtr<Action> setupAction = new SetupAction();
-    ref = Context::Create(this, setupAction);
-    mContext = ref;
-  }
-  return ref.forget();
 }
 
 Manager::ListenerId
@@ -1846,9 +1853,10 @@ Manager::NoteOrphanedBodyIdList(const nsTArray<nsID>& aDeletedBodyIdList)
     }
   }
 
-  if (!deleteNowList.IsEmpty()) {
+  // TODO: note that we need to check these bodies for staleness on startup (bug 1110446)
+  nsRefPtr<Context> context = mContext;
+  if (!deleteNowList.IsEmpty() && context && !context->IsCanceled()) {
     nsRefPtr<Action> action = new DeleteOrphanedBodyAction(deleteNowList);
-    nsRefPtr<Context> context = CurrentContext();
     context->Dispatch(mIOThread, action);
   }
 }
@@ -1863,16 +1871,17 @@ Manager::MaybeAllowContextToClose()
   // Cache state information to complete before doing this.  Once we allow
   // the Context to close we may not reliably get notified of storage
   // invalidation.
-  if (mContext && mListeners.IsEmpty()
-               && mCacheIdRefs.IsEmpty()
-               && mBodyIdRefs.IsEmpty()) {
+  nsRefPtr<Context> context = mContext;
+  if (context && mListeners.IsEmpty()
+              && mCacheIdRefs.IsEmpty()
+              && mBodyIdRefs.IsEmpty()) {
 
     // Mark this Manager as invalid so that it won't get used again.  We don't
     // want to start any new operations once we allow the Context to close since
     // it may race with the underlying storage getting invalidated.
-    mValid = false;
+    NoteClosing();
 
-    mContext->AllowToClose();
+    context->AllowToClose();
   }
 }
 

dom/cache/Manager.h

@@ -133,8 +133,8 @@ public:
 
   // Marks the Manager "invalid".  Once the Context completes no new operations
   // will be permitted with this Manager.  New actors will get a new Manager.
-  void Invalidate();
-  bool IsValid() const;
+  void NoteClosing();
+  bool IsClosing() const;
 
   // If an actor represents a long term reference to a cache or body stream,
   // then they must call AddRefCacheId() or AddRefBodyId().  This will
@@ -185,6 +185,7 @@ private:
 
   Manager(ManagerId* aManagerId, nsIThread* aIOThread);
   ~Manager();
+  void Init();
   void Shutdown();
   already_AddRefed<Context> CurrentContext();
 
@@ -248,7 +249,7 @@ private:
   nsTArray<StreamList*> mStreamLists;
 
   bool mShuttingDown;
-  bool mValid;
+  bool mClosing;
 
   struct CacheIdRefCounter
   {