Low firefox Race 🔧 Commit mapped

Overview

Low
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
Impactlow
DescriptionMissing thread synchronization primitives could have led to a data race on members of the PlaybackParams structure.
ComponentDOM
Bug ClassRace
Tracker1922912
Fix commit4752a783de5f (firefox) +48/-48
CISA KEVNot listed
CreditedSerban Stanca
Disclosed2024-11-26

Changed Functions

FunctionChangeNotes
if
dom/media/mediasink/AudioSink.cpp
modified

Files Changed

  • dom/media/mediasink/AudioSink.cpp
  • dom/media/mediasink/AudioSink.h
  • dom/media/mediasink/AudioSinkWrapper.cpp
  • dom/media/mediasink/AudioSinkWrapper.h
diff --git a/dom/media/mediasink/AudioSink.cpp b/dom/media/mediasink/AudioSink.cpp
index f879adfe9ae..6a824bb7dd7 100644
--- a/dom/media/mediasink/AudioSink.cpp
+++ b/dom/media/mediasink/AudioSink.cpp
@@ -98,7 +98,7 @@ AudioSink::~AudioSink() {
 }
 
 nsresult AudioSink::InitializeAudioStream(
-    const PlaybackParams& aParams, const RefPtr<AudioDeviceInfo>& aAudioDevice,
+    const RefPtr<AudioDeviceInfo>& aAudioDevice,
     AudioSink::InitializationType aInitializationType) {
   if (aInitializationType == AudioSink::InitializationType::UNMUTING) {
     // Consider the stream to be audible immediately, before initialization
@@ -133,19 +133,19 @@ nsresult AudioSink::InitializeAudioStream(
     return rv;
   }
 
-  // Set playback params before calling Start() so they can take effect
-  // as soon as the 1st DataCallback of the AudioStream fires.
-  mAudioStream->SetVolume(aParams.mVolume);
-  mAudioStream->SetPlaybackRate(aParams.mPlaybackRate);
-  mAudioStream->SetPreservesPitch(aParams.mPreservesPitch);
-
   return NS_OK;
 }
 
 RefPtr<MediaSink::EndedPromise> AudioSink::Start(
-    const media::TimeUnit& aStartTime) {
+    const PlaybackParams& aParams, const media::TimeUnit& aStartTime) {
   MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
 
+  // Set playback params before calling Start() so they can take effect
+  // as soon as the 1st DataCallback of the AudioStream fires.
+  mAudioStream->SetVolume(aParams.mVolume);
+  mAudioStream->SetPlaybackRate(aParams.mPlaybackRate);
+  mAudioStream->SetPreservesPitch(aParams.mPreservesPitch);
+
   mAudioQueueListener = mAudioQueue.PushEvent().Connect(
       mOwnerThread, this, &AudioSink::OnAudioPushed);
   mAudioQueueFinishListener = mAudioQueue.FinishEvent().Connect(
diff --git a/dom/media/mediasink/AudioSink.h b/dom/media/mediasink/AudioSink.h
index 154e1bdf619..bda060bc656 100644
--- a/dom/media/mediasink/AudioSink.h
+++ b/dom/media/mediasink/AudioSink.h
@@ -47,13 +47,13 @@ class AudioSink : private AudioStream::DataSource {
   ~AudioSink();
 
   // Allocate and initialize mAudioStream. Returns NS_OK on success.
-  nsresult InitializeAudioStream(const PlaybackParams& aParams,
-                                 const RefPtr<AudioDeviceInfo>& aAudioDevice,
+  nsresult InitializeAudioStream(const RefPtr<AudioDeviceInfo>& aAudioDevice,
                                  InitializationType aInitializationType);
 
   // Start audio playback.  aStartTime is compared with MediaData::mTime to
   // identify the first audio frame to be played.
-  RefPtr<MediaSink::EndedPromise> Start(const media::TimeUnit& aStartTime);
+  RefPtr<MediaSink::EndedPromise> Start(const PlaybackParams& aParams,
+                                        const media::TimeUnit& aStartTime);
 
   /*
    * All public functions are not thread-safe.
diff --git a/dom/media/mediasink/AudioSinkWrapper.cpp b/dom/media/mediasink/AudioSinkWrapper.cpp
index 909de78fd9d..45688614938 100644
--- a/dom/media/mediasink/AudioSinkWrapper.cpp
+++ b/dom/media/mediasink/AudioSinkWrapper.cpp
@@ -332,8 +332,8 @@ void AudioSinkWrapper::StartAudioSink(UniquePtr<AudioSink> aAudioSink,
   AssertOwnerThread();
   MOZ_ASSERT(!mAudioSink);
   mAudioSink = std::move(aAudioSink);
-  mAudioSink->Start(aStartTime)
-      ->Then(mOwnerThread.get(), __func__, this,
+  mAudioSink->Start(mParams, aStartTime)
+      ->Then(mOwnerThread.GetEventTarget(), __func__, this,
              &AudioSinkWrapper::OnAudioEnded)
       ->Track(mAudioSinkEndedRequest);
 }
@@ -370,37 +370,36 @@ RefPtr<GenericPromise> AudioSinkWrapper::MaybeAsyncCreateAudioSink(
   ++mAsyncCreateCount;
   using Promise =
       MozPromise<UniquePtr<AudioSink>, nsresult, /* IsExclusive = */ true>;
-  return InvokeAsync(mAsyncInitTaskQueue,
-                     "MaybeAsyncCreateAudioSink (Async part: initialization)",
-                     [self = RefPtr<AudioSinkWrapper>(this),
-                      audioSink{std::move(audioSink)},
-                      audioDevice = mAudioDevice, this]() mutable {
-                       if (!audioSink || !mAsyncInitTaskQueue->IsEmpty()) {
-                         // Either an AudioSink is not required or there's a
-                         // pending task to init an AudioSink with a possibly
-                         // different device.
-                         return Promise::CreateAndResolve(nullptr, __func__);
-                       }
-
-                       LOG("AudioSink initialization on background thread");
-                       // This can take about 200ms, e.g. on Windows, we don't
-                       // want to do it on the MDSM thread, because it would
-                       // make the clock not update for that amount of time, and
-                       // the video would therefore not update. The Start() call
-                       // is very cheap on the other hand, we can do it from the
-                       // MDSM thread.
-                       nsresult rv = audioSink->InitializeAudioStream(
-                           mParams, audioDevice,
-                           AudioSink::InitializationType::UNMUTING);
-                       if (NS_FAILED(rv)) {
-                         LOG("Async AudioSink initialization failed");
-                         return Promise::CreateAndReject(rv, __func__);
-                       }
-                       return Promise::CreateAndResolve(std::move(audioSink),
-                                                        __func__);
-                     })
+  return InvokeAsync(
+             mAsyncInitTaskQueue,
+             "MaybeAsyncCreateAudioSink (Async part: initialization)",
+             [self = RefPtr<AudioSinkWrapper>(this),
+              audioSink{std::move(audioSink)}, audioDevice = mAudioDevice,
+              this]() mutable {
+               if (!audioSink || !mAsyncInitTaskQueue->IsEmpty()) {
+                 // Either an AudioSink is not required or there's a
+                 // pending task to init an AudioSink with a possibly
+                 // different device.
+                 return Promise::CreateAndResolve(nullptr, __func__);
+               }
+
+               LOG("AudioSink initialization on background thread");
+               // This can take about 200ms, e.g. on Windows, we don't
+               // want to do it on the MDSM thread, because it would
+               // make the clock not update for that amount of time, and
+               // the video would therefore not update. The Start() call
+               // is very cheap on the other hand, we can do it from the
+               // MDSM thread.
+               nsresult rv = audioSink->InitializeAudioStream(
+                   audioDevice, AudioSink::InitializationType::UNMUTING);
+               if (NS_FAILED(rv)) {
+                 LOG("Async AudioSink initialization failed");
+                 return Promise::CreateAndReject(rv, __func__);
+               }
+               return Promise::CreateAndResolve(std::move(audioSink), __func__);
+             })
       ->Then(
-          mOwnerThread,
+          mOwnerThread.GetEventTarget(),
           "MaybeAsyncCreateAudioSink (Async part: start from MDSM thread)",
           [self = RefPtr<AudioSinkWrapper>(this), audioDevice = mAudioDevice,
            this](Promise::ResolveOrRejectValue&& aValue) mutable {
@@ -478,7 +477,7 @@ nsresult AudioSinkWrapper::SyncCreateAudioSink(const TimeUnit& aStartTime) {
 
   UniquePtr<AudioSink> audioSink = mSinkCreator();
   nsresult rv = audioSink->InitializeAudioStream(
-      mParams, mAudioDevice, AudioSink::InitializationType::INITIAL);
+      mAudioDevice, AudioSink::InitializationType::INITIAL);
   if (NS_FAILED(rv)) {
     LOG("Sync AudioSinkWrapper initialization failed");
     // If a specific device has been specified through setSinkId()
diff --git a/dom/media/mediasink/AudioSinkWrapper.h b/dom/media/mediasink/AudioSinkWrapper.h
index 3f0343d5f63..1545ea5def7 100644
--- a/dom/media/mediasink/AudioSinkWrapper.h
+++ b/dom/media/mediasink/AudioSinkWrapper.h
@@ -8,6 +8,7 @@
 #define AudioSinkWrapper_h_
 
 #include "mozilla/AbstractThread.h"
+#include "mozilla/EventTargetCapability.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/UniquePtr.h"
@@ -85,8 +86,8 @@ class AudioSinkWrapper : public MediaSink {
   void OnMuted(bool aMuted);
   virtual ~AudioSinkWrapper();
 
-  void AssertOwnerThread() const {
-    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
+  void AssertOwnerThread() const MOZ_ASSERT_CAPABILITY(mOwnerThread) {
+    mOwnerThread.AssertOnCurrentThread();
   }
 
   bool NeedAudioSink();
@@ -121,7 +122,7 @@ class AudioSinkWrapper : public MediaSink {
 
   bool IsAudioSourceEnded(const MediaInfo& aInfo) const;
 
-  const RefPtr<AbstractThread> mOwnerThread;
+  const EventTargetCapability<AbstractThread> mOwnerThread;
   const RefPtr<TaskQueue> mAsyncInitTaskQueue;
   SinkCreator mSinkCreator;
   UniquePtr<AudioSink> mAudioSink;
@@ -133,7 +134,7 @@ class AudioSinkWrapper : public MediaSink {
   MozPromiseHolder<EndedPromise> mEndedPromiseHolder;
   // true between Start() and Stop()
   bool mIsStarted = false;
-  PlaybackParams mParams;
+  PlaybackParams MOZ_GUARDED_BY(mOwnerThread) mParams;
   // mClockStartTime is null before Start(), after Stop(), and between
   // SetPlaying(false) and SetPlaying(true).  When the system time is used for
   // the clock, this is the time corresponding to mPositionAtClockStart.  When
Loading diff…