Bug 1151892 Refactor Cache Manager Context usage to be more sane and fix shutdown assert. r=ehsan
authorBen Kelly <ben@wanderview.com>
Thu, 16 Apr 2015 13:05:38 -0700
changeset 239667 dada69804f56b569f9d5552006c9f7828cd3c978
parent 239666 52e5ad1e8863170d704ca82ab23f5d103d716cc1
child 239668 103433e8eb3c2a034b45e614e1a036252d2bb52f
push id12444
push userryanvm@gmail.com
push dateFri, 17 Apr 2015 20:04:42 +0000
treeherderfx-team@560a202db924 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1151892
milestone40.0a1
Bug 1151892 Refactor Cache Manager Context usage to be more sane and fix shutdown assert. r=ehsan
dom/cache/Context.cpp
dom/cache/Context.h
dom/cache/Manager.cpp
dom/cache/Manager.h
--- a/dom/cache/Context.cpp
+++ b/dom/cache/Context.cpp
@@ -727,16 +727,17 @@ Context::Context(Manager* aManager)
 
 void
 Context::Dispatch(nsIEventTarget* aTarget, Action* aAction)
 {
   NS_ASSERT_OWNINGTHREAD(Context);
   MOZ_ASSERT(aTarget);
   MOZ_ASSERT(aAction);
 
+  MOZ_ASSERT(mState != STATE_CONTEXT_CANCELED);
   if (mState == STATE_CONTEXT_CANCELED) {
     return;
   } else if (mState == STATE_CONTEXT_INIT) {
     PendingAction* pending = mPendingActions.AppendElement();
     pending->mTarget = aTarget;
     pending->mAction = aAction;
     return;
   }
@@ -761,21 +762,28 @@ Context::CancelAll()
     ActivityList::ForwardIterator iter(mActivityList);
     while (iter.HasMore()) {
       iter.GetNext()->Cancel();
     }
   }
   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();
 }
 
 void
 Context::AllowToClose()
 {
   NS_ASSERT_OWNINGTHREAD(Context);
   if (mThreadsafeHandle) {
--- a/dom/cache/Context.h
+++ b/dom/cache/Context.h
@@ -121,16 +121,19 @@ public:
 
   // Cancel any Actions running or waiting to run.  This should allow the
   // Context to be released and Listener::RemoveContext() will be called
   // when complete.
   //
   // 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();
 
   // Remove any self references and allow the Context to be released when
   // there are no more Actions to process.
   void AllowToClose();
 
   // Cancel any Actions running or waiting to run that operate on the given
--- a/dom/cache/Manager.cpp
+++ b/dom/cache/Manager.cpp
@@ -152,16 +152,17 @@ public:
     nsRefPtr<Manager> ref = Get(aManagerId);
     if (!ref) {
       // TODO: replace this with a thread pool (bug 1119864)
       nsCOMPtr<nsIThread> ioThread;
       rv = NS_NewNamedThread("DOMCacheThread", getter_AddRefs(ioThread));
       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);
     }
 
     ref.forget(aManagerOut);
 
     return NS_OK;
@@ -176,17 +177,17 @@ public:
     if (NS_WARN_IF(NS_FAILED(rv))) { return nullptr; }
 
     ManagerList::ForwardIterator iter(sFactory->mManagerList);
     while (iter.HasMore()) {
       nsRefPtr<Manager> manager = iter.GetNext();
       // 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();
       }
     }
 
     return nullptr;
   }
 
   static void
@@ -1287,21 +1288,25 @@ public:
   Complete(Listener* aListener, ErrorResult&& aRv) override
   {
     if (mCacheDeleted) {
       // If content is referencing this cache, mark it orphaned to be
       // deleted later.
       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);
+        }
       }
     }
 
     aListener->OnOpComplete(Move(aRv), StorageDeleteResult(mCacheDeleted));
   }
 
 private:
   const Namespace mNamespace;
