[webkit-changes] [233539] trunk

Thu, 05 Jul 2018 14:03:14 -0700

Title: [233539] trunk
Revision
233539
Author
[email protected]
Date
2018-07-05 14:02:47 -0700 (Thu, 05 Jul 2018)

Log Message

Youtube video pages crash after a couple of minutes
https://bugs.webkit.org/show_bug.cgi?id=187316

Reviewed by Antti Koivisto.

Source/WebCore:

The crash was caused by HTMLMediaElement::stopWithoutDestroyingMediaPlayer invoking updatePlaybackControlsManager,
which traverses all media players across different documents including the one in the main frame while its iframe
is getting removed (to update the Touch Bar's media control).

Fixed the bug by making this code async in both stopWithoutDestroyingMediaPlayer and ~HTMLMediaElement. To do this,
this patch moves the timer to update the playback controls manager from HTMLMediaElement to Page since scheduling
a timer owned by HTMLMediaElement in its destructor wouldn't work as the timer would get destructed immediately.

Also replaced the call to clientWillPausePlayback by a call to stopSession in stopWithoutDestroyingMediaPlayer
since the former also updates the layout synchronously via updateNowPlayingInfo; the latter function schedules
a timer via scheduleUpdateNowPlayingInfo instead.

Test: media/remove-video-best-media-element-in-main-frame-crash.html

* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::~HTMLMediaElement): Call scheduleUpdatePlaybackControlsManager now that timer has been
moved to Page.
(WebCore::HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager): Made this return a RefPtr instead of
a raw pointer while we're at it.
(WebCore::HTMLMediaElement::clearMediaPlayer): Call scheduleUpdatePlaybackControlsManager.
(WebCore::HTMLMediaElement::stopWithoutDestroyingMediaPlayer): Ditto. Also invoke stopSession instead of
clientWillPausePlayback on MediaSession since clientWillPausePlayback will synchronously try to update the layout.
(WebCore::HTMLMediaElement::contextDestroyed):
(WebCore::HTMLMediaElement::stop):
(WebCore::HTMLMediaElement::scheduleUpdatePlaybackControlsManager):
(WebCore::HTMLMediaElement::updatePlaybackControlsManager): Moved to Page::schedulePlaybackControlsManagerUpdate.
* html/HTMLMediaElement.h:
* page/Page.cpp:
(WebCore::Page::schedulePlaybackControlsManagerUpdate): Added.
* page/Page.h:
* testing/Internals.cpp:
(WebCore::Internals::bestMediaElementForShowingPlaybackControlsManager):
* testing/Internals.h:

Source/WebKitLegacy/mac:

* WebView/WebView.mm:
(-[WebView _preferencesChanged:]):

LayoutTests:

Added a regression test to remove an iframe with a video while there is a main content
which is eligible to be shown in the Touch Bar.

* media/remove-video-best-media-element-in-main-frame-crash-expected.txt: Added.
* media/remove-video-best-media-element-in-main-frame-crash.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (233538 => 233539)


--- trunk/LayoutTests/ChangeLog	2018-07-05 20:41:48 UTC (rev 233538)
+++ trunk/LayoutTests/ChangeLog	2018-07-05 21:02:47 UTC (rev 233539)
@@ -1,3 +1,16 @@
+2018-07-04  Ryosuke Niwa  <[email protected]>
+
+        Youtube video pages crash after a couple of minutes
+        https://bugs.webkit.org/show_bug.cgi?id=187316
+
+        Reviewed by Antti Koivisto.
+
+        Added a regression test to remove an iframe with a video while there is a main content
+        which is eligible to be shown in the Touch Bar.
+
+        * media/remove-video-best-media-element-in-main-frame-crash-expected.txt: Added.
+        * media/remove-video-best-media-element-in-main-frame-crash.html: Added.
+
 2018-07-05  Zalan Bujtas  <[email protected]>
 
         Do not assume that hypen's width can be computed using the simplified text measure codepath.

Added: trunk/LayoutTests/media/remove-video-best-media-element-in-main-frame-crash-expected.txt (0 => 233539)


