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

@@ -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
 

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

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

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