Safari WebKitWebRTCAudioModule crash during <video> tag update when audio track prese...
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Mar 2018 16:55:10 +0000 (16:55 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Mar 2018 16:55:10 +0000 (16:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181180
<rdar://problem/36302375>

Reviewed by Eric Carlson.

Source/WebCore:

Test: webrtc/video-update-often.html

AudioTrackPrivateMediaStreamCocoa needs to be destroyed in the main thread since it owns a Ref to its MediaStreamTrackPrivate.
We can still ref it on a background thread but we always deref it on the main thread.

* platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp:
(WebCore::AudioTrackPrivateMediaStreamCocoa::audioSamplesAvailable):
(WebCore::AudioTrackPrivateMediaStreamCocoa::render):

LayoutTests:

* webrtc/video-update-often-expected.txt: Added.
* webrtc/video-update-often.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@229898 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/webrtc/video-update-often-expected.txt [new file with mode: 0644]
LayoutTests/webrtc/video-update-often.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp

index 05a30f5..767c1f7 100644 (file)
@@ -1,3 +1,14 @@
+2018-03-23  Youenn Fablet  <youenn@apple.com>
+
+        Safari WebKitWebRTCAudioModule crash during <video> tag update when audio track present in MediaStream
+        https://bugs.webkit.org/show_bug.cgi?id=181180
+        <rdar://problem/36302375>
+
+        Reviewed by Eric Carlson.
+
+        * webrtc/video-update-often-expected.txt: Added.
+        * webrtc/video-update-often.html: Added.
+
 2018-03-23  Antoine Quint  <graouts@apple.com>
 
         [Web Animations] infinite repeat counts aren't reflected for CSS Animations
diff --git a/LayoutTests/webrtc/video-update-often-expected.txt b/LayoutTests/webrtc/video-update-often-expected.txt
new file mode 100644 (file)
index 0000000..3d54952
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS Video element being updated did not crash 
+
diff --git a/LayoutTests/webrtc/video-update-often.html b/LayoutTests/webrtc/video-update-often.html
new file mode 100644 (file)
index 0000000..c27b43d
--- /dev/null
@@ -0,0 +1,56 @@
+<!doctype html>
+<html>
+    <head>
+        <meta charset="utf-8">
+        <title>Testing video being updated and changed often</title>
+        <script src="../resources/gc.js"></script>
+        <script src="../resources/testharness.js"></script>
+        <script src="../resources/testharnessreport.js"></script>
+    </head>
+    <body>
+        <video id="video" autoplay=""></video>
+        <script src ="routines.js"></script>
+        <script>
+video = document.getElementById("video");
+var counter =0;
+var resolveFunction;
+
+async function updateVideoElement()
+{
+    await video.play();
+    if (++counter >= 4) {
+        video.srcObject = null;
+        document.body.removeChild(video);
+        resolveFunction();
+        return;
+    }
+
+    document.body.removeChild(video);
+    video.srcObject = null;
+    video = document.createElement('video');
+    video.setAttribute("id", "video");
+    video.autoplay = true;
+    video.srcObject = await navigator.mediaDevices.getUserMedia({audio: true, video: true});
+    document.body.appendChild(video);
+    if (window.gc)
+        gc();
+    setTimeout(updateVideoElement, 0);
+}
+
+promise_test((test) => {
+    if (window.testRunner)
+        testRunner.setUserMediaPermission(true);
+
+    return navigator.mediaDevices.getUserMedia({audio: true, video: true}).then((stream) => {
+        video.srcObject = stream;
+        return video.play();
+    }).then(async () => {
+        await new Promise((resolve) => {
+            resolveFunction = resolve;
+            updateVideoElement();
+        });
+    });
+}, "Video element being updated did not crash");
+        </script>
+    </body>
+</html>
index b885c4a..69e7665 100644 (file)
@@ -1,3 +1,20 @@
+2018-03-23  Youenn Fablet  <youenn@apple.com>
+
+        Safari WebKitWebRTCAudioModule crash during <video> tag update when audio track present in MediaStream
+        https://bugs.webkit.org/show_bug.cgi?id=181180
+        <rdar://problem/36302375>
+
+        Reviewed by Eric Carlson.
+
+        Test: webrtc/video-update-often.html
+
+        AudioTrackPrivateMediaStreamCocoa needs to be destroyed in the main thread since it owns a Ref to its MediaStreamTrackPrivate.
+        We can still ref it on a background thread but we always deref it on the main thread.
+
+        * platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp:
+        (WebCore::AudioTrackPrivateMediaStreamCocoa::audioSamplesAvailable):
+        (WebCore::AudioTrackPrivateMediaStreamCocoa::render):
+
 2018-03-23  Sergio Villar Senin  <svillar@igalia.com>
 
         [css-grid] Fix auto repeat tracks computation with definite min sizes
index c24fc5a..35e9352 100644 (file)
@@ -34,6 +34,7 @@
 
 #include <pal/cf/CoreMediaSoftLink.h>
 #include <pal/spi/cocoa/AudioToolboxSPI.h>
+#include <wtf/Scope.h>
 
 #if ENABLE(VIDEO_TRACK) && ENABLE(MEDIA_STREAM)
 
@@ -169,7 +170,10 @@ void AudioTrackPrivateMediaStreamCocoa::audioSamplesAvailable(const MediaTime& s
 {
     // This function is called on a background thread. The following protectedThis object ensures the object is not
     // destroyed on the main thread before this function exits.
-    Ref<AudioTrackPrivateMediaStreamCocoa> protectedThis { *this };
+    auto scopeExit = WTF::makeScopeExit([protectedThis = makeRef(*this)]() mutable {
+        callOnMainThread([protectedThis = WTFMove(protectedThis)] { });
+    });
+
     ASSERT(description.platformDescription().type == PlatformDescription::CAAudioStreamBasicType);
 
     if (!m_inputDescription || *m_inputDescription != description) {
@@ -220,7 +224,9 @@ OSStatus AudioTrackPrivateMediaStreamCocoa::render(UInt32 sampleCount, AudioBuff
 {
     // This function is called on a high-priority background thread. The following protectedThis object ensures the object is not
     // destroyed on the main thread before this function exits.
-    Ref<AudioTrackPrivateMediaStreamCocoa> protectedThis { *this };
+    auto scopeExit = WTF::makeScopeExit([protectedThis = makeRef(*this)]() mutable {
+        callOnMainThread([protectedThis = WTFMove(protectedThis)] { });
+    });
 
     if (!m_isPlaying || m_muted || !m_dataSource || streamTrack().muted() || streamTrack().ended() || !streamTrack().enabled()) {
         AudioSampleBufferList::zeroABL(ioData, static_cast<size_t>(sampleCount * m_outputDescription->bytesPerFrame()));