Bug 1150691 Fix Cache API race with storage invalidation. r=ehsan

dom/cache/Manager.cpp

@@ -1411,9 +1411,7 @@ Manager::RemoveListener(Listener* aListener)
   mListeners.RemoveElement(aListener, ListenerEntryListenerComparator());
   MOZ_ASSERT(!mListeners.Contains(aListener,
                                   ListenerEntryListenerComparator()));
-  if (mListeners.IsEmpty() && mContext) {
-    mContext->AllowToClose();
-  }
+  MaybeAllowContextToClose();
 }
 
 void
@@ -1422,6 +1420,12 @@ 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);
+
   mContext = nullptr;
 
   // If we're trying to shutdown, then note that we're done.  This is the
@@ -1483,6 +1487,7 @@ Manager::ReleaseCacheId(CacheId aCacheId)
           context->Dispatch(mIOThread, action);
         }
       }
+      MaybeAllowContextToClose();
       return;
     }
   }
@@ -1524,6 +1529,7 @@ Manager::ReleaseBodyId(const nsID& aBodyId)
           context->Dispatch(mIOThread, action);
         }
       }
+      MaybeAllowContextToClose();
       return;
     }
   }
@@ -1906,6 +1912,29 @@ Manager::NoteOrphanedBodyIdList(const nsTArray<nsID>& aDeletedBodyIdList)
   }
 }
 
+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()) {
+
+    // 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;
+
+    mContext->AllowToClose();
+  }
+}
+
 } // namespace cache
 } // namespace dom
 } // namespace mozilla

dom/cache/Manager.h

@@ -41,23 +41,25 @@ class StreamList;
 // Cache API.  This uniqueness is defined by the ManagerId equality operator.
 // The uniqueness is enforced by the Manager GetOrCreate() factory method.
 //
-// The Manager object can out live the IPC actors in the case where the child
-// process is killed; e.g a child process OOM.  The Manager object can
-// The Manager object can potentially use non-trivial resources.  Long lived
-// DOM objects and their actors should not maintain a reference to the Manager
-// while idle.  Transient DOM objects that may keep a reference for their
-// lifetimes.
+// The life cycle of Manager objects is somewhat complex.  While code may
+// hold a strong reference to the Manager, it will invalidate itself once it
+// believes it has become completely idle.  This is currently determined when
+// all of the following conditions occur:
 //
-// For example, once a CacheStorage DOM object is access it will live until its
-// global is released.  Therefore, CacheStorage should release its Manager
-// reference after operations complete and it becomes idle.  Cache objects,
-// however, can be GC'd once content are done using them and can therefore keep
-// their Manager reference alive.  Its expected that more operations are
-// performed on a Cache object, so keeping the Manager reference will help
-// minimize overhead for each reference.
+//  1) There are no more Manager::Listener objects registered with the Manager
+//     by performing a Cache or Storage operation.
+//  2) There are no more CacheId references noted via Manager::AddRefCacheId().
+//  3) There are no more BodyId references noted via Manager::AddRefBodyId().
+//
+// In order to keep your Manager alive you should perform an operation to set
+// a Listener, call AddRefCacheId(), or call AddRefBodyId().
+//
+// Even once a Manager becomes invalid, however, it may still continue to
+// exist.  This is allowed so that any in-progress Actions can gracefully
+// complete.
 //
 // As an invariant, all Manager objects must cease all IO before shutdown.  This
-// is enforced by the ShutdownObserver.  If content still holds references to
+// is enforced by the Manager::Factory.  If content still holds references to
 // Cache DOM objects during shutdown, then all operations will begin rejecting.
 class Manager final
 {
@@ -209,6 +211,8 @@ private:
   bool SetBodyIdOrphanedIfRefed(const nsID& aBodyId);
   void NoteOrphanedBodyIdList(const nsTArray<nsID>& aDeletedBodyIdList);
 
+  void MaybeAllowContextToClose();
+
   nsRefPtr<ManagerId> mManagerId;
   nsCOMPtr<nsIThread> mIOThread;