--- trunk/LayoutTests/media/remove-video-best-media-element-in-main-frame-crash-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/media/remove-video-best-media-element-in-main-frame-crash-expected.txt	2018-07-05 21:02:47 UTC (rev 233539)
@@ -0,0 +1,4 @@
+This tests removing a video element inside an iframe while another video in the main frame is playing does not hit a release assertion.
+
+PASS
+

Added: trunk/LayoutTests/media/remove-video-best-media-element-in-main-frame-crash.html (0 => 233539)


--- trunk/LayoutTests/media/remove-video-best-media-element-in-main-frame-crash.html	                        (rev 0)
+++ trunk/LayoutTests/media/remove-video-best-media-element-in-main-frame-crash.html	2018-07-05 21:02:47 UTC (rev 233539)
@@ -0,0 +1,49 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p id="description">This tests removing a video element inside an iframe while another video in the main frame is playing does not hit a release assertion.</p>
+<div id="result"></div>
+<video id="video" autoplay width="500"></video>
+<script src=""
+<script>
+
+if (window.testRunner) {
+    testRunner.waitUntilDone();
+    testRunner.dumpAsText();
+}
+
+let count = 0;
+function continueTest() {
+    count++;
+    if (count < 2)
+        return;
+    setTimeout(() => {
+        iframe.remove();
+        result.textContent = 'PASS';
+        if (window.testRunner)
+            testRunner.notifyDone();
+    }, 0);
+}
+
+if (window.internals) {
+    testRunner.overridePreference("WebKitMainContentUserGestureOverrideEnabled", 1);
+    internals.setMediaElementRestrictions(video, "OverrideUserGestureRequirementForMainContent");
+} else
+    description.innerHTML += '<br>This test requries OverrideUserGestureRequirementForMainContent. Manually set this setting or use WebKitTestRunner.';
+
+const videoURL = findMediaFile("video", "content/test");
+const iframe = document.createElement('iframe');
+document.body.appendChild(iframe);
+video.src = ""
+video.addEventListener('loadeddata', continueTest);
+
+const doc = iframe.contentDocument;
+doc.open();
+doc.write(`<!DOCTYPE html><html><body>`);
+doc.write(`<video autoplay src=""
+doc.write(`<script>document.querySelector('video').addEventListener('loadeddata', () => top.continueTest());</` + `script>`);
+doc.close();
+
+</script>
+</body>
+</html>
\ No newline at end of file

Modified: trunk/Source/WebCore/ChangeLog (233538 => 233539)


--- trunk/Source/WebCore/ChangeLog	2018-07-05 20:41:48 UTC (rev 233538)
+++ trunk/Source/WebCore/ChangeLog	2018-07-05 21:02:47 UTC (rev 233539)
@@ -1,3 +1,44 @@
+2018-07-04  Ryosuke Niwa  <[email protected]>
+
+        Youtube video pages crash after a couple of minutes
+        https://bugs.webkit.org/show_bug.cgi?id=187316
+
+        Reviewed by Antti Koivisto.
+
+        The crash was caused by HTMLMediaElement::stopWithoutDestroyingMediaPlayer invoking updatePlaybackControlsManager,
+        which traverses all media players across different documents including the one in the main frame while its iframe
+        is getting removed (to update the Touch Bar's media control).
+
+        Fixed the bug by making this code async in both stopWithoutDestroyingMediaPlayer and ~HTMLMediaElement. To do this,
+        this patch moves the timer to update the playback controls manager from HTMLMediaElement to Page since scheduling
+        a timer owned by HTMLMediaElement in its destructor wouldn't work as the timer would get destructed immediately.
+
+        Also replaced the call to clientWillPausePlayback by a call to stopSession in stopWithoutDestroyingMediaPlayer
+        since the former also updates the layout synchronously via updateNowPlayingInfo; the latter function schedules
+        a timer via scheduleUpdateNowPlayingInfo instead.
+
+        Test: media/remove-video-best-media-element-in-main-frame-crash.html
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::~HTMLMediaElement): Call scheduleUpdatePlaybackControlsManager now that timer has been
+        moved to Page.
+        (WebCore::HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager): Made this return a RefPtr instead of
+        a raw pointer while we're at it.
+        (WebCore::HTMLMediaElement::clearMediaPlayer): Call scheduleUpdatePlaybackControlsManager.
+        (WebCore::HTMLMediaElement::stopWithoutDestroyingMediaPlayer): Ditto. Also invoke stopSession instead of
+        clientWillPausePlayback on MediaSession since clientWillPausePlayback will synchronously try to update the layout.
+        (WebCore::HTMLMediaElement::contextDestroyed):
+        (WebCore::HTMLMediaElement::stop):
+        (WebCore::HTMLMediaElement::scheduleUpdatePlaybackControlsManager):
+        (WebCore::HTMLMediaElement::updatePlaybackControlsManager): Moved to Page::schedulePlaybackControlsManagerUpdate.
+        * html/HTMLMediaElement.h:
+        * page/Page.cpp:
+        (WebCore::Page::schedulePlaybackControlsManagerUpdate): Added.
+        * page/Page.h:
+        * testing/Internals.cpp:
+        (WebCore::Internals::bestMediaElementForShowingPlaybackControlsManager):
+        * testing/Internals.h:
+
 2018-07-05  Zalan Bujtas  <[email protected]>
 
         Do not assume that hypen's width can be computed using the simplified text measure codepath.

Modified: trunk/Source/WebCore/html/HTMLMediaElement.cpp (233538 => 233539)


--- trunk/Source/WebCore/html/HTMLMediaElement.cpp	2018-07-05 20:41:48 UTC (rev 233538)
+++ trunk/Source/WebCore/html/HTMLMediaElement.cpp	2018-07-05 21:02:47 UTC (rev 233539)
@@ -680,7 +680,6 @@
     m_seekTaskQueue.close();
     m_promiseTaskQueue.close();
     m_pauseAfterDetachedTaskQueue.close();
-    m_updatePlaybackControlsManagerQueue.close();
     m_playbackControlsManagerBehaviorRestrictionsQueue.close();
     m_resourceSelectionTaskQueue.close();
     m_visibilityChangeTaskQueue.close();
@@ -696,7 +695,7 @@
     }
 
     m_mediaSession = nullptr;
-    updatePlaybackControlsManager();
+    scheduleUpdatePlaybackControlsManager();
 }
 
 static bool needsAutoplayPlayPauseEventsQuirk(const Document& document)
@@ -709,7 +708,7 @@
     return loader && loader->allowedAutoplayQuirks().contains(AutoplayQuirk::SynthesizedPauseEvents);
 }
 
-HTMLMediaElement* HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose purpose)
+RefPtr<HTMLMediaElement> HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose purpose)
 {
     auto allSessions = PlatformMediaSessionManager::sharedManager().currentSessionsMatching([] (const PlatformMediaSession& session) {
         return is<MediaElementSession>(session);
@@ -5522,7 +5521,7 @@
         m_player->invalidate();
         m_player = nullptr;
     }
-    updatePlaybackControlsManager();
+    scheduleUpdatePlaybackControlsManager();
 
     stopPeriodicTimers();
     m_pendingActionTimer.stop();
@@ -5562,13 +5561,13 @@
 
     setPreparedToReturnVideoLayerToInline(true);
 
-    updatePlaybackControlsManager();
+    scheduleUpdatePlaybackControlsManager();
     setInActiveDocument(false);
 
     // Stop the playback without generating events
     setPlaying(false);
     setPausedInternal(true);
-    m_mediaSession->clientWillPausePlayback();
+    m_mediaSession->stopSession();
 
     setPlaybackWithoutUserGesture(PlaybackWithoutUserGesture::None);
 
@@ -5587,7 +5586,6 @@
     m_shadowDOMTaskQueue.close();
     m_promiseTaskQueue.close();
     m_pauseAfterDetachedTaskQueue.close();
-    m_updatePlaybackControlsManagerQueue.close();
 #if ENABLE(ENCRYPTED_MEDIA)
     m_encryptedMediaQueue.close();
 #endif
@@ -5606,7 +5604,6 @@
 
     m_asyncEventQueue.close();
     m_promiseTaskQueue.close();
-    m_updatePlaybackControlsManagerQueue.close();
     m_resourceSelectionTaskQueue.close();
 
     // Once an active DOM object has been stopped it can not be restarted, so we can deallocate
@@ -7908,25 +7905,14 @@
     return renderer && renderer->visibleInViewportState() == VisibleInViewportState::Yes;
 }
 
-void HTMLMediaElement::updatePlaybackControlsManager()
+void HTMLMediaElement::scheduleUpdatePlaybackControlsManager()
 {
     Page* page = document().page();
     if (!page)
         return;
-
-    // FIXME: Ensure that the renderer here should be up to date.
-    if (auto bestMediaElement = bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose::ControlsManager))
-        page->chrome().client().setUpPlaybackControlsManager(*bestMediaElement);
-    else
-        page->chrome().client().clearPlaybackControlsManager();
+    page->schedulePlaybackControlsManagerUpdate();
 }
 
-void HTMLMediaElement::scheduleUpdatePlaybackControlsManager()
-{
-    if (!m_updatePlaybackControlsManagerQueue.hasPendingTasks())
-        m_updatePlaybackControlsManagerQueue.enqueueTask(std::bind(&HTMLMediaElement::updatePlaybackControlsManager, this));
-}
-
 void HTMLMediaElement::playbackControlsManagerBehaviorRestrictionsTimerFired()
 {
     if (m_playbackControlsManagerBehaviorRestrictionsQueue.hasPendingTasks())

Modified: trunk/Source/WebCore/html/HTMLMediaElement.h (233538 => 233539)


--- trunk/Source/WebCore/html/HTMLMediaElement.h	2018-07-05 20:41:48 UTC (rev 233538)
+++ trunk/Source/WebCore/html/HTMLMediaElement.h	2018-07-05 21:02:47 UTC (rev 233539)
@@ -155,7 +155,7 @@
 
     static HashSet<HTMLMediaElement*>& allMediaElements();
 
-    WEBCORE_EXPORT static HTMLMediaElement* bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose);
+    WEBCORE_EXPORT static RefPtr<HTMLMediaElement> bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose);
 
     static bool isRunningDestructor();
 
@@ -933,7 +933,6 @@
     GenericTaskQueue<Timer> m_shadowDOMTaskQueue;
     GenericTaskQueue<Timer> m_promiseTaskQueue;
     GenericTaskQueue<Timer> m_pauseAfterDetachedTaskQueue;
-    GenericTaskQueue<Timer> m_updatePlaybackControlsManagerQueue;
     GenericTaskQueue<Timer> m_playbackControlsManagerBehaviorRestrictionsQueue;
     GenericTaskQueue<Timer> m_resourceSelectionTaskQueue;
     GenericTaskQueue<Timer> m_visibilityChangeTaskQueue;

Modified: trunk/Source/WebCore/page/Page.cpp (233538 => 233539)


--- trunk/Source/WebCore/page/Page.cpp	2018-07-05 20:41:48 UTC (rev 233538)
+++ trunk/Source/WebCore/page/Page.cpp	2018-07-05 21:02:47 UTC (rev 233539)
@@ -1487,6 +1487,19 @@
     chrome().client().isPlayingMediaDidChange(state, sourceElementID);
 }
 
+void Page::schedulePlaybackControlsManagerUpdate()
+{
+    if (m_playbackControlsManagerUpdateTimer)
+        return;
+
+    m_playbackControlsManagerUpdateTimer = std::make_unique<DeferrableOneShotTimer>([this] () mutable {
+        if (auto bestMediaElement = HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose::ControlsManager))
+            chrome().client().setUpPlaybackControlsManager(*bestMediaElement);
+        else
+            chrome().client().clearPlaybackControlsManager();
+    }, 0_s);
+}
+
 void Page::setMuted(MediaProducer::MutedStateFlags muted)
 {
     if (m_mutedState == muted)

Modified: trunk/Source/WebCore/page/Page.h (233538 => 233539)


--- trunk/Source/WebCore/page/Page.h	2018-07-05 20:41:48 UTC (rev 233538)
+++ trunk/Source/WebCore/page/Page.h	2018-07-05 21:02:47 UTC (rev 233539)
@@ -569,6 +569,7 @@
     MediaProducer::MutedStateFlags mutedState() const { return m_mutedState; }
     bool isAudioMuted() const { return m_mutedState & MediaProducer::AudioIsMuted; }
     bool isMediaCaptureMuted() const { return m_mutedState & MediaProducer::CaptureDevicesAreMuted; };
+    void schedulePlaybackControlsManagerUpdate();
     WEBCORE_EXPORT void setMuted(MediaProducer::MutedStateFlags);
     WEBCORE_EXPORT void stopMediaCapture();
 
@@ -849,7 +850,9 @@
     bool m_isRestoringCachedPage { false };
 
     MediaProducer::MediaStateFlags m_mediaState { MediaProducer::IsNotPlaying };
-    
+
+    std::unique_ptr<DeferrableOneShotTimer> m_playbackControlsManagerUpdateTimer;
+
     bool m_allowsMediaDocumentInlinePlayback { false };
     bool m_allowsPlaybackControlsForAutoplayingAudio { false };
     bool m_showAllPlugins { false };

Modified: trunk/Source/WebCore/testing/Internals.cpp (233538 => 233539)


--- trunk/Source/WebCore/testing/Internals.cpp	2018-07-05 20:41:48 UTC (rev 233538)
+++ trunk/Source/WebCore/testing/Internals.cpp	2018-07-05 21:02:47 UTC (rev 233539)
@@ -3750,7 +3750,7 @@
 }
 
 #if ENABLE(VIDEO)
-HTMLMediaElement* Internals::bestMediaElementForShowingPlaybackControlsManager(Internals::PlaybackControlsPurpose purpose)
+RefPtr<HTMLMediaElement> Internals::bestMediaElementForShowingPlaybackControlsManager(Internals::PlaybackControlsPurpose purpose)
 {
     return HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(purpose);
 }

Modified: trunk/Source/WebCore/testing/Internals.h (233538 => 233539)


--- trunk/Source/WebCore/testing/Internals.h	2018-07-05 20:41:48 UTC (rev 233538)
+++ trunk/Source/WebCore/testing/Internals.h	2018-07-05 21:02:47 UTC (rev 233539)
@@ -704,7 +704,7 @@
 
 #if ENABLE(VIDEO)
     using PlaybackControlsPurpose = MediaElementSession::PlaybackControlsPurpose;
-    HTMLMediaElement* bestMediaElementForShowingPlaybackControlsManager(PlaybackControlsPurpose);
+    RefPtr<HTMLMediaElement> bestMediaElementForShowingPlaybackControlsManager(PlaybackControlsPurpose);
 
     using MediaSessionState = PlatformMediaSession::State;
     MediaSessionState mediaSessionState(HTMLMediaElement&);

Modified: trunk/Source/WebKitLegacy/mac/ChangeLog (233538 => 233539)


--- trunk/Source/WebKitLegacy/mac/ChangeLog	2018-07-05 20:41:48 UTC (rev 233538)
+++ trunk/Source/WebKitLegacy/mac/ChangeLog	2018-07-05 21:02:47 UTC (rev 233539)
@@ -1,3 +1,13 @@
+2018-07-04  Ryosuke Niwa  <[email protected]>
+
+        Youtube video pages crash after a couple of minutes
+        https://bugs.webkit.org/show_bug.cgi?id=187316
+
+        Reviewed by Antti Koivisto.
+
+        * WebView/WebView.mm:
+        (-[WebView _preferencesChanged:]):
+
 2018-07-04  Tim Horton  <[email protected]>
 
         Introduce PLATFORM(IOSMAC)

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

Reply via email to