From f6ebda2a9d01140b4ab1370fe27811623e804296 Mon Sep 17 00:00:00 2001
From: alwu <alwu@mozilla.com>
Date: Tue, 9 Apr 2024 17:05:19 +0000
Subject: [PATCH] Bug 1644383 - add mutexs to avoid data race.
r=media-playback-reviewers,padenot
Differential Revision: https://phabricator.services.mozilla.com/D206943
---
dom/media/ipc/RemoteMediaDataDecoder.cpp | 17 +++++++++--
dom/media/ipc/RemoteMediaDataDecoder.h | 16 +++++-----
.../platforms/wrappers/MediaChangeMonitor.cpp | 7 +++++
.../platforms/wrappers/MediaChangeMonitor.h | 30 ++++++++++++-------
4 files changed, 51 insertions(+), 19 deletions(-)
diff --git a/dom/media/ipc/RemoteMediaDataDecoder.cpp b/dom/media/ipc/RemoteMediaDataDecoder.cpp
index 6db3c0d940c20..32e1ee6b31bac 100644
--- a/dom/media/ipc/RemoteMediaDataDecoder.cpp
+++ b/dom/media/ipc/RemoteMediaDataDecoder.cpp
@@ -18,7 +18,12 @@ namespace mozilla {
##__VA_ARGS__)
RemoteMediaDataDecoder::RemoteMediaDataDecoder(RemoteDecoderChild* aChild)
- : mChild(aChild) {
+ : mChild(aChild),
+ mDescription("RemoteMediaDataDecoder"_ns),
+ mProcessName("unknown"_ns),
+ mCodecName("unknown"_ns),
+ mIsHardwareAccelerated(false),
+ mConversion(ConversionRequired::kNeedNone) {
LOG("%p is created", this);
}
@@ -48,6 +53,7 @@ RefPtr<MediaDataDecoder::InitPromise> RemoteMediaDataDecoder::Init() {
->Then(
RemoteDecoderManagerChild::GetManagerThread(), __func__,
[self, this](TrackType aTrack) {
+ MutexAutoLock lock(mMutex);
// If shutdown has started in the meantime shutdown promise may
// be resloved before this task. In this case mChild will be null
// and the init promise has to be canceled.
@@ -127,6 +133,7 @@ RefPtr<ShutdownPromise> RemoteMediaDataDecoder::Shutdown() {
bool RemoteMediaDataDecoder::IsHardwareAccelerated(
nsACString& aFailureReason) const {
+ MutexAutoLock lock(mMutex);
aFailureReason = mHardwareAcceleratedReason;
return mIsHardwareAccelerated;
}
@@ -145,18 +152,24 @@ void RemoteMediaDataDecoder::SetSeekThreshold(const media::TimeUnit& aTime) {
MediaDataDecoder::ConversionRequired RemoteMediaDataDecoder::NeedsConversion()
const {
+ MutexAutoLock lock(mMutex);
return mConversion;
}
nsCString RemoteMediaDataDecoder::GetDescriptionName() const {
+ MutexAutoLock lock(mMutex);
return mDescription;
}
nsCString RemoteMediaDataDecoder::GetProcessName() const {
+ MutexAutoLock lock(mMutex);
return mProcessName;
}
-nsCString RemoteMediaDataDecoder::GetCodecName() const { return mCodecName; }
+nsCString RemoteMediaDataDecoder::GetCodecName() const {
+ MutexAutoLock lock(mMutex);
+ return mCodecName;
+}
#undef LOG
diff --git a/dom/media/ipc/RemoteMediaDataDecoder.h b/dom/media/ipc/RemoteMediaDataDecoder.h
index 4acc5801f73e4..5d8612529d48e 100644
--- a/dom/media/ipc/RemoteMediaDataDecoder.h
+++ b/dom/media/ipc/RemoteMediaDataDecoder.h
@@ -53,14 +53,16 @@ class RemoteMediaDataDecoder final
// destructor when we can guarantee no other threads are accessing it). Only
// read from the manager thread.
RefPtr<RemoteDecoderChild> mChild;
+
+ mutable Mutex mMutex{"RemoteMediaDataDecoder"};
+
// Only ever written/modified during decoder initialisation.
- // As such can be accessed from any threads after that.
- nsCString mDescription = "RemoteMediaDataDecoder"_ns;
- nsCString mProcessName = "unknown"_ns;
- nsCString mCodecName = "unknown"_ns;
- bool mIsHardwareAccelerated = false;
- nsCString mHardwareAcceleratedReason;
- ConversionRequired mConversion = ConversionRequired::kNeedNone;
+ nsCString mDescription MOZ_GUARDED_BY(mMutex);
+ nsCString mProcessName MOZ_GUARDED_BY(mMutex);
+ nsCString mCodecName MOZ_GUARDED_BY(mMutex);
+ bool mIsHardwareAccelerated MOZ_GUARDED_BY(mMutex);
+ nsCString mHardwareAcceleratedReason MOZ_GUARDED_BY(mMutex);
+ ConversionRequired mConversion MOZ_GUARDED_BY(mMutex);
};
} // namespace mozilla
diff --git a/dom/media/platforms/wrappers/MediaChangeMonitor.cpp b/dom/media/platforms/wrappers/MediaChangeMonitor.cpp
index 544abea161b86..5295a9ab44b0a 100644
--- a/dom/media/platforms/wrappers/MediaChangeMonitor.cpp
+++ b/dom/media/platforms/wrappers/MediaChangeMonitor.cpp
@@ -668,6 +668,7 @@ RefPtr<ShutdownPromise> MediaChangeMonitor::ShutdownDecoder() {
AssertOnThread();
mConversionRequired.reset();
if (mDecoder) {
+ MutexAutoLock lock(mMutex);
RefPtr<MediaDataDecoder> decoder = std::move(mDecoder);
return decoder->Shutdown();
}
@@ -715,6 +716,7 @@ MediaChangeMonitor::CreateDecoder() {
->Then(
GetCurrentSerialEventTarget(), __func__,
[self = RefPtr{this}, this](RefPtr<MediaDataDecoder>&& aDecoder) {
+ MutexAutoLock lock(mMutex);
mDecoder = std::move(aDecoder);
DDLINKCHILD("decoder", mDecoder.get());
return CreateDecoderPromise::CreateAndResolve(true, __func__);
@@ -963,6 +965,11 @@ void MediaChangeMonitor::FlushThenShutdownDecoder(
->Track(mFlushRequest);
}
+MediaDataDecoder* MediaChangeMonitor::GetDecoderOnNonOwnerThread() const {
+ MutexAutoLock lock(mMutex);
+ return mDecoder;
+}
+
#undef LOG
} // namespace mozilla
diff --git a/dom/media/platforms/wrappers/MediaChangeMonitor.h b/dom/media/platforms/wrappers/MediaChangeMonitor.h
index 843dd9035b4e2..89f67d500f841 100644
--- a/dom/media/platforms/wrappers/MediaChangeMonitor.h
+++ b/dom/media/platforms/wrappers/MediaChangeMonitor.h
@@ -41,34 +41,34 @@ class MediaChangeMonitor final
RefPtr<ShutdownPromise> Shutdown() override;
bool IsHardwareAccelerated(nsACString& aFailureReason) const override;
nsCString GetDescriptionName() const override {
- if (mDecoder) {
- return mDecoder->GetDescriptionName();
+ if (RefPtr<MediaDataDecoder> decoder = GetDecoderOnNonOwnerThread()) {
+ return decoder->GetDescriptionName();
}
return "MediaChangeMonitor decoder (pending)"_ns;
}
nsCString GetProcessName() const override {
- if (mDecoder) {
- return mDecoder->GetProcessName();
+ if (RefPtr<MediaDataDecoder> decoder = GetDecoderOnNonOwnerThread()) {
+ return decoder->GetProcessName();
}
return "MediaChangeMonitor"_ns;
}
nsCString GetCodecName() const override {
- if (mDecoder) {
- return mDecoder->GetCodecName();
+ if (RefPtr<MediaDataDecoder> decoder = GetDecoderOnNonOwnerThread()) {
+ return decoder->GetCodecName();
}
return "MediaChangeMonitor"_ns;
}
void SetSeekThreshold(const media::TimeUnit& aTime) override;
bool SupportDecoderRecycling() const override {
- if (mDecoder) {
- return mDecoder->SupportDecoderRecycling();
+ if (RefPtr<MediaDataDecoder> decoder = GetDecoderOnNonOwnerThread()) {
+ return decoder->SupportDecoderRecycling();
}
return false;
}
ConversionRequired NeedsConversion() const override {
- if (mDecoder) {
- return mDecoder->NeedsConversion();
+ if (RefPtr<MediaDataDecoder> decoder = GetDecoderOnNonOwnerThread()) {
+ return decoder->NeedsConversion();
}
// Default so no conversion is performed.
return ConversionRequired::kNeedNone;
@@ -97,6 +97,9 @@ class MediaChangeMonitor final
MOZ_ASSERT(!mThread || mThread->IsOnCurrentThread());
}
+ // This is used for getting decoder debug info on other threads. Thread-safe.
+ MediaDataDecoder* GetDecoderOnNonOwnerThread() const;
+
bool CanRecycleDecoder() const;
typedef MozPromise<bool, MediaResult, true /* exclusive */>
@@ -137,6 +140,13 @@ class MediaChangeMonitor final
const CreateDecoderParamsForAsync mParams;
// Keep any seek threshold set for after decoder creation and initialization.
Maybe<media::TimeUnit> mPendingSeekThreshold;
+
+ // This lock is used for mDecoder specifically, but it doens't need to be used
+ // for every places accessing mDecoder which is mostly on the owner thread.
+ // However, when requesting decoder debug info, it can happen on other
+ // threads, so we need this mutex to avoid the data race of
+ // creating/destroying decoder and accessing decoder's debug info.
+ mutable Mutex MOZ_ANNOTATED mMutex{"MediaChangeMonitor"};
};
} // namespace mozilla
--
GitLab