⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 233560 in webkit


Ignore:
Timestamp:
Jul 5, 2018, 8:20:20 PM (8 years ago)
Author:
rniwa@webkit.org
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::schedulePlaybackControlsManagerUpdate): Renamed from scheduleUpdatePlaybackControlsManager.
(WebCore::HTMLMediaElement::updatePlaybackControlsManager): Moved to Page::playbackControlsManagerUpdateTimerFired.

  • html/HTMLMediaElement.h:
  • page/Page.cpp:

(WebCore::Page::Page):
(WebCore::Page::schedulePlaybackControlsManagerUpdate): Added.
(WebCore::Page::playbackControlsManagerUpdateTimerFired): Moved from HTMLMediaElement::updatePlaybackControlsManager.

  • page/Page.h:
  • testing/Internals.cpp:

(WebCore::Internals::bestMediaElementForShowingPlaybackControlsManager):

  • testing/Internals.h:

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.
Location:
trunk
Files:
2 added
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r233556 r233560  
     12018-07-05  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Youtube video pages crash after a couple of minutes
     4        https://bugs.webkit.org/show_bug.cgi?id=187316
     5
     6        Reviewed by Antti Koivisto.
     7
     8        Added a regression test to remove an iframe with a video while there is a main content
     9        which is eligible to be shown in the Touch Bar.
     10
     11        * media/remove-video-best-media-element-in-main-frame-crash-expected.txt: Added.
     12        * media/remove-video-best-media-element-in-main-frame-crash.html: Added.
     13
    1142018-07-05  Fujii Hironori  <Hironori.Fujii@sony.com>
    215

  • trunk/Source/WebCore/ChangeLog

    r233557 r233560  
     12018-07-05  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Youtube video pages crash after a couple of minutes
     4        https://bugs.webkit.org/show_bug.cgi?id=187316
     5
     6        Reviewed by Antti Koivisto.
     7
     8        The crash was caused by HTMLMediaElement::stopWithoutDestroyingMediaPlayer invoking updatePlaybackControlsManager,
     9        which traverses all media players across different documents including the one in the main frame while its iframe
     10        is getting removed (to update the Touch Bar's media control).
     11
     12        Fixed the bug by making this code async in both stopWithoutDestroyingMediaPlayer and ~HTMLMediaElement. To do this,
     13        this patch moves the timer to update the playback controls manager from HTMLMediaElement to Page since scheduling
     14        a timer owned by HTMLMediaElement in its destructor wouldn't work as the timer would get destructed immediately.
     15
     16        Also replaced the call to clientWillPausePlayback by a call to stopSession in stopWithoutDestroyingMediaPlayer
     17        since the former also updates the layout synchronously via updateNowPlayingInfo; the latter function schedules
     18        a timer via scheduleUpdateNowPlayingInfo instead.
     19
     20        Test: media/remove-video-best-media-element-in-main-frame-crash.html
     21
     22        * html/HTMLMediaElement.cpp:
     23        (WebCore::HTMLMediaElement::~HTMLMediaElement): Call scheduleUpdatePlaybackControlsManager now that timer has been
     24        moved to Page.
     25        (WebCore::HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager): Made this return a RefPtr instead of
     26        a raw pointer while we're at it.
     27        (WebCore::HTMLMediaElement::clearMediaPlayer): Call scheduleUpdatePlaybackControlsManager.
     28        (WebCore::HTMLMediaElement::stopWithoutDestroyingMediaPlayer): Ditto. Also invoke stopSession instead of
     29        clientWillPausePlayback on MediaSession since clientWillPausePlayback will synchronously try to update the layout.
     30        (WebCore::HTMLMediaElement::contextDestroyed):
     31        (WebCore::HTMLMediaElement::stop):
     32        (WebCore::HTMLMediaElement::schedulePlaybackControlsManagerUpdate): Renamed from scheduleUpdatePlaybackControlsManager.
     33        (WebCore::HTMLMediaElement::updatePlaybackControlsManager): Moved to Page::playbackControlsManagerUpdateTimerFired.
     34        * html/HTMLMediaElement.h:
     35        * page/Page.cpp:
     36        (WebCore::Page::Page):
     37        (WebCore::Page::schedulePlaybackControlsManagerUpdate): Added.
     38        (WebCore::Page::playbackControlsManagerUpdateTimerFired): Moved from HTMLMediaElement::updatePlaybackControlsManager.
     39        * page/Page.h:
     40        * testing/Internals.cpp:
     41        (WebCore::Internals::bestMediaElementForShowingPlaybackControlsManager):
     42        * testing/Internals.h:
     43
    1442018-07-05  Ryosuke Niwa  <rniwa@webkit.org>
    245

  • trunk/Source/WebCore/html/HTMLMediaElement.cpp

    r233557 r233560  
    681681    m_promiseTaskQueue.close();
    682682    m_pauseAfterDetachedTaskQueue.close();
    683     m_updatePlaybackControlsManagerQueue.close();
    684683    m_playbackControlsManagerBehaviorRestrictionsQueue.close();
    685684    m_resourceSelectionTaskQueue.close();
    … …  
    697696
    698697    m_mediaSession = nullptr;
    699     updatePlaybackControlsManager();
     698    schedulePlaybackControlsManagerUpdate();
    700699}
    701700
    … …  
    710709}
    711710
    712 HTMLMediaElement* HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose purpose)
     711RefPtr<HTMLMediaElement> HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose purpose)
    713712{
    714713    auto allSessions = PlatformMediaSessionManager::sharedManager().currentSessionsMatching([] (const PlatformMediaSession& session) {
    … …  
    11411140    resolvePendingPlayPromises(WTFMove(pendingPlayPromises));
    11421141
    1143     scheduleUpdatePlaybackControlsManager();
     1142    schedulePlaybackControlsManagerUpdate();
    11441143}
    11451144
    … …  
    37453744    }
    37463745
    3747     scheduleUpdatePlaybackControlsManager();
     3746    schedulePlaybackControlsManagerUpdate();
    37483747}
    37493748
    … …  
    44044403    if (!m_receivedLayoutSizeChanged) {
    44054404        m_receivedLayoutSizeChanged = true;
    4406         scheduleUpdatePlaybackControlsManager();
     4405        schedulePlaybackControlsManagerUpdate();
    44074406    }
    44084407
    … …  
    53305329
    53315330    if (shouldBePlaying) {
    5332         scheduleUpdatePlaybackControlsManager();
     5331        schedulePlaybackControlsManagerUpdate();
    53335332
    53345333        setDisplayMode(Video);
    … …  
    53595358        setPlaying(true);
    53605359    } else {
    5361         scheduleUpdatePlaybackControlsManager();
     5360        schedulePlaybackControlsManagerUpdate();
    53625361
    53635362        if (!playerPaused)
    … …  
    55235522        m_player = nullptr;
    55245523    }
    5525     updatePlaybackControlsManager();
     5524    schedulePlaybackControlsManagerUpdate();
    55265525
    55275526    stopPeriodicTimers();
    … …  
    55635562    setPreparedToReturnVideoLayerToInline(true);
    55645563
    5565     updatePlaybackControlsManager();
     5564    schedulePlaybackControlsManagerUpdate();
    55665565    setInActiveDocument(false);
    55675566
    … …  
    55695568    setPlaying(false);
    55705569    setPausedInternal(true);
    5571     m_mediaSession->clientWillPausePlayback();
     5570    m_mediaSession->stopSession();
    55725571
    55735572    setPlaybackWithoutUserGesture(PlaybackWithoutUserGesture::None);
    … …  
    55885587    m_promiseTaskQueue.close();
    55895588    m_pauseAfterDetachedTaskQueue.close();
    5590     m_updatePlaybackControlsManagerQueue.close();
    55915589#if ENABLE(ENCRYPTED_MEDIA)
    55925590    m_encryptedMediaQueue.close();
    … …  
    56075605    m_asyncEventQueue.close();
    56085606    m_promiseTaskQueue.close();
    5609     m_updatePlaybackControlsManagerQueue.close();
    56105607    m_resourceSelectionTaskQueue.close();
    56115608
    … …  
    65546551    m_player = MediaPlayer::create(*this);
    65556552    m_player->setShouldBufferData(m_shouldBufferData);
    6556     scheduleUpdatePlaybackControlsManager();
     6553    schedulePlaybackControlsManagerUpdate();
    65576554
    65586555#if ENABLE(WEB_AUDIO)
    … …  
    78667863        m_mediaSession->isVisibleInViewportChanged();
    78677864        updateShouldAutoplay();
    7868         scheduleUpdatePlaybackControlsManager();
     7865        schedulePlaybackControlsManagerUpdate();
    78697866    });
    78707867}
    … …  
    79097906}
    79107907
    7911 void HTMLMediaElement::updatePlaybackControlsManager()
     7908void HTMLMediaElement::schedulePlaybackControlsManagerUpdate()
    79127909{
    79137910    Page* page = document().page();
    79147911    if (!page)
    79157912        return;
    7916 
    7917     // FIXME: Ensure that the renderer here should be up to date.
    7918     if (auto bestMediaElement = bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose::ControlsManager))
    7919         page->chrome().client().setUpPlaybackControlsManager(*bestMediaElement);
    7920     else
    7921         page->chrome().client().clearPlaybackControlsManager();
    7922 }
    7923 
    7924 void HTMLMediaElement::scheduleUpdatePlaybackControlsManager()
    7925 {
    7926     if (!m_updatePlaybackControlsManagerQueue.hasPendingTasks())
    7927         m_updatePlaybackControlsManagerQueue.enqueueTask(std::bind(&HTMLMediaElement::updatePlaybackControlsManager, this));
     7913    page->schedulePlaybackControlsManagerUpdate();
    79287914}
    79297915
    … …  
    79437929
    79447930        mediaElementSession->addBehaviorRestriction(MediaElementSession::RequirePlaybackToControlControlsManager);
    7945         protectedThis->scheduleUpdatePlaybackControlsManager();
     7931        protectedThis->schedulePlaybackControlsManagerUpdate();
    79467932    });
    79477933}
    … …  
    79647950    m_videoFullscreenMode = mode;
    79657951    visibilityStateChanged();
    7966     scheduleUpdatePlaybackControlsManager();
     7952    schedulePlaybackControlsManagerUpdate();
    79677953}
    79687954

  • trunk/Source/WebCore/html/HTMLMediaElement.h

    r233549 r233560  
    156156    static HashSet<HTMLMediaElement*>& allMediaElements();
    157157
    158     WEBCORE_EXPORT static HTMLMediaElement* bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose);
     158    WEBCORE_EXPORT static RefPtr<HTMLMediaElement> bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose);
    159159
    160160    static bool isRunningDestructor();
    … …  
    899899    void pauseAfterDetachedTask();
    900900    void updatePlaybackControlsManager();
    901     void scheduleUpdatePlaybackControlsManager();
     901    void schedulePlaybackControlsManagerUpdate();
    902902    void playbackControlsManagerBehaviorRestrictionsTimerFired();
    903903
    … …  
    934934    GenericTaskQueue<Timer> m_promiseTaskQueue;
    935935    GenericTaskQueue<Timer> m_pauseAfterDetachedTaskQueue;
    936     GenericTaskQueue<Timer> m_updatePlaybackControlsManagerQueue;
    937936    GenericTaskQueue<Timer> m_playbackControlsManagerBehaviorRestrictionsQueue;
    938937    GenericTaskQueue<Timer> m_resourceSelectionTaskQueue;

  • trunk/Source/WebCore/page/Page.cpp

    r233552 r233560  
    240240    , m_visitedLinkStore(*WTFMove(pageConfiguration.visitedLinkStore))
    241241    , m_sessionID(PAL::SessionID::defaultSessionID())
     242    , m_playbackControlsManagerUpdateTimer(*this, &Page::playbackControlsManagerUpdateTimerFired)
    242243    , m_isUtilityPage(isUtilityPageChromeClient(chrome().client()))
    243244    , m_performanceMonitor(isUtilityPage() ? nullptr : std::make_unique<PerformanceMonitor>(*this))
    … …  
    14861487
    14871488    chrome().client().isPlayingMediaDidChange(state, sourceElementID);
     1489}
     1490
     1491void Page::schedulePlaybackControlsManagerUpdate()
     1492{
     1493    if (!m_playbackControlsManagerUpdateTimer.isActive())
     1494        m_playbackControlsManagerUpdateTimer.startOneShot(0_s);
     1495}
     1496
     1497void Page::playbackControlsManagerUpdateTimerFired()
     1498{
     1499    if (auto bestMediaElement = HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(MediaElementSession::PlaybackControlsPurpose::ControlsManager))
     1500        chrome().client().setUpPlaybackControlsManager(*bestMediaElement);
     1501    else
     1502        chrome().client().clearPlaybackControlsManager();
    14881503}
    14891504

  • trunk/Source/WebCore/page/Page.h

    r233552 r233560  
    570570    bool isAudioMuted() const { return m_mutedState & MediaProducer::AudioIsMuted; }
    571571    bool isMediaCaptureMuted() const { return m_mutedState & MediaProducer::CaptureDevicesAreMuted; };
     572    void schedulePlaybackControlsManagerUpdate();
    572573    WEBCORE_EXPORT void setMuted(MediaProducer::MutedStateFlags);
    573574    WEBCORE_EXPORT void stopMediaCapture();
    … …  
    674675
    675676    std::optional<std::pair<MediaCanStartListener&, Document&>> takeAnyMediaCanStartListener();
     677
     678    void playbackControlsManagerUpdateTimerFired();
    676679
    677680    Vector<Ref<PluginViewBase>> pluginViews();
    … …  
    850853
    851854    MediaProducer::MediaStateFlags m_mediaState { MediaProducer::IsNotPlaying };
    852    
     855
     856    Timer m_playbackControlsManagerUpdateTimer;
     857
    853858    bool m_allowsMediaDocumentInlinePlayback { false };
    854859    bool m_allowsPlaybackControlsForAutoplayingAudio { false };

  • trunk/Source/WebCore/testing/Internals.cpp

    r233549 r233560  
    37513751
    37523752#if ENABLE(VIDEO)
    3753 HTMLMediaElement* Internals::bestMediaElementForShowingPlaybackControlsManager(Internals::PlaybackControlsPurpose purpose)
     3753RefPtr<HTMLMediaElement> Internals::bestMediaElementForShowingPlaybackControlsManager(Internals::PlaybackControlsPurpose purpose)
    37543754{
    37553755    return HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(purpose);

  • trunk/Source/WebCore/testing/Internals.h

    r233549 r233560  
    705705#if ENABLE(VIDEO)
    706706    using PlaybackControlsPurpose = MediaElementSession::PlaybackControlsPurpose;
    707     HTMLMediaElement* bestMediaElementForShowingPlaybackControlsManager(PlaybackControlsPurpose);
     707    RefPtr<HTMLMediaElement> bestMediaElementForShowingPlaybackControlsManager(PlaybackControlsPurpose);
    708708
    709709    using MediaSessionState = PlatformMediaSession::State;
Note: See TracChangeset for help on using the changeset viewer.