Bug 1110485 P4 Keep Cache Actors alive during async operations. r=baku

dom/cache/Cache.cpp

@@ -14,7 +14,6 @@
 #include "mozilla/dom/CacheBinding.h"
 #include "mozilla/dom/cache/AutoUtils.h"
 #include "mozilla/dom/cache/CacheChild.h"
-#include "mozilla/dom/cache/CacheOpChild.h"
 #include "mozilla/dom/cache/CachePushStreamChild.h"
 #include "mozilla/dom/cache/ReadStream.h"
 #include "mozilla/ErrorResult.h"
@@ -350,10 +349,7 @@ Cache::CreatePushStream(nsIAsyncInputStream* aStream)
   NS_ASSERT_OWNINGTHREAD(Cache);
   MOZ_ASSERT(mActor);
   MOZ_ASSERT(aStream);
-  auto actor = mActor->SendPCachePushStreamConstructor(
+  return mActor->CreatePushStream(aStream);
-    new CachePushStreamChild(mActor->GetFeature(), aStream));
-  MOZ_ASSERT(actor);
-  return static_cast<CachePushStreamChild*>(actor);
 }
 
 Cache::~Cache()
@@ -375,10 +371,7 @@ Cache::ExecuteOp(AutoChildOpArgs& aOpArgs, ErrorResult& aRv)
     return nullptr;
   }
 
-  unused << mActor->SendPCacheOpConstructor(
+  mActor->ExecuteOp(mGlobal, promise, aOpArgs.SendAsOpArgs());
-    new CacheOpChild(mActor->GetFeature(), mGlobal, this, promise),
-    aOpArgs.SendAsOpArgs());
-
   return promise.forget();
 }
 

dom/cache/CacheChild.cpp

@@ -9,8 +9,8 @@
 #include "mozilla/unused.h"
 #include "mozilla/dom/cache/ActorUtils.h"
 #include "mozilla/dom/cache/Cache.h"
-#include "mozilla/dom/cache/PCacheOpChild.h"
+#include "mozilla/dom/cache/CacheOpChild.h"
-#include "mozilla/dom/cache/PCachePushStreamChild.h"
+#include "mozilla/dom/cache/CachePushStreamChild.h"
 #include "mozilla/dom/cache/StreamUtils.h"
 
 namespace mozilla {
@@ -33,6 +33,7 @@ DeallocPCacheChild(PCacheChild* aActor)
 
 CacheChild::CacheChild()
   : mListener(nullptr)
+  , mNumChildActors(0)
 {
   MOZ_COUNT_CTOR(cache::CacheChild);
 }
@@ -42,6 +43,7 @@ CacheChild::~CacheChild()
   MOZ_COUNT_DTOR(cache::CacheChild);
   NS_ASSERT_OWNINGTHREAD(CacheChild);
   MOZ_ASSERT(!mListener);
+  MOZ_ASSERT(!mNumChildActors);
 }
 
 void
@@ -61,6 +63,25 @@ CacheChild::ClearListener()
   mListener = nullptr;
 }
 
+void
+CacheChild::ExecuteOp(nsIGlobalObject* aGlobal, Promise* aPromise,
+                      const CacheOpArgs& aArgs)
+{
+  mNumChildActors += 1;
+  MOZ_ALWAYS_TRUE(SendPCacheOpConstructor(
+    new CacheOpChild(GetFeature(), aGlobal, aPromise), aArgs));
+}
+
+CachePushStreamChild*
+CacheChild::CreatePushStream(nsIAsyncInputStream* aStream)
+{
+  mNumChildActors += 1;
+  auto actor = SendPCachePushStreamConstructor(
+    new CachePushStreamChild(GetFeature(), aStream));
+  MOZ_ASSERT(actor);
+  return static_cast<CachePushStreamChild*>(actor);
+}
+
 void
 CacheChild::StartDestroy()
 {
@@ -73,13 +94,19 @@ CacheChild::StartDestroy()
     return;
   }
 
