Bug 1160147 Improve Cache API WorkerFeature shutdown handling. r=baku

dom/cache/Cache.cpp

@@ -229,7 +229,10 @@ already_AddRefed<Promise>
 Cache::Match(const RequestOrUSVString& aRequest,
              const CacheQueryOptions& aOptions, ErrorResult& aRv)
 {
-  MOZ_ASSERT(mActor);
+  if (!mActor) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return nullptr;
+  }
 
   nsRefPtr<InternalRequest> ir = ToInternalRequest(aRequest, IgnoreBody, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
@@ -253,7 +256,10 @@ already_AddRefed<Promise>
 Cache::MatchAll(const Optional<RequestOrUSVString>& aRequest,
                 const CacheQueryOptions& aOptions, ErrorResult& aRv)
 {
-  MOZ_ASSERT(mActor);
+  if (!mActor) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return nullptr;
+  }
 
   CacheQueryParams params;
   ToCacheQueryParams(params, aOptions);
@@ -280,6 +286,11 @@ already_AddRefed<Promise>
 Cache::Add(JSContext* aContext, const RequestOrUSVString& aRequest,
            ErrorResult& aRv)
 {
+  if (!mActor) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return nullptr;
+  }
+
   if (!IsValidPutRequestMethod(aRequest, aRv)) {
     return nullptr;
   }
@@ -309,6 +320,11 @@ Cache::AddAll(JSContext* aContext,
               const Sequence<OwningRequestOrUSVString>& aRequestList,
               ErrorResult& aRv)
 {
+  if (!mActor) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return nullptr;
+  }
+
   GlobalObject global(aContext, mGlobal->GetGlobalJSObject());
   MOZ_ASSERT(!global.Failed());
 
@@ -349,7 +365,10 @@ already_AddRefed<Promise>
 Cache::Put(const RequestOrUSVString& aRequest, Response& aResponse,
            ErrorResult& aRv)
 {
-  MOZ_ASSERT(mActor);
+  if (!mActor) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return nullptr;
+  }
 
   if (!IsValidPutRequestMethod(aRequest, aRv)) {
     return nullptr;
@@ -375,7 +394,10 @@ already_AddRefed<Promise>
 Cache::Delete(const RequestOrUSVString& aRequest,
               const CacheQueryOptions& aOptions, ErrorResult& aRv)
 {
-  MOZ_ASSERT(mActor);
+  if (!mActor) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return nullptr;
+  }
 
   nsRefPtr<InternalRequest> ir = ToInternalRequest(aRequest, IgnoreBody, aRv);
   if (aRv.Failed()) {
@@ -399,7 +421,10 @@ already_AddRefed<Promise>
 Cache::Keys(const Optional<RequestOrUSVString>& aRequest,
             const CacheQueryOptions& aOptions, ErrorResult& aRv)
 {
-  MOZ_ASSERT(mActor);
+  if (!mActor) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return nullptr;
+  }
 
   CacheQueryParams params;
   ToCacheQueryParams(params, aOptions);
@@ -493,9 +518,9 @@ Cache::~Cache()
 {
   NS_ASSERT_OWNINGTHREAD(Cache);
   if (mActor) {
-    mActor->StartDestroy();
+    mActor->StartDestroyFromListener();
-    // DestroyInternal() is called synchronously by StartDestroy().  So we
+    // DestroyInternal() is called synchronously by StartDestroyFromListener().
-    // should have already cleared the mActor.
+    // So we should have already cleared the mActor.
     MOZ_ASSERT(!mActor);
   }
 }
@@ -508,7 +533,7 @@ Cache::ExecuteOp(AutoChildOpArgs& aOpArgs, ErrorResult& aRv)
     return nullptr;
   }
 
-  mActor->ExecuteOp(mGlobal, promise, aOpArgs.SendAsOpArgs());
+  mActor->ExecuteOp(mGlobal, promise, this, aOpArgs.SendAsOpArgs());
   return promise.forget();
 }
 
@@ -570,9 +595,13 @@ Cache::PutAll(const nsTArray<nsRefPtr<Request>>& aRequestList,
               const nsTArray<nsRefPtr<Response>>& aResponseList,
               ErrorResult& aRv)
 {
-  MOZ_ASSERT(mActor);
   MOZ_ASSERT(aRequestList.Length() == aResponseList.Length());
 
+  if (!mActor) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return nullptr;
+  }
+
   AutoChildOpArgs args(this, CachePutAllArgs());
 
   for (uint32_t i = 0; i < aRequestList.Length(); ++i) {

dom/cache/CacheChild.cpp

@@ -33,6 +33,7 @@ DeallocPCacheChild(PCacheChild* aActor)
 CacheChild::CacheChild()
   : mListener(nullptr)
   , mNumChildActors(0)
+  , mDelayedDestroy(false)
 {
   MOZ_COUNT_CTOR(cache::CacheChild);
 }
@@ -64,11 +65,11 @@ CacheChild::ClearListener()
 
 void
 CacheChild::ExecuteOp(nsIGlobalObject* aGlobal, Promise* aPromise,
-                      const CacheOpArgs& aArgs)
+                      nsISupports* aParent, const CacheOpArgs& aArgs)
 {
   mNumChildActors += 1;
   MOZ_ALWAYS_TRUE(SendPCacheOpConstructor(
-    new CacheOpChild(GetFeature(), aGlobal, aPromise), aArgs));
+    new CacheOpChild(GetFeature(), aGlobal, aParent, aPromise), aArgs));
 }
 
 CachePushStreamChild*
