From 9c8d43b8a200a4d1f6a175a40f7c0fb3f49e8050 Mon Sep 17 00:00:00 2001 From: roytam1 Date: Tue, 25 Jun 2024 16:03:58 +0800 Subject: [PATCH] import changes from `dev' branch of rmottola/Arctic-Fox: - Bug 1257063 - Don't destruct the runnable inside the lock when TaskQueue::Dispatch fails. r=bobbyholley. (1f6b254bb0) - Bug 1202148 - Move current in only one location in Intervals.h, r=jya (2c98d86b10) - Bug 1258673. Part 1 - cache mStreamOffset so we won't read at the wrong position when Other Read() interrupt the current Read(). r=jya. (87ab65cc30) - Bug 1258673. Part 2 - since mStreamOffset is not updated until the end of MediaCacheStream::Read(), we have to pass stream offset to MediaCache::NoteBlockUsage explicitly to avoid hitting the assertion. r=jya. (f02806ea1c) --- dom/media/FlushableTaskQueue.cpp | 20 +++++++++++------- dom/media/Intervals.h | 7 +------ dom/media/MediaCache.cpp | 36 ++++++++++++++++++-------------- xpcom/threads/TaskQueue.cpp | 13 ++++++------ xpcom/threads/TaskQueue.h | 18 +++++++++++----- 5 files changed, 54 insertions(+), 40 deletions(-) diff --git a/dom/media/FlushableTaskQueue.cpp b/dom/media/FlushableTaskQueue.cpp index 6443045fdd..b28340d964 100644 --- a/dom/media/FlushableTaskQueue.cpp +++ b/dom/media/FlushableTaskQueue.cpp @@ -20,13 +20,19 @@ FlushableTaskQueue::Flush() nsresult FlushableTaskQueue::FlushAndDispatch(already_AddRefed aRunnable) { - MonitorAutoLock mon(mQueueMonitor); - AutoSetFlushing autoFlush(this); - FlushLocked(); - nsCOMPtr r = dont_AddRef(aRunnable.take()); - nsresult rv = DispatchLocked(r.forget(), IgnoreFlushing, AssertDispatchSuccess); - NS_ENSURE_SUCCESS(rv, rv); - AwaitIdleLocked(); + nsCOMPtr r = aRunnable; + { + MonitorAutoLock mon(mQueueMonitor); + AutoSetFlushing autoFlush(this); + FlushLocked(); + nsresult rv = DispatchLocked(/* passed by ref */r, IgnoreFlushing, AssertDispatchSuccess); + NS_ENSURE_SUCCESS(rv, rv); + AwaitIdleLocked(); + } + // If the ownership of |r| is not transferred in DispatchLocked() due to + // dispatch failure, it will be deleted here outside the lock. We do so + // since the destructor of the runnable might access TaskQueue and result + // in deadlocks. return NS_OK; } diff --git a/dom/media/Intervals.h b/dom/media/Intervals.h index 6500765eb0..4e0cbf161f 100644 --- a/dom/media/Intervals.h +++ b/dom/media/Intervals.h @@ -361,23 +361,18 @@ public: ContainerType normalized; ElemType current(aInterval); - bool inserted = false; IndexType i = 0; for (; i < mIntervals.Length(); i++) { ElemType& interval = mIntervals[i]; if (current.Touches(interval)) { current = current.Span(interval); } else if (current.LeftOf(interval)) { - normalized.AppendElement(Move(current)); - inserted = true; break; } else { normalized.AppendElement(Move(interval)); } } - if (!inserted) { - normalized.AppendElement(Move(current)); - } + normalized.AppendElement(Move(current)); for (; i < mIntervals.Length(); i++) { normalized.AppendElement(Move(mIntervals[i])); } diff --git a/dom/media/MediaCache.cpp b/dom/media/MediaCache.cpp index 5b14d23432..0667265ce7 100644 --- a/dom/media/MediaCache.cpp +++ b/dom/media/MediaCache.cpp @@ -181,6 +181,7 @@ public: // and thus hasn't yet been committed to the cache. The caller will // call QueueUpdate(). void NoteBlockUsage(MediaCacheStream* aStream, int32_t aBlockIndex, + int64_t aStreamOffset, MediaCacheStream::ReadMode aMode, TimeStamp aNow); // Mark aStream as having the block, adding it as an owner. void AddBlockOwnerAsReadahead(int32_t aBlockIndex, MediaCacheStream* aStream, @@ -1622,8 +1623,8 @@ MediaCache::Truncate() void MediaCache::NoteBlockUsage(MediaCacheStream* aStream, int32_t aBlockIndex, - MediaCacheStream::ReadMode aMode, - TimeStamp aNow) + int64_t aStreamOffset, + MediaCacheStream::ReadMode aMode, TimeStamp aNow) { mReentrantMonitor.AssertCurrentThreadIn(); @@ -1640,7 +1641,7 @@ MediaCache::NoteBlockUsage(MediaCacheStream* aStream, int32_t aBlockIndex, // The following check has to be <= because the stream offset has // not yet been updated for the data read from this block - NS_ASSERTION(bo->mStreamBlock*BLOCK_SIZE <= bo->mStream->mStreamOffset, + NS_ASSERTION(bo->mStreamBlock*BLOCK_SIZE <= aStreamOffset, "Using a block that's behind the read position?"); GetListForBlock(bo)->RemoveBlock(aBlockIndex); @@ -1673,8 +1674,8 @@ MediaCache::NoteSeek(MediaCacheStream* aStream, int64_t aOldOffset) if (cacheBlockIndex >= 0) { // Marking the block used may not be exactly what we want but // it's simple - NoteBlockUsage(aStream, cacheBlockIndex, MediaCacheStream::MODE_PLAYBACK, - now); + NoteBlockUsage(aStream, cacheBlockIndex, aStream->mStreamOffset, + MediaCacheStream::MODE_PLAYBACK, now); } ++blockIndex; } @@ -2208,17 +2209,20 @@ MediaCacheStream::Read(char* aBuffer, uint32_t aCount, uint32_t* aBytes) if (mClosed) return NS_ERROR_FAILURE; + // Cache the offset in case it is changed again when we are waiting for the + // monitor to be notified to avoid reading at the wrong position. + auto streamOffset = mStreamOffset; + uint32_t count = 0; // Read one block (or part of a block) at a time while (count < aCount) { - uint32_t streamBlock = uint32_t(mStreamOffset/BLOCK_SIZE); - uint32_t offsetInStreamBlock = - uint32_t(mStreamOffset - streamBlock*BLOCK_SIZE); + uint32_t streamBlock = uint32_t(streamOffset/BLOCK_SIZE); + uint32_t offsetInStreamBlock = uint32_t(streamOffset - streamBlock*BLOCK_SIZE); int64_t size = std::min(aCount - count, BLOCK_SIZE - offsetInStreamBlock); if (mStreamLength >= 0) { // Don't try to read beyond the end of the stream - int64_t bytesRemaining = mStreamLength - mStreamOffset; + int64_t bytesRemaining = mStreamLength - streamOffset; if (bytesRemaining <= 0) { // Get out of here and return NS_OK break; @@ -2246,7 +2250,7 @@ MediaCacheStream::Read(char* aBuffer, uint32_t aCount, uint32_t* aBytes) MediaCache::ResourceStreamIterator iter(mResourceID); while (MediaCacheStream* stream = iter.Next()) { if (uint32_t(stream->mChannelOffset/BLOCK_SIZE) == streamBlock && - mStreamOffset < stream->mChannelOffset) { + streamOffset < stream->mChannelOffset) { streamWithPartialBlock = stream; break; } @@ -2255,7 +2259,7 @@ MediaCacheStream::Read(char* aBuffer, uint32_t aCount, uint32_t* aBytes) // We can just use the data in mPartialBlockBuffer. In fact we should // use it rather than waiting for the block to fill and land in // the cache. - int64_t bytes = std::min(size, streamWithPartialBlock->mChannelOffset - mStreamOffset); + int64_t bytes = std::min(size, streamWithPartialBlock->mChannelOffset - streamOffset); // Clamp bytes until 64-bit file size issues are fixed. bytes = std::min(bytes, int64_t(INT32_MAX)); MOZ_ASSERT(bytes >= 0 && bytes <= aCount, "Bytes out of range."); @@ -2264,7 +2268,7 @@ MediaCacheStream::Read(char* aBuffer, uint32_t aCount, uint32_t* aBytes) if (mCurrentMode == MODE_METADATA) { streamWithPartialBlock->mMetadataInPartialBlockBuffer = true; } - mStreamOffset += bytes; + streamOffset += bytes; count = bytes; break; } @@ -2279,7 +2283,7 @@ MediaCacheStream::Read(char* aBuffer, uint32_t aCount, uint32_t* aBytes) continue; } - gMediaCache->NoteBlockUsage(this, cacheBlock, mCurrentMode, TimeStamp::Now()); + gMediaCache->NoteBlockUsage(this, cacheBlock, streamOffset, mCurrentMode, TimeStamp::Now()); int64_t offset = cacheBlock*BLOCK_SIZE + offsetInStreamBlock; int32_t bytes; @@ -2291,7 +2295,7 @@ MediaCacheStream::Read(char* aBuffer, uint32_t aCount, uint32_t* aBytes) // If we did successfully read some data, may as well return it break; } - mStreamOffset += bytes; + streamOffset += bytes; count += bytes; } @@ -2300,9 +2304,9 @@ MediaCacheStream::Read(char* aBuffer, uint32_t aCount, uint32_t* aBytes) // have changed gMediaCache->QueueUpdate(); } - CACHE_LOG(LogLevel::Debug, - ("Stream %p Read at %lld count=%d", this, (long long)(mStreamOffset-count), count)); + CACHE_LOG(LogLevel::Debug, ("Stream %p Read at %lld count=%d", this, streamOffset-count, count)); *aBytes = count; + mStreamOffset = streamOffset; return NS_OK; } diff --git a/xpcom/threads/TaskQueue.cpp b/xpcom/threads/TaskQueue.cpp index bfad502953..cadfebbc0f 100644 --- a/xpcom/threads/TaskQueue.cpp +++ b/xpcom/threads/TaskQueue.cpp @@ -39,15 +39,16 @@ TaskQueue::TailDispatcher() return *mTailDispatcher; } +// Note aRunnable is passed by ref to support conditional ownership transfer. +// See Dispatch() in TaskQueue.h for more details. nsresult -TaskQueue::DispatchLocked(already_AddRefed aRunnable, - DispatchMode aMode, DispatchFailureHandling aFailureHandling, - DispatchReason aReason) +TaskQueue::DispatchLocked(nsCOMPtr& aRunnable, + DispatchMode aMode, DispatchFailureHandling aFailureHandling, + DispatchReason aReason) { - nsCOMPtr r = aRunnable; AbstractThread* currentThread; if (aReason != TailDispatch && (currentThread = GetCurrent()) && RequiresTailDispatch(currentThread)) { - currentThread->TailDispatcher().AddTask(this, r.forget(), aFailureHandling); + currentThread->TailDispatcher().AddTask(this, aRunnable.forget(), aFailureHandling); return NS_OK; } @@ -58,7 +59,7 @@ TaskQueue::DispatchLocked(already_AddRefed aRunnable, if (mIsShutdown) { return NS_ERROR_FAILURE; } - mTasks.push(r.forget()); + mTasks.push(aRunnable.forget()); if (mIsRunning) { return NS_OK; } diff --git a/xpcom/threads/TaskQueue.h b/xpcom/threads/TaskQueue.h index 6b3ebd009e..cd3c6e8870 100644 --- a/xpcom/threads/TaskQueue.h +++ b/xpcom/threads/TaskQueue.h @@ -43,10 +43,17 @@ public: DispatchFailureHandling aFailureHandling = AssertDispatchSuccess, DispatchReason aReason = NormalDispatch) override { - MonitorAutoLock mon(mQueueMonitor); - nsresult rv = DispatchLocked(Move(aRunnable), AbortIfFlushing, aFailureHandling, aReason); - MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess || NS_SUCCEEDED(rv)); - Unused << rv; + nsCOMPtr r = aRunnable; + { + MonitorAutoLock mon(mQueueMonitor); + nsresult rv = DispatchLocked(/* passed by ref */r, AbortIfFlushing, aFailureHandling, aReason); + MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess || NS_SUCCEEDED(rv)); + Unused << rv; + } + // If the ownership of |r| is not transferred in DispatchLocked() due to + // dispatch failure, it will be deleted here outside the lock. We do so + // since the destructor of the runnable might access TaskQueue and result + // in deadlocks. } // Puts the queue in a shutdown state and returns immediately. The queue will @@ -81,7 +88,8 @@ protected: enum DispatchMode { AbortIfFlushing, IgnoreFlushing }; - nsresult DispatchLocked(already_AddRefed aRunnable, DispatchMode aMode, + nsresult DispatchLocked(nsCOMPtr& aRunnable, + DispatchMode aMode, DispatchFailureHandling aFailureHandling, DispatchReason aReason = NormalDispatch);