-  // TODO: only destroy if there are no ops or push streams still running
-
   listener->DestroyInternal(this);
 
   // Cache listener should call ClearListener() in DestroyInternal()
   MOZ_ASSERT(!mListener);
 
+  // If we have outstanding child actors, then don't destroy ourself yet.
+  // The child actors should be short lived and we should allow them to complete
+  // if possible.  SendTeardown() will be called when the count drops to zero
+  // in NoteDeletedActor().
+  if (mNumChildActors) {
+    return;
+  }
+
   // Start actor destruction from parent process
   unused << SendTeardown();
 }
@@ -109,6 +136,7 @@ bool
 CacheChild::DeallocPCacheOpChild(PCacheOpChild* aActor)
 {
   delete aActor;
+  NoteDeletedActor();
   return true;
 }
 
@@ -123,9 +151,19 @@ bool
 CacheChild::DeallocPCachePushStreamChild(PCachePushStreamChild* aActor)
 {
   delete aActor;
+  NoteDeletedActor();
   return true;
 }
 
+void
+CacheChild::NoteDeletedActor()
+{
+  mNumChildActors -= 1;
+  if (!mNumChildActors && !mListener) {
+    unused << SendTeardown();
+  }
+}
+
 } // namespace cache
 } // namespace dom
 } // namesapce mozilla

dom/cache/CacheChild.h

@@ -10,11 +10,19 @@
 #include "mozilla/dom/cache/ActorChild.h"
 #include "mozilla/dom/cache/PCacheChild.h"
 