@@ -81,9 +82,33 @@ CacheChild::CreatePushStream(nsIAsyncInputStream* aStream)
   return static_cast<CachePushStreamChild*>(actor);
 }
 
+void
+CacheChild::StartDestroyFromListener()
+{
+  NS_ASSERT_OWNINGTHREAD(CacheChild);
+
+  // The listener should be held alive by any async operations, so if it
+  // is going away then there must not be any child actors.  This in turn
+  // ensures that StartDestroy() will not trigger the delayed path.
+  MOZ_ASSERT(!mNumChildActors);
+
+  StartDestroy();
+}
+
 void
 CacheChild::StartDestroy()
 {
+  NS_ASSERT_OWNINGTHREAD(CacheChild);
+
+  // 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.  NoteDeletedActor() will call back into this Shutdown()
+  // method when the last child actor is gone.
+  if (mNumChildActors) {
+    mDelayedDestroy = true;
+    return;
+  }
+
   nsRefPtr<Cache> listener = mListener;
 
   // StartDestroy() can get called from either Cache or the Feature.
@@ -98,14 +123,6 @@ CacheChild::StartDestroy()
   // 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();
 }
@@ -158,8 +175,8 @@ void
 CacheChild::NoteDeletedActor()
 {
   mNumChildActors -= 1;
-  if (!mNumChildActors && !mListener) {
+  if (!mNumChildActors && mDelayedDestroy) {
-    unused << SendTeardown();
+    StartDestroy();
   }
 }
 

dom/cache/CacheChild.h

@@ -33,25 +33,27 @@ public:
 
   void SetListener(Cache* aListener);
 
-  // Must be called by the associated Cache listener in its ActorDestroy()
+  // Must be called by the associated Cache listener in its DestroyInternal()
-  // method.  Also, Cache must Send__delete__() the actor in its destructor to
+  // method.  Also, Cache must call StartDestroyFromListener() on the actor in
-  // trigger ActorDestroy() if it has not been called yet.
+  // its destructor to trigger ActorDestroy() if it has not been called yet.
   void ClearListener();
 
   void
   ExecuteOp(nsIGlobalObject* aGlobal, Promise* aPromise,
-            const CacheOpArgs& aArgs);
+            nsISupports* aParent, const CacheOpArgs& aArgs);
 
   CachePushStreamChild*
   CreatePushStream(nsIAsyncInputStream* aStream);
 
-  // ActorChild methods
+  // Our parent Listener object has gone out of scope and is being destroyed.
-
+  void StartDestroyFromListener();
-  // Synchronously call ActorDestroy on our Cache listener and then start the
-  // actor destruction asynchronously from the parent-side.
-  virtual void StartDestroy() override;
 
 private:
+  // ActorChild methods
+
+  // Feature is trying to destroy due to worker shutdown.
+  virtual void StartDestroy() override;
+
   // PCacheChild methods
   virtual void
   ActorDestroy(ActorDestroyReason aReason) override;
@@ -77,6 +79,7 @@ private:
   // destroyed.
   Cache* MOZ_NON_OWNING_REF mListener;
   uint32_t mNumChildActors;
+  bool mDelayedDestroy;
 
   NS_DECL_OWNINGTHREAD
 };

dom/cache/CacheOpChild.cpp

@@ -57,11 +57,13 @@ AddFeatureToStreamChild(const CacheRequest& aRequest, Feature* aFeature)
 } // anonymous namespace
 
 CacheOpChild::CacheOpChild(Feature* aFeature, nsIGlobalObject* aGlobal,
-                           Promise* aPromise)
+                           nsISupports* aParent, 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

@@ -31,7 +31,8 @@ class CacheOpChild final : public PCacheOpChild
 private:
   // This class must be constructed by CacheChild or CacheStorageChild using
   // their ExecuteOp() factory method.
-  CacheOpChild(Feature* aFeature, nsIGlobalObject* aGlobal, Promise* aPromise);
+  CacheOpChild(Feature* aFeature, nsIGlobalObject* aGlobal,
+               nsISupports* aParent, Promise* aPromise);
   ~CacheOpChild();
 
   // PCacheOpChild methods
@@ -68,6 +69,9 @@ private:
   HandleRequestList(const nsTArray<CacheRequest>& aRequestList);
 
   nsCOMPtr<nsIGlobalObject> mGlobal;
+  // Hold the parent Cache or CacheStorage object alive until this async
+  // operation completes.
+  nsCOMPtr<nsISupports> mParent;
   nsRefPtr<Promise> mPromise;
 
   NS_DECL_OWNINGTHREAD

dom/cache/CachePushStreamChild.cpp

@@ -118,8 +118,9 @@ CachePushStreamChild::Start()
 void
 CachePushStreamChild::StartDestroy()
 {
-  // called if we are running on a Worker and the thread gets shutdown
+  // The worker has signaled its shutting down, but continue streaming.  The
-  OnEnd(NS_ERROR_ABORT);
+  // Cache is now designed to hold the worker open until all async operations
+  // complete.
 }
 
 void

dom/cache/CacheStorage.cpp

@@ -413,9 +413,9 @@ CacheStorage::~CacheStorage()
 {
   NS_ASSERT_OWNINGTHREAD(CacheStorage);
   if (mActor) {
-    mActor->StartDestroy();
+    mActor->StartDestroyFromListener();
-    // DestroyInternal() is called synchronously by StartDestroy().  So we
+    // DestroyInternal() is called synchronously by StartDestroyFromListener().
-    // should have already cleared the mActor.
+    // So we should have already cleared the mActor.
     MOZ_ASSERT(!mActor);
   }
 }
@@ -438,7 +438,7 @@ CacheStorage::MaybeRunPendingRequests()
       entry->mPromise->MaybeReject(rv);
       continue;
     }
-    mActor->ExecuteOp(mGlobal, entry->mPromise, args.SendAsOpArgs());
+    mActor->ExecuteOp(mGlobal, entry->mPromise, this, args.SendAsOpArgs());
   }
   mPendingRequests.Clear();
 }

dom/cache/CacheStorageChild.cpp

@@ -25,6 +25,7 @@ DeallocPCacheStorageChild(PCacheStorageChild* aActor)
 CacheStorageChild::CacheStorageChild(CacheStorage* aListener, Feature* aFeature)
   : mListener(aListener)
   , mNumChildActors(0)