@@ -1438,42 +1443,41 @@ Manager::RemoveListener(Listener* aListe
 void
 Manager::RemoveContext(Context* aContext)
 {
   NS_ASSERT_OWNINGTHREAD(Manager);
   MOZ_ASSERT(mContext);
   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
 Manager::AddRefCacheId(CacheId aCacheId)
 {
   NS_ASSERT_OWNINGTHREAD(Manager);
   for (uint32_t i = 0; i < mCacheIdRefs.Length(); ++i) {
     if (mCacheIdRefs[i].mCacheId == aCacheId) {
@@ -1495,18 +1499,18 @@ Manager::ReleaseCacheId(CacheId aCacheId
     if (mCacheIdRefs[i].mCacheId == aCacheId) {
       DebugOnly<uint32_t> oldRef = mCacheIdRefs[i].mCount;
       mCacheIdRefs[i].mCount -= 1;
       MOZ_ASSERT(mCacheIdRefs[i].mCount < oldRef);
       if (mCacheIdRefs[i].mCount == 0) {
         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);
           context->Dispatch(mIOThread, action);
         }
       }
       MaybeAllowContextToClose();
       return;
@@ -1539,19 +1543,19 @@ Manager::ReleaseBodyId(const nsID& aBody
     if (mBodyIdRefs[i].mBodyId == aBodyId) {
       DebugOnly<uint32_t> oldRef = mBodyIdRefs[i].mCount;
       mBodyIdRefs[i].mCount -= 1;
       MOZ_ASSERT(mBodyIdRefs[i].mCount < oldRef);
       if (mBodyIdRefs[i].mCount < 1) {
         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);
         }
       }
       MaybeAllowContextToClose();
       return;
     }
   }
   MOZ_ASSERT_UNREACHABLE("Attempt to release BodyId that is not referenced!");
@@ -1584,22 +1588,24 @@ void
 Manager::ExecuteCacheOp(Listener* aListener, CacheId aCacheId,
                         const CacheOpArgs& aOpArgs)
 {
   NS_ASSERT_OWNINGTHREAD(Manager);
   MOZ_ASSERT(aListener);
   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);
 
   nsRefPtr<Action> action;
   switch(aOpArgs.type()) {
     case CacheOpArgs::TCacheMatchArgs:
       action = new CacheMatchAction(this, listenerId, aCacheId,
                                     aOpArgs.get_CacheMatchArgs(), streamList);
@@ -1626,22 +1632,24 @@ Manager::ExecuteCacheOp(Listener* aListe
 
 void
 Manager::ExecuteStorageOp(Listener* aListener, Namespace aNamespace,
                           const CacheOpArgs& aOpArgs)
 {
   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);
 
   nsRefPtr<Action> action;
   switch(aOpArgs.type()) {
     case CacheOpArgs::TStorageMatchArgs:
       action = new StorageMatchAction(this, listenerId, aNamespace,
                                       aOpArgs.get_StorageMatchArgs(),
@@ -1673,100 +1681,99 @@ void
 Manager::ExecutePutAll(Listener* aListener, CacheId aCacheId,
                        const nsTArray<CacheRequestResponse>& aPutList,
                        const nsTArray<nsCOMPtr<nsIInputStream>>& aRequestStreamList,
                        const nsTArray<nsCOMPtr<nsIInputStream>>& aResponseStreamList)
 {
   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,
                                                   aPutList, aRequestStreamList,
                                                   aResponseStreamList);
 
   context->Dispatch(mIOThread, action);
 }
 
 Manager::Manager(ManagerId* aManagerId, nsIThread* aIOThread)
   : mManagerId(aManagerId)
   , mIOThread(aIOThread)
   , mContext(nullptr)
   , mShuttingDown(false)
-  , mValid(true)
+  , mClosing(false)
 {
   MOZ_ASSERT(mManagerId);
   MOZ_ASSERT(mIOThread);
 }
 
 Manager::~Manager()
 {
   NS_ASSERT_OWNINGTHREAD(Manager);
+  MOZ_ASSERT(mClosing);
   MOZ_ASSERT(!mContext);
-  Shutdown();
 
   nsCOMPtr<nsIThread> ioThread;
   mIOThread.swap(ioThread);
 
   // Don't spin the event loop in the destructor waiting for the thread to
   // shutdown.  Defer this to the main thread, instead.
   nsCOMPtr<nsIRunnable> runnable =
     NS_NewRunnableMethod(ioThread, &nsIThread::Shutdown);
   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()
 {
   NS_ASSERT_OWNINGTHREAD(Manager);
 
   // Ignore duplicate attempts to shutdown.  This can occur when we start
   // a browser initiated shutdown and then run ~Manager() which also
   // calls Shutdown().
   if (mShuttingDown) {
     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
 Manager::SaveListener(Listener* aListener)
 {
   NS_ASSERT_OWNINGTHREAD(Manager);
 
   // Once a Listener is added, we keep a reference to it until its
@@ -1841,41 +1848,43 @@ Manager::NoteOrphanedBodyIdList(const ns
   deleteNowList.SetCapacity(aDeletedBodyIdList.Length());
 
   for (uint32_t i = 0; i < aDeletedBodyIdList.Length(); ++i) {
     if (!SetBodyIdOrphanedIfRefed(aDeletedBodyIdList[i])) {
       deleteNowList.AppendElement(aDeletedBodyIdList[i]);
     }
   }
 
-  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);
   }
 }
 
 void
 Manager::MaybeAllowContextToClose()
 {
   NS_ASSERT_OWNINGTHREAD(Manager);
 
   // If we have an active context, but we have no more users of the Manager,
   // then let it shut itself down.  We must wait for all possible users of
   // 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();
   }
 }
 
 } // namespace cache
 } // namespace dom
 } // namespace mozilla
--- a/dom/cache/Manager.h
+++ b/dom/cache/Manager.h
@@ -128,18 +128,18 @@ public:
   // Must be called by Listener objects before they are destroyed.
   void RemoveListener(Listener* aListener);
 
   // Must be called by Context objects before they are destroyed.
   void RemoveContext(Context* aContext);
 
   // 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
   // cause the Manager to keep the backing data store alive for the given
   // object.  The actor must then call ReleaseCacheId() or ReleaseBodyId()
   // exactly once for every AddRef*() call it made.  Any delayed deletion
   // will then be performed.
   void AddRefCacheId(CacheId aCacheId);
@@ -180,16 +180,17 @@ private:
   class StorageOpenAction;
   class StorageDeleteAction;
   class StorageKeysAction;
 
   typedef uint64_t ListenerId;
 
   Manager(ManagerId* aManagerId, nsIThread* aIOThread);
   ~Manager();
+  void Init();
   void Shutdown();
   already_AddRefed<Context> CurrentContext();
 
   ListenerId SaveListener(Listener* aListener);
   Listener* GetListener(ListenerId aListenerId) const;
 
   bool SetCacheIdOrphanedIfRefed(CacheId aCacheId);
   bool SetBodyIdOrphanedIfRefed(const nsID& aBodyId);
@@ -243,17 +244,17 @@ private:
   typedef nsTArray<ListenerEntry> ListenerList;
   ListenerList mListeners;
   static ListenerId sNextListenerId;
 
   // Weak references cleared by RemoveStreamList() in StreamList destructors.
   nsTArray<StreamList*> mStreamLists;
 
   bool mShuttingDown;
-  bool mValid;
+  bool mClosing;
 
   struct CacheIdRefCounter
   {
     CacheId mCacheId;
     MozRefCountType mCount;
     bool mOrphaned;
   };
   nsTArray<CacheIdRefCounter> mCacheIdRefs;