[webkit-changes] [249000] trunk/Source/WebCore

Thu, 22 Aug 2019 02:44:10 -0700

Title: [249000] trunk/Source/WebCore
Revision
249000
Author
[email protected]
Date
2019-08-22 02:41:57 -0700 (Thu, 22 Aug 2019)

Log 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:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (248999 => 249000)


--- trunk/Source/WebCore/ChangeLog	2019-08-22 09:40:25 UTC (rev 248999)
+++ trunk/Source/WebCore/ChangeLog	2019-08-22 09:41:57 UTC (rev 249000)
@@ -1,3 +1,22 @@
+2019-08-22  Chris Dumez  <[email protected]>
+
+        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  <[email protected]>
 
         Remove the dead code of ScalableImageDecoder for scaling

Modified: trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp (248999 => 249000)


--- trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp	2019-08-22 09:40:25 UTC (rev 248999)
+++ trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp	2019-08-22 09:41:57 UTC (rev 249000)
@@ -56,7 +56,8 @@
 }
 
 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))
 #if !RELEASE_LOG_DISABLED
@@ -63,6 +64,7 @@
     , m_logIdentifier(uniqueLogIdentifier())
 #endif
 {
+    ASSERT(isMainThread());
     UNUSED_PARAM(logger);
 #if !RELEASE_LOG_DISABLED
     m_source->setLogger(m_logger.copyRef(), m_logIdentifier);
@@ -72,6 +74,7 @@
 
 MediaStreamTrackPrivate::~MediaStreamTrackPrivate()
 {
+    ASSERT(isMainThread());
     m_source->removeObserver(*this);
 }
 
@@ -262,7 +265,10 @@
 void MediaStreamTrackPrivate::audioSamplesAvailable(const MediaTime& mediaTime, const PlatformAudioData& data, const AudioStreamDescription& description, size_t sampleCount)
 {
     if (!m_hasSentStartProducedData) {
-        callOnMainThread([this, protectedThis = makeRef(*this)] {
+        callOnMainThread([this, weakThis = m_weakThis] {
+            if (!weakThis)
+                return;
+
             if (!m_haveProducedData) {
                 m_haveProducedData = true;
                 updateReadyState();

Modified: trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h (248999 => 249000)


--- trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h	2019-08-22 09:40:25 UTC (rev 248999)
+++ trunk/Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.h	2019-08-22 09:41:57 UTC (rev 249000)
@@ -31,6 +31,7 @@
 
 #include "RealtimeMediaSource.h"
 #include <wtf/LoggerHelper.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
 
@@ -42,6 +43,7 @@
 
 class MediaStreamTrackPrivate final
     : public ThreadSafeRefCounted<MediaStreamTrackPrivate, WTF::DestructionThread::Main>
+    , public CanMakeWeakPtr<MediaStreamTrackPrivate>
     , public RealtimeMediaSource::Observer
 #if !RELEASE_LOG_DISABLED
     , private LoggerHelper
@@ -142,6 +144,7 @@
     WTFLogChannel& logChannel() const final;
 #endif
 
+    WeakPtr<MediaStreamTrackPrivate> m_weakThis;
     mutable RecursiveLock m_observersLock;
     HashSet<Observer*> m_observers;
     Ref<RealtimeMediaSource> m_source;

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to