diff --git a/dom/cache/Cache.cpp b/dom/cache/Cache.cpp index c2bff652928..7a69510bb43 100644 --- a/dom/cache/Cache.cpp +++ b/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( - new CachePushStreamChild(mActor->GetFeature(), aStream)); - MOZ_ASSERT(actor); - return static_cast(actor); + return mActor->CreatePushStream(aStream); } Cache::~Cache() @@ -375,10 +371,7 @@ Cache::ExecuteOp(AutoChildOpArgs& aOpArgs, ErrorResult& aRv) return nullptr; } - unused << mActor->SendPCacheOpConstructor( - new CacheOpChild(mActor->GetFeature(), mGlobal, this, promise), - aOpArgs.SendAsOpArgs()); - + mActor->ExecuteOp(mGlobal, promise, aOpArgs.SendAsOpArgs()); return promise.forget(); } diff --git a/dom/cache/CacheChild.cpp b/dom/cache/CacheChild.cpp index c283d01e240..f814e242001 100644 --- a/dom/cache/CacheChild.cpp +++ b/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/PCachePushStreamChild.h" +#include "mozilla/dom/cache/CacheOpChild.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(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 diff --git a/dom/cache/CacheChild.h b/dom/cache/CacheChild.h index 54250877f45..9c27aaf0f3c 100644 --- a/dom/cache/CacheChild.h +++ b/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 }; diff --git a/dom/cache/CacheOpChild.cpp b/dom/cache/CacheOpChild.cpp index 797e41af522..de667688b71 100644 --- a/dom/cache/CacheOpChild.cpp +++ b/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); diff --git a/dom/cache/CacheOpChild.h b/dom/cache/CacheOpChild.h index 4794bebad35..a2395b9614a 100644 --- a/dom/cache/CacheOpChild.h +++ b/dom/cache/CacheOpChild.h @@ -25,12 +25,15 @@ class CacheOpChild final : public PCacheOpChild , public ActorChild , public TypeUtils { -public: - CacheOpChild(Feature* aFeature, nsIGlobalObject* aGlobal, - nsISupports* aParent, Promise* aPromise); - ~CacheOpChild(); + friend class CacheChild; + friend class CacheStorageChild; 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& aRequestList); nsCOMPtr mGlobal; - nsCOMPtr mParent; nsRefPtr mPromise; NS_DECL_OWNINGTHREAD diff --git a/dom/cache/CachePushStreamChild.h b/dom/cache/CachePushStreamChild.h index cfbf1759181..03b1d3ed4f1 100644 --- a/dom/cache/CachePushStreamChild.h +++ b/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); - ~CachePushStreamChild(); + void Start(); 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; diff --git a/dom/cache/CacheStorage.cpp b/dom/cache/CacheStorage.cpp index c5ed9f74796..a4ff49ed7a8 100644 --- a/dom/cache/CacheStorage.cpp +++ b/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( - new CacheOpChild(mActor->GetFeature(), mGlobal, this, entry->mPromise), - args.SendAsOpArgs()); + mActor->ExecuteOp(mGlobal, entry->mPromise, args.SendAsOpArgs()); } mPendingRequests.Clear(); } diff --git a/dom/cache/CacheStorageChild.cpp b/dom/cache/CacheStorageChild.cpp index eb1e69a4369..15c1af168c6 100644 --- a/dom/cache/CacheStorageChild.cpp +++ b/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 diff --git a/dom/cache/CacheStorageChild.h b/dom/cache/CacheStorageChild.h index d904cf9e118..1d0fa194a44 100644 --- a/dom/cache/CacheStorageChild.h +++ b/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 }; diff --git a/dom/cache/CacheStreamControlChild.cpp b/dom/cache/CacheStreamControlChild.cpp index 920b3050f4e..78b12221dd0 100644 --- a/dom/cache/CacheStreamControlChild.cpp +++ b/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 diff --git a/dom/cache/CacheStreamControlChild.h b/dom/cache/CacheStreamControlChild.h index 50b956809a1..ff0c6c6e715 100644 --- a/dom/cache/CacheStreamControlChild.h +++ b/dom/cache/CacheStreamControlChild.h @@ -56,6 +56,7 @@ private: virtual bool RecvCloseAll() override; bool mDestroyStarted; + bool mDestroyDelayed; NS_DECL_OWNINGTHREAD }; diff --git a/dom/cache/ReadStream.cpp b/dom/cache/ReadStream.cpp index 22aa95cfed9..68fa862742a 100644 --- a/dom/cache/ReadStream.cpp +++ b/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 mState; + Atomic 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; } diff --git a/dom/cache/ReadStream.h b/dom/cache/ReadStream.h index 6c1381b3c0b..eaec9bfd1a9 100644 --- a/dom/cache/ReadStream.h +++ b/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; diff --git a/dom/cache/StreamControl.cpp b/dom/cache/StreamControl.cpp index 8be74bab728..72655d2408f 100644 --- a/dom/cache/StreamControl.cpp +++ b/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 diff --git a/dom/cache/StreamControl.h b/dom/cache/StreamControl.h index a1e5518186c..9dbe50b3f1c 100644 --- a/dom/cache/StreamControl.h +++ b/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;