Bug 1398711. P2 - write to mClosed only when the cache monitor is held.
This fixes the data race when Seek() read mClosed off the main thread.
MozReview-Commit-ID: GO7Kk5VgVpg
--- a/dom/media/MediaCache.cpp
+++ b/dom/media/MediaCache.cpp
@@ -445,17 +445,16 @@ MediaCacheFlusher::Observe(nsISupports *
}
return NS_OK;
}
MediaCacheStream::MediaCacheStream(ChannelMediaResource* aClient,
bool aIsPrivateBrowsing)
: mMediaCache(nullptr)
, mClient(aClient)
- , mClosed(false)
, mDidNotifyDataEnded(false)
, mResourceID(0)
, mIsTransportSeekable(false)
, mCacheSuspended(false)
, mChannelEnded(false)
, mChannelOffset(0)
, mStreamLength(-1)
, mStreamOffset(0)
@@ -1873,16 +1872,19 @@ MediaCacheStream::UpdatePrincipal(nsIPri
return nsContentUtils::CombineResourcePrincipals(&mPrincipal, aPrincipal);
}
void
MediaCacheStream::NotifyDataReceived(int64_t aSize, const char* aData,
nsIPrincipal* aPrincipal)
{
// This might happen off the main thread.
+
+ // It is safe to read mClosed without holding the monitor because this
+ // function is guaranteed to happen before Close().
MOZ_DIAGNOSTIC_ASSERT(!mClosed);
// Update principals before putting the data in the cache. This is important,
// we want to make sure all principals are updated before any consumer
// can see the new data.
// We do this without holding the cache monitor, in case the client wants
// to do something that takes a lock.
{
@@ -2105,24 +2107,23 @@ void
MediaCacheStream::Close()
{
NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
if (!mMediaCache || mClosed) {
return;
}
- mClosed = true;
-
// Closing a stream will change the return value of
// MediaCacheStream::AreAllStreamsForResourceSuspended as well as
// ChannelMediaResource::IsSuspendedByCache. Let's notify it.
mMediaCache->QueueSuspendedStatusUpdate(mResourceID);
ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
+ mClosed = true;
mMediaCache->ReleaseStreamBlocks(this);
// Wake up any blocked readers
mon.NotifyAll();
// Queue an Update since we may have created more free space. Don't do
// it from CloseInternal since that gets called by Update() itself
// sometimes, and we try to not to queue updates from Update().
mMediaCache->QueueUpdate();
--- a/dom/media/MediaCache.h
+++ b/dom/media/MediaCache.h
@@ -216,19 +216,21 @@ public:
// be called while the stream is at channel offset 0. Seekability can
// change during the lifetime of the MediaCacheStream --- every time
// we do an HTTP load the seekability may be different (and sometimes
// is, in practice, due to the effects of caching proxies).
void SetTransportSeekable(bool aIsTransportSeekable);
// This must be called (and return) before the ChannelMediaResource
// used to create this MediaCacheStream is deleted.
void Close();
- // This returns true when the stream has been closed
+ // This returns true when the stream has been closed.
+ // Must be used on the main thread or while holding the cache lock.
bool IsClosed() const { return mClosed; }
- // Returns true when this stream is can be shared by a new resource load
+ // Returns true when this stream is can be shared by a new resource load.
+ // Called on the main thread only.
bool IsAvailableForSharing() const
{
return !mClosed && !mIsPrivateBrowsing &&
(!mDidNotifyDataEnded || NS_SUCCEEDED(mNotifyDataEndedStatus));
}
// Get the principal for this stream. Anything accessing the contents of
// this stream must have a principal that subsumes this principal.
nsIPrincipal* GetCurrentPrincipal() { return mPrincipal; }
@@ -431,26 +433,26 @@ private:
bool UpdatePrincipal(nsIPrincipal* aPrincipal);
// Instance of MediaCache to use with this MediaCacheStream.
RefPtr<MediaCache> mMediaCache;
// These fields are main-thread-only.
ChannelMediaResource* mClient;
nsCOMPtr<nsIPrincipal> mPrincipal;
- // Set to true when the stream has been closed either explicitly or
- // due to an internal cache error
- bool mClosed;
// True if CacheClientNotifyDataEnded has been called for this stream.
bool mDidNotifyDataEnded;
// The following fields must be written holding the cache's monitor and
// only on the main thread, thus can be read either on the main thread
// or while holding the cache's monitor.
+ // Set to true when the stream has been closed either explicitly or
+ // due to an internal cache error
+ bool mClosed = false;
// This is a unique ID representing the resource we're loading.
// All streams with the same mResourceID are loading the same
// underlying resource and should share data.
int64_t mResourceID;
// The last reported seekability state for the underlying channel
bool mIsTransportSeekable;
// True if the cache has suspended our channel because the cache is
// full and the priority of the data that would be received is lower