Skip to content

Commit 3a0c494

Browse files
pgorszkowski-igaliapgorszkowski-igalia
committed
MEDIA_ERR_ABORTED after restore page with media from bfcache
https://bugs.webkit.org/show_bug.cgi?id=319665 Reviewed by Alicia Boya Garcia and Nikolas Zimmermann. This bundles three related fixes uncovered while restoring pages containing a media element (e.g.: "video") from the back/forward cache: 1. HTMLMediaElement::suspend() destroyed the media player on entering the cache (via stopWithoutDestroyingMediaPlayer() -> userCancelledLoad()), so a restored page reported video.error.code == MEDIA_ERR_ABORTED. userCancelledLoad() now takes a destroyMediaPlayer flag, false for the BackForwardCache suspension path, so it early-returns instead of tearing the player down. 2. A <video> that was playing before being cached did not resume playing after restore. HTMLMediaElement::resume() schedules an updatePlayState() task while the media session is still interrupted. mayResumePlayback() now also retries updatePlayState() when the element isn't paused. 3. A <video> that was paused before being cached showed a blank/white frame instead of its last decoded frame after restore. Restoring a page rebuilds the render tree, so CoordinatedPlatformLayerBufferProxy gets attached to a brand new CoordinatedPlatformLayer with no buffer of its own, and a paused GStreamer pipeline has no new sample to push. The proxy now invokes a layer-attached callback whenever a new non-null layer is attached, which MediaPlayerPrivateGStreamer uses to re-deliver its current sample so the new layer isn't left blank. Also fixes a related macOS-only assert crash (ASSERTION FAILED: m_mediaSessions.contains(identifier) in MediaUsageManagerCocoa:: updateMediaUsage): the UI process unconditionally clears its usage-tracking map on every navigation commit, including a back/forward cache restore, but MediaElementSession kept believing it had already added its session across the whole suspend/resume cycle. mediaUsageManagerSessionWillBeSuspended() resets that flag on suspend so the next update re-adds the session first. A LayoutTest for the playing-before-caching scenario (fix #2): it plays a video, waits for the playing event, navigates away and back via history.back(), and asserts the video is still not paused and currentTime has advanced after restore. Added a LayoutTest based on media/restore-from-page-cache.html for a video that was paused before entering the cache, as a real pixel/screenshot comparison to catch the blank-frame regression from fix #3. Tests: media/restore-from-page-cache-paused.html media/restore-from-page-cache-playing.html * LayoutTests/media/restore-from-page-cache-paused-expected.png: Added. * LayoutTests/media/restore-from-page-cache-paused-expected.txt: Added. * LayoutTests/media/restore-from-page-cache-paused.html: Added. * LayoutTests/media/restore-from-page-cache-playing-expected.txt: Added. * LayoutTests/media/restore-from-page-cache-playing.html: Added. * LayoutTests/platform/ios/TestExpectations: * Source/WebCore/html/HTMLMediaElement.cpp: (WebCore::HTMLMediaElement::userCancelledLoad): (WebCore::HTMLMediaElement::stopWithoutDestroyingMediaPlayer): (WebCore::HTMLMediaElement::suspend): (WebCore::HTMLMediaElement::mayResumePlayback): * Source/WebCore/html/HTMLMediaElement.h: * Source/WebCore/html/MediaElementSession.cpp: (WebCore::MediaElementSession::mediaUsageManagerSessionWillBeSuspended): * Source/WebCore/html/MediaElementSession.h: * Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp: (WebCore::MediaPlayerPrivateGStreamer::MediaPlayerPrivateGStreamer): (WebCore::MediaPlayerPrivateGStreamer::pushTextureToCompositor): (WebCore::MediaPlayerPrivateGStreamer::triggerRepaint): * Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: * Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedPlatformLayerBufferProxy.cpp: (WebCore::CoordinatedPlatformLayerBufferProxy::CoordinatedPlatformLayerBufferProxy): (WebCore::CoordinatedPlatformLayerBufferProxy::create): (WebCore::CoordinatedPlatformLayerBufferProxy::setTargetLayer): * Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedPlatformLayerBufferProxy.h: Canonical link: https://commits.webkit.org/319472@main
1 parent 54382f2 commit 3a0c494

18 files changed