+class nsIAsyncInputStream;
+class nsIGlobalObject;
+
 namespace mozilla {
 namespace dom {
+
+class Promise;
+
 namespace cache {
 
 class Cache;
+class CacheOpArgs;
+class CachePushStreamChild;
 
 class CacheChild final : public PCacheChild
                        , public ActorChild
@@ -30,6 +38,13 @@ public:
   // trigger ActorDestroy() if it has not been called yet.
   void ClearListener();
 
+  void
+  ExecuteOp(nsIGlobalObject* aGlobal, Promise* aPromise,
+            const CacheOpArgs& aArgs);
+
+  CachePushStreamChild*
+  CreatePushStream(nsIAsyncInputStream* aStream);
+
   // ActorChild methods
 
   // Synchronously call ActorDestroy on our Cache listener and then start the
@@ -53,10 +68,15 @@ private:
   virtual bool
   DeallocPCachePushStreamChild(PCachePushStreamChild* aActor) override;
 
+  // utility methods
+  void
+  NoteDeletedActor();
+
   // Use a weak ref so actor does not hold DOM object alive past content use.
   // The Cache object must call ClearListener() to null this before its
   // destroyed.
   Cache* MOZ_NON_OWNING_REF mListener;
+  uint32_t mNumChildActors;
 
   NS_DECL_OWNINGTHREAD
 };

dom/cache/CacheOpChild.cpp

@@ -17,13 +17,11 @@ namespace dom {
 namespace cache {
 
 CacheOpChild::CacheOpChild(Feature* aFeature, nsIGlobalObject* aGlobal,
-                           nsISupports* aParent, Promise* aPromise)
+                           Promise* aPromise)
   : mGlobal(aGlobal)
-  , mParent(aParent)
   , mPromise(aPromise)
 {
   MOZ_ASSERT(mGlobal);
-  MOZ_ASSERT(mParent);
   MOZ_ASSERT(mPromise);
 
   MOZ_ASSERT_IF(!NS_IsMainThread(), aFeature);

dom/cache/CacheOpChild.h

@@ -25,12 +25,15 @@ class CacheOpChild final : public PCacheOpChild
                          , public ActorChild
                          , public TypeUtils
 {
-public:
+  friend class CacheChild;
-  CacheOpChild(Feature* aFeature, nsIGlobalObject* aGlobal,
+  friend class CacheStorageChild;
-               nsISupports* aParent, Promise* aPromise);
-  ~CacheOpChild();
 
 private:
+  // This class must be constructed by CacheChild or CacheStorageChild using
+  // their ExecuteOp() factory method.
+  CacheOpChild(Feature* aFeature, nsIGlobalObject* aGlobal, Promise* aPromise);
+  ~CacheOpChild();
+
   // PCacheOpChild methods
   virtual void
   ActorDestroy(ActorDestroyReason aReason) override;
@@ -65,7 +68,6 @@ private:
   HandleRequestList(const nsTArray<CacheRequest>& aRequestList);
 
   nsCOMPtr<nsIGlobalObject> mGlobal;
-  nsCOMPtr<nsISupports> mParent;
   nsRefPtr<Promise> mPromise;
 
   NS_DECL_OWNINGTHREAD

dom/cache/CachePushStreamChild.h

@@ -20,17 +20,20 @@ namespace cache {
 class CachePushStreamChild final : public PCachePushStreamChild
                                  , public ActorChild
 {
+  friend class CacheChild;
+
 public:
-  CachePushStreamChild(Feature* aFeature, nsIAsyncInputStream* aStream);
+  void Start();
-  ~CachePushStreamChild();
 
   virtual void StartDestroy() override;
 
-  void Start();
-
 private:
   class Callback;
 
+  // This class must be constructed using CacheChild::CreatePushStream()
+  CachePushStreamChild(Feature* aFeature, nsIAsyncInputStream* aStream);
+  ~CachePushStreamChild();
+
   // PCachePushStreamChild methods
   virtual void
   ActorDestroy(ActorDestroyReason aReason) override;

dom/cache/CacheStorage.cpp

@@ -13,7 +13,6 @@
 #include "mozilla/dom/cache/AutoUtils.h"
 #include "mozilla/dom/cache/Cache.h"
 #include "mozilla/dom/cache/CacheChild.h"
-#include "mozilla/dom/cache/CacheOpChild.h"
 #include "mozilla/dom/cache/CacheStorageChild.h"
 #include "mozilla/dom/cache/Feature.h"
 #include "mozilla/dom/cache/PCacheChild.h"
@@ -440,9 +439,7 @@ CacheStorage::MaybeRunPendingRequests()
       entry->mPromise->MaybeReject(rv);
       continue;
     }
-    unused << mActor->SendPCacheOpConstructor(
+    mActor->ExecuteOp(mGlobal, entry->mPromise, args.SendAsOpArgs());
-      new CacheOpChild(mActor->GetFeature(), mGlobal, this, entry->mPromise),
-      args.SendAsOpArgs());
   }
   mPendingRequests.Clear();
 }

dom/cache/CacheStorageChild.cpp

@@ -8,8 +8,8 @@
 
 #include "mozilla/unused.h"
 #include "mozilla/dom/cache/CacheChild.h"
+#include "mozilla/dom/cache/CacheOpChild.h"
 #include "mozilla/dom/cache/CacheStorage.h"
-#include "mozilla/dom/cache/PCacheOpChild.h"
 #include "mozilla/dom/cache/StreamUtils.h"
 
 namespace mozilla {
@@ -25,6 +25,7 @@ DeallocPCacheStorageChild(PCacheStorageChild* aActor)
 
 CacheStorageChild::CacheStorageChild(CacheStorage* aListener, Feature* aFeature)
   : mListener(aListener)
+  , mNumChildActors(0)
 {
   MOZ_COUNT_CTOR(cache::CacheStorageChild);
   MOZ_ASSERT(mListener);
@@ -47,6 +48,15 @@ CacheStorageChild::ClearListener()
   mListener = nullptr;
 }
 
+void
+CacheStorageChild::ExecuteOp(nsIGlobalObject* aGlobal, Promise* aPromise,
+                             const CacheOpArgs& aArgs)
+{
+  mNumChildActors += 1;
+  unused << SendPCacheOpConstructor(
+    new CacheOpChild(GetFeature(), aGlobal, aPromise), aArgs);
+}
+
 void
 CacheStorageChild::StartDestroy()
 {
@@ -61,13 +71,19 @@ CacheStorageChild::StartDestroy()
     return;
   }
 
-  // TODO: don't destroy if we have outstanding ops
-
   listener->DestroyInternal(this);
 
   // CacheStorage listener should call ClearListener() in DestroyInternal()
   MOZ_ASSERT(!mListener);
 
+  // If we have outstanding child actors, then don't destroy ourself yet.
+  // The child actors should be short lived and we should allow them to complete
+  // if possible.  SendTeardown() will be called when the count drops to zero
+  // in NoteDeletedActor().
+  if (mNumChildActors) {
+    return;
+  }
+
   // Start actor destruction from parent process
   unused << SendTeardown();
 }
@@ -97,9 +113,20 @@ bool
 CacheStorageChild::DeallocPCacheOpChild(PCacheOpChild* aActor)
 {
   delete aActor;
+  NoteDeletedActor();
   return true;
 }
 
+void
+CacheStorageChild::NoteDeletedActor()
+{
+  MOZ_ASSERT(mNumChildActors);
+  mNumChildActors -= 1;
+  if (!mNumChildActors && !mListener) {
+    unused << SendTeardown();
+  }
+}
+
 } // namespace cache
 } // namespace dom
 } // namespace mozilla

dom/cache/CacheStorageChild.h

@@ -11,10 +11,16 @@
 #include "mozilla/dom/cache/Types.h"
 #include "mozilla/dom/cache/PCacheStorageChild.h"
 
+class nsIGlobalObject;
+
 namespace mozilla {
 namespace dom {
+
+class Promise;
+
 namespace cache {
 
+class CacheOpArgs;
 class CacheStorage;
 class PCacheChild;
 class Feature;
@@ -32,6 +38,10 @@ public:
   // called yet.
   void ClearListener();
 
+  void
+  ExecuteOp(nsIGlobalObject* aGlobal, Promise* aPromise,
+            const CacheOpArgs& aArgs);
+
   // ActorChild methods
 
   // Synchronously call ActorDestroy on our CacheStorage listener and then start
@@ -48,10 +58,15 @@ private:
   virtual bool
   DeallocPCacheOpChild(PCacheOpChild* aActor) override;
 
+  // utility methods
+  void
+  NoteDeletedActor();
+
   // Use a weak ref so actor does not hold DOM object alive past content use.
   // The CacheStorage object must call ClearListener() to null this before its
   // destroyed.
   CacheStorage* MOZ_NON_OWNING_REF mListener;
+  uint32_t mNumChildActors;
 
   NS_DECL_OWNINGTHREAD
 };

dom/cache/CacheStreamControlChild.cpp

@@ -41,6 +41,7 @@ DeallocPCacheStreamControlChild(PCacheStreamControlChild* aActor)
 
 CacheStreamControlChild::CacheStreamControlChild()
   : mDestroyStarted(false)
+  , mDestroyDelayed(false)
 {
   MOZ_COUNT_CTOR(cache::CacheStreamControlChild);
 }
@@ -63,6 +64,21 @@ CacheStreamControlChild::StartDestroy()
   }
   mDestroyStarted = true;
 
+  // If any of the streams have started to be read, then wait for them to close
+  // naturally.
+  if (HasEverBeenRead()) {
+    // Note that we are delaying so that we can re-check for active streams
+    // in NoteClosedAfterForget().
+    mDestroyDelayed = true;
+    return;
+  }
+
+  // Otherwise, if the streams have not been touched then just pre-emptively
+  // close them now.  This handles the case where someone retrieves a Response
+  // from the Cache, but never accesses the body.  We should not keep the
+  // Worker alive until that Response is GC'd just because of its ignored
+  // body stream.
+
   // Begin shutting down all streams.  This is the same as if the parent had
   // asked us to shutdown.  So simulate the CloseAll IPC message.
   RecvCloseAll();
@@ -120,6 +136,15 @@ CacheStreamControlChild::NoteClosedAfterForget(const nsID& aId)
 {
   NS_ASSERT_OWNINGTHREAD(CacheStreamControlChild);
   unused << SendNoteClosed(aId);
+
+  // A stream has closed.  If we delayed StartDestry() due to this stream
+  // being read, then we should check to see if any of the remaining streams
+  // are active.  If none of our other streams have been read, then we can
+  // proceed with the shutdown now.
+  if (mDestroyDelayed && !HasEverBeenRead()) {
+    mDestroyDelayed = false;
+    RecvCloseAll();
+  }
 }
 
 #ifdef DEBUG

dom/cache/CacheStreamControlChild.h

@@ -56,6 +56,7 @@ private:
   virtual bool RecvCloseAll() override;
 
   bool mDestroyStarted;
+  bool mDestroyDelayed;
 
   NS_DECL_OWNINGTHREAD
 };

dom/cache/ReadStream.cpp

@@ -50,6 +50,9 @@ public:
   virtual bool
   MatchId(const nsID& aId) const override;
 
+  virtual bool
+  HasEverBeenRead() const override;
+
   // Simulate nsIInputStream methods, but we don't actually inherit from it
   NS_METHOD
   Close();
@@ -103,6 +106,7 @@ private:
     NumStates
   };
   Atomic<State> mState;