+  , mDelayedDestroy(false)
 {
   MOZ_COUNT_CTOR(cache::CacheStorageChild);
   MOZ_ASSERT(mListener);
@@ -49,11 +50,24 @@ CacheStorageChild::ClearListener()
 
 void
 CacheStorageChild::ExecuteOp(nsIGlobalObject* aGlobal, Promise* aPromise,
-                             const CacheOpArgs& aArgs)
+                             nsISupports* aParent, const CacheOpArgs& aArgs)
 {
   mNumChildActors += 1;
   unused << SendPCacheOpConstructor(
-    new CacheOpChild(GetFeature(), aGlobal, aPromise), aArgs);
+    new CacheOpChild(GetFeature(), aGlobal, aParent, aPromise), aArgs);
+}
+
+void
+CacheStorageChild::StartDestroyFromListener()
+{
+  NS_ASSERT_OWNINGTHREAD(CacheStorageChild);
+
+  // The listener should be held alive by any async operations, so if it
+  // is going away then there must not be any child actors.  This in turn
+  // ensures that StartDestroy() will not trigger the delayed path.
+  MOZ_ASSERT(!mNumChildActors);
+
+  StartDestroy();
 }
 
 void
@@ -61,6 +75,15 @@ CacheStorageChild::StartDestroy()
 {
   NS_ASSERT_OWNINGTHREAD(CacheStorageChild);
 
+  // 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.  NoteDeletedActor() will call back into this Shutdown()
+  // method when the last child actor is gone.
+  if (mNumChildActors) {
+    mDelayedDestroy = true;
+    return;
+  }
+
   nsRefPtr<CacheStorage> listener = mListener;
 
   // StartDestroy() can get called from either CacheStorage or the Feature.
@@ -75,14 +98,6 @@ CacheStorageChild::StartDestroy()
   // 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();
 }
@@ -121,8 +136,8 @@ CacheStorageChild::NoteDeletedActor()
 {
   MOZ_ASSERT(mNumChildActors);
   mNumChildActors -= 1;
-  if (!mNumChildActors && !mListener) {
+  if (!mNumChildActors && mDelayedDestroy) {
-    unused << SendTeardown();
+    StartDestroy();
   }
 }
 

dom/cache/CacheStorageChild.h

@@ -33,22 +33,24 @@ public:
   ~CacheStorageChild();
 
   // Must be called by the associated CacheStorage listener in its
-  // ActorDestroy() method.  Also, CacheStorage must call SendDestroy() on the
+  // DestroyInternal() method.  Also, CacheStorage must call
-  // actor in its destructor to trigger ActorDestroy() if it has not been
+  // SendDestroyFromListener() on the actor in its destructor to trigger
-  // called yet.
+  // ActorDestroy() if it has not been called yet.
   void ClearListener();
 
   void
   ExecuteOp(nsIGlobalObject* aGlobal, Promise* aPromise,
-            const CacheOpArgs& aArgs);
+            nsISupports* aParent, const CacheOpArgs& aArgs);
 
-  // ActorChild methods
+  // Our parent Listener object has gone out of scope and is being destroyed.
-
+  void StartDestroyFromListener();
-  // Synchronously call ActorDestroy on our CacheStorage listener and then start
-  // the actor destruction asynchronously from the parent-side.
-  virtual void StartDestroy() override;
 
 private:
+  // ActorChild methods
+
+  // Feature is trying to destroy due to worker shutdown.
+  virtual void StartDestroy() override;
+
   // PCacheStorageChild methods
   virtual void ActorDestroy(ActorDestroyReason aReason) override;
 
@@ -67,6 +69,7 @@ private:
   // destroyed.
   CacheStorage* MOZ_NON_OWNING_REF mListener;
   uint32_t mNumChildActors;
+  bool mDelayedDestroy;
 
   NS_DECL_OWNINGTHREAD
 };

dom/cache/Feature.cpp

@@ -13,7 +13,7 @@ namespace mozilla {
 namespace dom {
 namespace cache {
 
-using mozilla::dom::workers::Running;
+using mozilla::dom::workers::Canceling;
 using mozilla::dom::workers::Status;
 using mozilla::dom::workers::WorkerPrivate;
 
@@ -73,7 +73,7 @@ Feature::Notify(JSContext* aCx, Status aStatus)
 {
   NS_ASSERT_OWNINGTHREAD(Feature);
 
-  if (aStatus <= Running || mNotified) {
+  if (aStatus < Canceling || mNotified) {
     return true;
   }