Lines changed: 298 additions & 20 deletions

‎LayoutTests/media/restore-from-page-cache-paused-expected.png‎

50.4 KB
Loading

‎LayoutTests/media/restore-from-page-cache-paused-expected.txt‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
2+
Test that a <video> that was paused before entering the back/forward cache is still paused, and still visibly displaying its last frame, after being restored. The visible-frame part of this is verified by the pixel/screenshot comparison against restore-from-page-cache-paused-expected.png, not by anything in this script.
3+
4+
EVENT(loadeddata)
5+
EXPECTED (loadCount == '0') OK
6+
EXPECTED (video.paused == 'true') OK
7+
*** Page going into cache while paused
8+
*** Page going into cache, video.paused = true
9+
*** Page returned from cache
10+
EXPECTED (loadCount == '1') OK
11+
EXPECTED (video.paused == 'true') OK
12+

‎LayoutTests/media/restore-from-page-cache-paused.html‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
<!-- webkit-test-runner [ UsesBackForwardCache=true ] -->
2+
<html>
3+
<head>
4+
<script src=media-file.js></script>
5+
<script src=video-test.js></script>
6+
<script>
7+
var mediaFile = "content/counting.mp4";
8+
var loadCount = 0;
9+
10+
if (window.testRunner) {
11+
testRunner.dumpAsText(true);
12+
testRunner.waitUntilDone();
13+
}
14+
15+
function pageshow(evt)
16+
{
17+
if (!evt.persisted) {
18+
// this is the first time the page has been loaded, setup the video element
19+
setup();
20+
return;
21+
}
22+
23+
consoleWrite("*** Page returned from cache");
24+
testExpected("loadCount", 1);
25+
26+
video = document.getElementsByTagName("video")[0];
27+
testExpected("video.paused", true);
28+
29+
// Give the restored compositing layer a moment to settle before the harness
30+
// forces a repaint and takes its snapshot.
31+
requestAnimationFrame(function() {
32+
if (window.testRunner)
33+
testRunner.notifyDone();
34+
});
35+
}
36+
37+
function pagehide()
38+
{
39+
consoleWrite("*** Page going into cache, video.paused = " + video.paused);
40+
}
41+
42+
function loadeddata()
43+
{
44+
testExpected("loadCount", 0);
45+
testExpected("video.paused", true);
46+
47+
if (!loadCount) {
48+
consoleWrite("*** Page going into cache while paused");
49+
setTimeout('window.location = "data:text/html,<script>history.back()<" + "/script>"', 0);
50+
}
51+
++loadCount;
52+
}
53+
54+
function setup()
55+
{
56+
video = document.getElementsByTagName("video")[0];
57+
waitForEvent("loadeddata", loadeddata);
58+
video.src = mediaFile;
59+
}
60+
61+
window.onpageshow = pageshow;
62+
window.onpagehide = pagehide;
63+
</script>
64+
</head>
65+
<body>
66+
67+
<video></video>
68+
<p>Test that a &lt;video&gt; that was paused before entering the back/forward cache is
69+
still paused, and still visibly displaying its last frame, after being restored. The
70+
visible-frame part of this is verified by the pixel/screenshot comparison against
71+
restore-from-page-cache-paused-expected.png, not by anything in this script.</p>
72+
</body>
73+
</html>

‎LayoutTests/media/restore-from-page-cache-playing-expected.txt‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
2+
Test that a <video> that was playing before entering the back/forward cache is still playing (and currentTime is advancing again) after being restored.
3+
4+
EVENT(canplaythrough)
5+
EXPECTED (loadCount == '0') OK
6+
EVENT(playing)
7+
*** Page going into cache while playing
8+
*** Page going into cache, video.paused = false
9+
*** Page returned from cache
10+
EXPECTED (loadCount == '1') OK
11+
EXPECTED (video.paused == 'false') OK
12+
EXPECTED (video.currentTime > timeBeforeCache == 'true') OK
13+

