Crash under HTMLMediaElement::{resolve, reject}PendingPlayPromises() when playback...
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 30 Jul 2016 00:30:13 +0000 (00:30 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 30 Jul 2016 00:30:13 +0000 (00:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=160366
<rdar://problem/27317407>

Reviewed by Eric Carlson.

Source/WebCore:

Fixes a crash/assertion failure in DeferredWrapper::{resolve, rejectWithValue}() caused by a Promise
being settled twice. In particular, if a system interruption occurs when media.play() is invoked
the returned Promise may ultimately be settled twice upon cessation of the interruption.

A Promise can be settled (resolved) exactly once. When a system interruption occurs media
playback is paused and resumes on cessation of the interruption. Currently we also immediately
reject the Promise p retuned by media.play() if the interruption occurs during its invocation.
So, when we resume playback on cessation of an interruption we try to resolve p again. But a
Promise can only be resolved once and hence we violate the assertions that p has both a valid
reference to a JSPromiseDeferred object and a reference to the global object of the page.

Tests: media/non-existent-video-playback-interrupted.html
       media/video-playback-interrupted.html

* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::play): Modified to reject the Promise and return immediately if
playInternal() returns false.
(WebCore::HTMLMediaElement::playInternal): Treat an interruption as success and return true
so that HTMLMediaElement::play() adds the Promise to the end of the list of pending promises.
We treat an interruption as a success because we will resume playback (assuming the media
can be loaded and is well-formed) upon cessation of the interruption and therefore can either
fulfill or reject the Promise object returned by media.play().

LayoutTests:

* media/non-existent-video-playback-interrupted-expected.txt: Added.
* media/non-existent-video-playback-interrupted.html: Added.
* media/video-playback-interrupted-expected.txt: Added.
* media/video-playback-interrupted.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/media/non-existent-video-playback-interrupted-expected.txt [new file with mode: 0644]
LayoutTests/media/non-existent-video-playback-interrupted.html [new file with mode: 0644]
LayoutTests/media/video-playback-interrupted-expected.txt [new file with mode: 0644]
LayoutTests/media/video-playback-interrupted.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLMediaElement.cpp

index d20e9ae..8e10ee4 100644 (file)
@@ -1,3 +1,16 @@
+2016-07-29  Daniel Bates  <dabates@apple.com>
+
+        Crash under HTMLMediaElement::{resolve, reject}PendingPlayPromises() when playback is interrupted
+        https://bugs.webkit.org/show_bug.cgi?id=160366
+        <rdar://problem/27317407>
+
+        Reviewed by Eric Carlson.
+
+        * media/non-existent-video-playback-interrupted-expected.txt: Added.
+        * media/non-existent-video-playback-interrupted.html: Added.
+        * media/video-playback-interrupted-expected.txt: Added.
+        * media/video-playback-interrupted.html: Added.
+
 2016-07-29  Ryan Haddad  <ryanhaddad@apple.com>
 
         Land test expectations for rdar://problem/27611932.
diff --git a/LayoutTests/media/non-existent-video-playback-interrupted-expected.txt b/LayoutTests/media/non-existent-video-playback-interrupted-expected.txt
new file mode 100644 (file)
index 0000000..41e59a4
--- /dev/null
@@ -0,0 +1,11 @@
+
+RUN(internals.setMediaSessionRestrictions("video", "InterruptedPlaybackNotPermitted"))
+RUN(video.src = "non-existent.mp4")
+EXPECTED (video.paused == 'true') OK
+RUN(internals.beginMediaSessionInterruption("System"))
+RUN(video.play().then(didResolvePromise).catch(didRejectPromise))
+RUN(internals.endMediaSessionInterruption("MayResumePlaying"))
+Promise rejected. OK
+EXPECTED (error.name == 'NotSupportedError') OK
+END OF TEST
+
diff --git a/LayoutTests/media/non-existent-video-playback-interrupted.html b/LayoutTests/media/non-existent-video-playback-interrupted.html
new file mode 100644 (file)
index 0000000..badfd7e
--- /dev/null
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="media-file.js"></script>
+<script src="video-test.js"></script>
+<script>
+var error;
+
+function didResolvePromise()
+{
+    logResult(Failed, "Expected promise to be rejected. Was resolved.");
+
+    // Wait some time before ending the test towards ensuring that we ended the session interruption.
+    endTestLater();
+}
+
+function didRejectPromise(e)
+{
+    error = e;
+    logResult(true, "Promise rejected.");
+    testExpected("error.name", "NotSupportedError");
+
+    // Wait some time before ending the test towards ensuring that we ended the session interruption.
+    endTestLater();
+}
+
+function runTest()
+{
+    if (!window.internals) {
+        failTest("This test must be run in DumpRenderTree or WebKitTestRunner.");
+        return;
+    }
+    findMediaElement();
+    run('internals.setMediaSessionRestrictions("video", "InterruptedPlaybackNotPermitted")');
+    run('video.src = "non-existent.mp4"');
+    testExpected("video.paused", true);
+    run('internals.beginMediaSessionInterruption("System")');
+    run("video.play().then(didResolvePromise).catch(didRejectPromise)");
+    run('internals.endMediaSessionInterruption("MayResumePlaying")');
+}
+
+window.onload = runTest;
+</script>
+</head>
+<body>
+<video></video>
+</body>
+</html>
diff --git a/LayoutTests/media/video-playback-interrupted-expected.txt b/LayoutTests/media/video-playback-interrupted-expected.txt
new file mode 100644 (file)
index 0000000..a584d6f
--- /dev/null
@@ -0,0 +1,11 @@
+
+RUN(internals.setMediaSessionRestrictions("video", "InterruptedPlaybackNotPermitted"))
+RUN(video.src = findMediaFile("video", "content/test"))
+EXPECTED (video.paused == 'true') OK
+RUN(internals.beginMediaSessionInterruption("System"))
+RUN(video.play().then(didResolvePromise).catch(didRejectPromise))
+RUN(internals.endMediaSessionInterruption("MayResumePlaying"))
+Promise resolved. OK
+EXPECTED (video.paused == 'false') OK
+END OF TEST
+
diff --git a/LayoutTests/media/video-playback-interrupted.html b/LayoutTests/media/video-playback-interrupted.html
new file mode 100644 (file)
index 0000000..20c75e9
--- /dev/null
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="media-file.js"></script>
+<script src="video-test.js"></script>
+<script>
+function didResolvePromise()
+{
+    logResult(true, "Promise resolved.");
+    testExpected("video.paused", false);
+
+    // Wait some time before ending the test towards ensuring that we ended the session interruption.
+    endTestLater();
+}
+
+function didRejectPromise(error)
+{
+    logResult(Failed, "Expected promise to be resolved. Was rejected with error " + error);
+
+    // Wait some time before ending the test towards ensuring that we ended the session interruption.
+    endTestLater();
+}
+
+function runTest()
+{
+    if (!window.internals) {
+        failTest("This test must be run in DumpRenderTree or WebKitTestRunner.");
+        return;
+    }
+    findMediaElement();
+    run('internals.setMediaSessionRestrictions("video", "InterruptedPlaybackNotPermitted")');
+    run('video.src = findMediaFile("video", "content/test")');
+    testExpected("video.paused", true);
+    run('internals.beginMediaSessionInterruption("System")');
+    run("video.play().then(didResolvePromise).catch(didRejectPromise)");
+    run('internals.endMediaSessionInterruption("MayResumePlaying")');
+}
+
+window.onload = runTest;
+</script>
+</head>
+<body>
+<video></video>
+</body>
+</html>
index 312a17f..718ee2c 100644 (file)
@@ -1,5 +1,36 @@
 2016-07-29  Daniel Bates  <dabates@apple.com>
 
+        Crash under HTMLMediaElement::{resolve, reject}PendingPlayPromises() when playback is interrupted
+        https://bugs.webkit.org/show_bug.cgi?id=160366
+        <rdar://problem/27317407>
+
+        Reviewed by Eric Carlson.
+
+        Fixes a crash/assertion failure in DeferredWrapper::{resolve, rejectWithValue}() caused by a Promise
+        being settled twice. In particular, if a system interruption occurs when media.play() is invoked
+        the returned Promise may ultimately be settled twice upon cessation of the interruption.
+
+        A Promise can be settled (resolved) exactly once. When a system interruption occurs media
+        playback is paused and resumes on cessation of the interruption. Currently we also immediately
+        reject the Promise p retuned by media.play() if the interruption occurs during its invocation.
+        So, when we resume playback on cessation of an interruption we try to resolve p again. But a
+        Promise can only be resolved once and hence we violate the assertions that p has both a valid
+        reference to a JSPromiseDeferred object and a reference to the global object of the page.
+
+        Tests: media/non-existent-video-playback-interrupted.html
+               media/video-playback-interrupted.html
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::play): Modified to reject the Promise and return immediately if
+        playInternal() returns false.
+        (WebCore::HTMLMediaElement::playInternal): Treat an interruption as success and return true
+        so that HTMLMediaElement::play() adds the Promise to the end of the list of pending promises.
+        We treat an interruption as a success because we will resume playback (assuming the media
+        can be loaded and is well-formed) upon cessation of the interruption and therefore can either
+        fulfill or reject the Promise object returned by media.play().
+
+2016-07-29  Daniel Bates  <dabates@apple.com>
+
         [iOS] HTMLMediaElement::updateVolume() calls MediaPlayer::volume() on null media player
         https://bugs.webkit.org/show_bug.cgi?id=160353
 
index 4613ac4..6f9f8d0 100644 (file)
@@ -3068,8 +3068,10 @@ void HTMLMediaElement::play(PlayPromise&& promise)
     if (ScriptController::processingUserGestureForMedia())
         removeBehaviorsRestrictionsAfterFirstUserGesture();
 
-    if (!playInternal())
+    if (!playInternal()) {
         promise.reject(NotAllowedError);
+        return;
+    }
 
     m_pendingPlayPromises.append(WTFMove(promise));
 }
@@ -3092,7 +3094,7 @@ bool HTMLMediaElement::playInternal()
     
     if (!m_mediaSession->clientWillBeginPlayback()) {
         LOG(Media, "  returning because of interruption");
-        return false;
+        return true; // Treat as success because we will begin playback on cessation of the interruption.
     }
 
     // 4.8.10.9. Playing the media resource