+  Atomic<bool> mHasEverBeenRead;
 
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(cache::ReadStream::Inner, override)
 };
@@ -249,6 +253,13 @@ ReadStream::Inner::MatchId(const nsID& aId) const
   return mId.Equals(aId);
 }
 
+bool
+ReadStream::Inner::HasEverBeenRead() const
+{
+  MOZ_ASSERT(NS_GetCurrentThread() == mOwningThread);
+  return mHasEverBeenRead;
+}
+
 NS_IMETHODIMP
 ReadStream::Inner::Close()
 {
@@ -284,6 +295,8 @@ ReadStream::Inner::Read(char* aBuf, uint32_t aCount, uint32_t* aNumReadOut)
     Close();
   }
 
+  mHasEverBeenRead = true;
+
   return rv;
 }
 
@@ -294,6 +307,10 @@ ReadStream::Inner::ReadSegments(nsWriteSegmentFun aWriter, void* aClosure,
   // stream ops can happen on any thread
   MOZ_ASSERT(aNumReadOut);
 
+  if (aCount) {
+    mHasEverBeenRead = true;
+  }
+
   nsresult rv = mSnappyStream->ReadSegments(aWriter, aClosure, aCount,
                                             aNumReadOut);
 
@@ -302,6 +319,14 @@ ReadStream::Inner::ReadSegments(nsWriteSegmentFun aWriter, void* aClosure,
     Close();
   }
 
+  // Verify bytes were actually read before marking as being ever read.  For
+  // example, code can test if the stream supports ReadSegments() by calling
+  // this method with a dummy callback which doesn't read anything.  We don't
+  // want to trigger on that.
+  if (*aNumReadOut) {
+    mHasEverBeenRead = true;
+  }
+
   return rv;
 }
 

dom/cache/ReadStream.h

@@ -63,6 +63,9 @@ public:
     virtual bool
     MatchId(const nsID& aId) const = 0;
 
+    virtual bool
+    HasEverBeenRead() const = 0;
+
     NS_IMETHOD_(MozExternalRefCountType)
     AddRef(void) = 0;
 

dom/cache/StreamControl.cpp

@@ -84,6 +84,18 @@ StreamControl::CloseAllReadStreamsWithoutReporting()
   }
 }
 
+bool
+StreamControl::HasEverBeenRead() const
+{
+  ReadStreamList::ForwardIterator iter(mReadStreamList);
+  while (iter.HasMore()) {
+    if (iter.GetNext()->HasEverBeenRead()) {
+      return true;
+    }
+  }
+  return false;
+}
+
 } // namespace cache
 } // namespace dom
 } // namespace mozilla

dom/cache/StreamControl.h

@@ -68,6 +68,9 @@ protected:
   void
   CloseAllReadStreamsWithoutReporting();
 
+  bool
+  HasEverBeenRead() const;
+
   // protected parts of the abstract interface
   virtual void
   NoteClosedAfterForget(const nsID& aId) = 0;