‎LayoutTests/media/restore-from-page-cache-playing.html‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
<!-- webkit-test-runner [ UsesBackForwardCache=true ] -->
2+
<html>
3+
<head>
4+
<script src=media-file.js></script>
5+
<script src=video-test.js></script>
6+
<script>
7+
var mediaFile = findMediaFile("video", "content/test");
8+
var loadCount = 0;
9+
var timeBeforeCache;
10+
11+
if (window.testRunner) {
12+
testRunner.dumpAsText();
13+
testRunner.waitUntilDone();
14+
}
15+
16+
function waitForTimeToAdvance() {
17+
if (video.currentTime > timeBeforeCache) {
18+
testExpected("video.currentTime > timeBeforeCache", true);
19+
if (window.testRunner)
20+
testRunner.notifyDone();
21+
return;
22+
}
23+
requestAnimationFrame(waitForTimeToAdvance);
24+
}
25+
26+
function pageshow(evt)
27+
{
28+
if (!evt.persisted) {
29+
// this is the first time the page has been loaded, setup the video element
30+
setup();
31+
return;
32+
}
33+
34+
consoleWrite("*** Page returned from cache");
35+
testExpected("loadCount", 1);
36+
37+
video = document.getElementsByTagName("video")[0];
38+
testExpected("video.paused", false);
39+
40+
waitForTimeToAdvance();
41+
}
42+
43+
function pagehide()
44+
{
45+
timeBeforeCache = video.currentTime;
46+
consoleWrite("*** Page going into cache, video.paused = " + video.paused);
47+
}
48+
49+
function canplaythrough()
50+
{
51+
testExpected("loadCount", 0);
52+
53+
if (!loadCount) {
54+
// Wait for the 'playing' event (not just calling .play()) before caching, so
55+
// the pipeline has actually reached the PLAYING state (not just requested it)
56+
// matching a genuinely-playing video rather than one that was paused
57+
// again before playback really started.
58+
waitForEvent("playing", function() {
59+
consoleWrite("*** Page going into cache while playing");
60+
var blob = new Blob(['<!DOCTYPE html><script>setTimeout(function() { history.back(); }, 0);<\/script>'], { type: "text/html" });
61+
window.location.href = URL.createObjectURL(blob);
62+
}, false, true);
63+
video.play();
64+
}
65+
++loadCount;
66+
}
67+
68+
function setup()
69+
{
70+
video = document.getElementsByTagName("video")[0];
71+
waitForEvent("canplaythrough", canplaythrough);
72+
video.src = mediaFile;
73+
}
74+
75+
window.onpageshow = pageshow;
76+
window.onpagehide = pagehide;
77+
</script>
78+
</head>
79+
<body>
80+
81+
<video></video>
82+
<p>Test that a &lt;video&gt; that was playing before entering the back/forward cache
83+
is still playing (and currentTime is advancing again) after being restored.</p>
84+
</body>
85+
</html>

‎LayoutTests/platform/ios/TestExpectations‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2643,6 +2643,7 @@ media/event-attributes.html [ Failure ]
26432643
media/media-can-play-webm.html [ Failure ]
26442644
media/media-higher-prio-audio-stream.html [ Failure ]
26452645
media/restore-from-page-cache.html [ Failure ]
2646+
media/restore-from-page-cache-playing.html [ Failure ]
26462647
media/track/track-forced-subtitles-in-band.html [ Failure ]
26472648
media/track/track-in-band-mode.html [ Pass Failure ]
26482649
media/track/w3c/interfaces/TextTrackCue/align.html [ Failure ]

‎Source/WebCore/html/HTMLMediaElement.cpp‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6523,16 +6523,17 @@ void HTMLMediaElement::cancelSniffer()
65236523
sniffer->cancel();
65246524
}
65256525

