Changeset 249000 in webkit

Timestamp:

Aug 22, 2019, 2:41:57 AM (7 years ago)

Author:

Chris Dumez

Message:

Fix unsafe usage of MediaStreamTrackPrivate from background thread in MediaStreamTrackPrivate::audioSamplesAvailable()
​https://bugs.webkit.org/show_bug.cgi?id=200924

Reviewed by Youenn Fablet.

MediaStreamTrackPrivate is constructed / destructed on the main thread but its MediaStreamTrackPrivate::audioSamplesAvailable()
gets called on a background thread. The audioSamplesAvailable() method may get called until the MediaStreamTrackPrivate
destructor unregisters |this| as an observer from m_source. Event though MediaStreamTrackPrivate subclasses ThreadSafeRefCounted,
ref'ing |this| on the background thread inside audioSamplesAvailable() is still unsafe as the destructor may already be running
on the main thread.

• platform/mediastream/MediaStreamTrackPrivate.cpp:

(WebCore::MediaStreamTrackPrivate::MediaStreamTrackPrivate):
(WebCore::MediaStreamTrackPrivate::~MediaStreamTrackPrivate):
(WebCore::MediaStreamTrackPrivate::audioSamplesAvailable):

• platform/mediastream/MediaStreamTrackPrivate.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/mediastream/MediaStreamTrackPrivate.cpp (modified) (4 diffs)
• platform/mediastream/MediaStreamTrackPrivate.h (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-08-22  Chris Dumez  <cdumez@apple.com>
+
+        Fix unsafe usage of MediaStreamTrackPrivate from background thread in MediaStreamTrackPrivate::audioSamplesAvailable()
+        https://bugs.webkit.org/show_bug.cgi?id=200924
+
+        Reviewed by Youenn Fablet.
+
+        MediaStreamTrackPrivate is constructed / destructed on the main thread but its MediaStreamTrackPrivate::audioSamplesAvailable()
+        gets called on a background thread. The audioSamplesAvailable() method may get called until the MediaStreamTrackPrivate
+        destructor unregisters |this| as an observer from m_source. Event though MediaStreamTrackPrivate subclasses ThreadSafeRefCounted,
+        ref'ing |this| on the background thread inside audioSamplesAvailable() is still unsafe as the destructor may already be running
+        on the main thread.
+
+        * platform/mediastream/MediaStreamTrackPrivate.cpp:
+        (WebCore::MediaStreamTrackPrivate::MediaStreamTrackPrivate):
+        (WebCore::MediaStreamTrackPrivate::~MediaStreamTrackPrivate):
+        (WebCore::MediaStreamTrackPrivate::audioSamplesAvailable):
+        * platform/mediastream/MediaStreamTrackPrivate.h:
+
 2019-08-22  Fujii Hironori  <Hironori.Fujii@sony.com>
 

trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp

@@ -57 +57 @@
 
 MediaStreamTrackPrivate::MediaStreamTrackPrivate(Ref<const Logger>&& logger, Ref<RealtimeMediaSource>&& source, String&& id)
-    : m_source(WTFMove(source))
+    : m_weakThis(makeWeakPtr(*this))
+    , m_source(WTFMove(source))
     , m_id(WTFMove(id))
     , m_logger(WTFMove(logger))
@@ -64 +65 @@
 #endif
 {
+    ASSERT(isMainThread());
     UNUSED_PARAM(logger);
 #if !RELEASE_LOG_DISABLED
@@ -73 +75 @@
 MediaStreamTrackPrivate::~MediaStreamTrackPrivate()
 {
+    ASSERT(isMainThread());
     m_source->removeObserver(*this);
 }
@@ -263 +266 @@
 {
     if (!m_hasSentStartProducedData) {
-        callOnMainThread([this, protectedThis = makeRef(*this)] {
+        callOnMainThread([this, weakThis = m_weakThis] {
+            if (!weakThis)
+                return;
+
             if (!m_haveProducedData) {
                 m_haveProducedData = true;

trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h

@@ -32 +32 @@
 #include "RealtimeMediaSource.h"
 #include <wtf/LoggerHelper.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
@@ -43 +44 @@
 class MediaStreamTrackPrivate final
     : public ThreadSafeRefCounted<MediaStreamTrackPrivate, WTF::DestructionThread::Main>
+    , public CanMakeWeakPtr<MediaStreamTrackPrivate>
     , public RealtimeMediaSource::Observer
 #if !RELEASE_LOG_DISABLED
@@ -143 +145 @@
 #endif
 
+    WeakPtr<MediaStreamTrackPrivate> m_weakThis;
     mutable RecursiveLock m_observersLock;
     HashSet<Observer*> m_observers;