Bug 187316 – Youtube video pages crash after a couple of minutes

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ryosuke Niwa]

We hit a crash like this:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fff3d076004 WebCore::Document::updateLayout() + 116
1   com.apple.WebCore             	0x00007fff3d13297e WebCore::RenderView::hitTest(WebCore::HitTestRequest const&, WebCore::HitTestLocation const&, WebCore::HitTestResult&) + 46
2   com.apple.WebCore             	0x00007fff3df48b26 WebCore::MediaElementSession::updateIsMainContent() const + 518
3   com.apple.WebCore             	0x00007fff3df48367 WebCore::MediaElementSession::playbackPermitted() const + 199
4   com.apple.WebCore             	0x00007fff3df4931a WebCore::MediaElementSession::canShowControlsManager(WebCore::MediaElementSession::PlaybackControlsPurpose) const + 794
5   com.apple.WebCore             	0x00007fff3decb910 WebCore::HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(WebCore::MediaElementSession::PlaybackControlsPurpose) + 208
6   com.apple.WebCore             	0x00007fff3dedb242 WebCore::HTMLMediaElement::stopWithoutDestroyingMediaPlayer() + 210
7   com.apple.WebCore             	0x00007fff3d115ee8 WebCore::HTMLMediaElement::stop() + 120
8   com.apple.WebCore             	0x00007fff3d03c0de WebCore::ScriptExecutionContext::stopActiveDOMObjects() + 494
9   com.apple.WebCore             	0x00007fff3d03b93b WebCore::Document::prepareForDestruction() + 763
10  com.apple.WebCore             	0x00007fff3e0f1915 WebCore::Frame::setView(WTF::RefPtr<WebCore::FrameView, WTF::DumbPtrTraits<WebCore::FrameView> >&&) + 245
11  com.apple.WebCore             	0x00007fff3d0899a4 WebCore::FrameLoader::detachFromParent() + 436
12  com.apple.WebCore             	0x00007fff3cffe59f WebCore::FrameLoader::detachChildren() + 351
13  com.apple.WebCore             	0x00007fff3d089885 WebCore::FrameLoader::detachFromParent() + 149
14  com.apple.WebCore             	0x00007fff3d0cdcc4 WebCore::FrameLoader::frameDetached() + 196
15  com.apple.WebCore             	0x00007fff3d0cdbb3 WebCore::HTMLFrameOwnerElement::disconnectContentFrame() + 35
16  com.apple.WebCore             	0x00007fff3dd2bdc8 WebCore::disconnectSubframes(WebCore::ContainerNode&, WebCore::SubframeDisconnectPolicy) + 216
17  com.apple.WebCore             	0x00007fff3dd28659 WebCore::ContainerNode::removeChild(WebCore::Node&) + 217
18  com.apple.WebCore             	0x00007fff3dd94bdb WebCore::Node::removeChild(WebCore::Node&) + 43
19  com.apple.WebCore             	0x00007fff3d08377e WebCore::jsNodePrototypeFunctionRemoveChild(JSC::ExecState*) + 238


<rdar://problem/41538778>

[Ryosuke Niwa]

Created attachment 344266 [details]
Fixes the crash

[Ryosuke Niwa]

Created attachment 344267 [details]
Fixes the crash

[EWS Watchlist]

Attachment 344267 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.h:707:  'bestMediaElementForShowingPlaybackControlsManager' is incorrectly named. It should be named 'protector' or 'protectedPlaybackControlsPurpose'.  [readability/naming/protected] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 344267 [details]
Fixes the crash

Attachment 344267 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8436025

New failing tests:
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344284 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Antti Koivisto]

Comment on attachment 344267 [details]
Fixes the crash

View in context: https://bugs.webkit.org/attachment.cgi?id=344267&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:7891
> +void HTMLMediaElement::scheduleUpdatePlaybackControlsManager()

schedulePlaybackControlsManagerUpdate would read better (and not be randomly different from the Page version).

[Eric Carlson]

Comment on attachment 344267 [details]
Fixes the crash

View in context: https://bugs.webkit.org/attachment.cgi?id=344267&action=review

> Source/WebCore/page/Page.h:856
> +    Timer m_playbackControlsManagerUpdateTimer;

Nit: If you use a std::unique_ptr<DeferrableOneShotTimer>, the timer can be allocated lazily and you can use a lambda instead of adding a method for the fire proc (see PlatformMediaSessionManager::scheduleUpdateSessionState for an example).

[Ryosuke Niwa]

(In reply to Eric Carlson from comment #7)
> Comment on attachment 344267 [details]
> Fixes the crash
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344267&action=review
> 
> > Source/WebCore/page/Page.h:856
> > +    Timer m_playbackControlsManagerUpdateTimer;
> 
> Nit: If you use a std::unique_ptr<DeferrableOneShotTimer>, the timer can be
> allocated lazily and you can use a lambda instead of adding a method for the
> fire proc (see PlatformMediaSessionManager::scheduleUpdateSessionState for
> an example).

That's a good idea. Will do that.

[Ryosuke Niwa]

Committed r233539: <https://trac.webkit.org/changeset/233539>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 187368

[Ryosuke Niwa]

Created attachment 344382 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 344382 [details]
Patch for landing

Wait for EWS

[EWS Watchlist]

Attachment 344382 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.h:707:  'bestMediaElementForShowingPlaybackControlsManager' is incorrectly named. It should be named 'protector' or 'protectedPlaybackControlsPurpose'.  [readability/naming/protected] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 344382 [details]
Patch for landing

Actually, it's odd to use DeferrableOneShotTimer here since we never defer this timer. We can just use std::optional<Timer> instead. There is negligible cost to creating a timer per page anyway so I'd just go with the original patch.

[Ryosuke Niwa]

Committed r233560: <https://trac.webkit.org/changeset/233560>