6526-
void HTMLMediaElement::userCancelledLoad()
6526+
void HTMLMediaElement::userCancelledLoad(ShouldDestroyMediaPlayer shouldDestroyMediaPlayer)
65276527
{
65286528
INFO_LOG(LOGIDENTIFIER);
65296529

65306530
// FIXME: We should look to reconcile the iOS and non-iOS code (below).
65316531
#if PLATFORM(IOS_FAMILY)
6532+
UNUSED_PARAM(shouldDestroyMediaPlayer);
65326533
if (m_networkState == NETWORK_EMPTY || m_readyState >= HAVE_METADATA)
65336534
return;
65346535
#else
6535-
if (m_networkState == NETWORK_EMPTY || m_completelyLoaded)
6536+
if (m_networkState == NETWORK_EMPTY || m_completelyLoaded || shouldDestroyMediaPlayer == ShouldDestroyMediaPlayer::No)
65366537
return;
65376538
#endif
65386539

@@ -6665,7 +6666,7 @@ void HTMLMediaElement::stopWithoutDestroyingMediaPlayer()
66656666

66666667
setAutoplayEventPlaybackState(AutoplayEventPlaybackState::None);
66676668

6668-
userCancelledLoad();
6669+
userCancelledLoad(ShouldDestroyMediaPlayer::No);
66696670

66706671
updateRenderer();
66716672

@@ -6718,8 +6719,10 @@ void HTMLMediaElement::suspend(ReasonForSuspension reason)
67186719
case ReasonForSuspension::BackForwardCache:
67196720
stopWithoutDestroyingMediaPlayer();
67206721
setBufferingPolicy(BufferingPolicy::MakeResourcesPurgeable);
6721-
if (m_mediaSession)
6722+
if (m_mediaSession) {
67226723
m_mediaSession->addBehaviorRestriction(MediaElementSession::RequirePageConsentToResumeMedia);
6724+
m_mediaSession->mediaUsageManagerSessionWillBeSuspended();
6725+
}
67236726
break;
67246727
case ReasonForSuspension::PageWillBeSuspended:
67256728
stopWithoutDestroyingMediaPlayer();
@@ -8912,8 +8915,16 @@ void HTMLMediaElement::resumeAutoplaying()
89128915
void HTMLMediaElement::mayResumePlayback(bool shouldResume)
89138916
{
89148917
ALWAYS_LOG(LOGIDENTIFIER, "paused = ", paused());
8915-
if (!ended() && paused() && shouldResume)
8916-
play();
8918+
if (ended())
8919+
return;
8920+
8921+
if (paused()) {
8922+
if (shouldResume)
8923+
play();
8924+
return;
8925+
}
8926+
8927+
updatePlayState();
89178928
}
89188929

89198930
String HTMLMediaElement::mediaSessionTitle() const

‎Source/WebCore/html/HTMLMediaElement.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -908,7 +908,8 @@ class HTMLMediaElement
908908
void loadResource(const URL&, const ContentType&, const String& keySystem);
909909
void scheduleNextSourceChild();
910910
void loadNextSourceChild();
911-
void userCancelledLoad();
911+
enum class ShouldDestroyMediaPlayer : bool { No, Yes };
912+
void userCancelledLoad(ShouldDestroyMediaPlayer = ShouldDestroyMediaPlayer::Yes);
912913
void clearMediaPlayer();
913914
bool havePotentialSourceChild();
914915
void noneSupported();

‎Source/WebCore/html/MediaElementSession.cpp‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,18 @@ void MediaElementSession::addMediaUsageManagerSessionIfNecessary()
206206
#endif
207207
}
208208

209+
void MediaElementSession::mediaUsageManagerSessionWillBeSuspended()
210+
{
211+
#if ENABLE(MEDIA_USAGE)
212+
// The back/forward cache keeps this MediaElementSession (and thus this flag) alive across
213+
// suspend/resume, but the UI process unconditionally clears its usage-tracking map on every
214+
// navigation commit, including the eventual restore commit.
215+
// Reset the flag so the next updateMediaUsageIfChanged() after resuming re-adds the session
216+
// before updating it, instead of sending an update for an identifier the UI process no longer has.
217+
m_haveAddedMediaUsageManagerSession = false;
218+
#endif
219+
}
220+
209221
void MediaElementSession::registerWithDocument(Document& document)
210222
{
211223
#if ENABLE(WIRELESS_PLAYBACK_TARGET)

‎Source/WebCore/html/MediaElementSession.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,7 @@ class MediaElementSession final : public PlatformMediaSession {
150150
bool hasBehaviorRestriction(BehaviorRestrictions restriction) const { return restriction & m_restrictions; }
151151

152152
HTMLMediaElement& element() const { return m_element; }
153+
void mediaUsageManagerSessionWillBeSuspended();
153154

154155
bool wantsToObserveViewportVisibilityForMediaControls() const;
155156
bool wantsToObserveViewportVisibilityForAutoplay() const;

0 commit comments

Comments
 (0)