Bug 187316

Summary: Youtube video pages crash after a couple of minutes
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: MediaAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, eric.carlson, ews-watchlist, jeremyj-wk, jer.noble, jonlee, koivisto, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 187368    
Bug Blocks:    
Attachments:
Description Flags
Fixes the crash
none
Fixes the crash
none
Archive of layout-test-results from ews206 for win-future
none
Patch for landing none

Description Ryosuke Niwa 2018-07-04 00:50:31 PDT 
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>
Comment 1 Ryosuke Niwa 2018-07-04 01:08:42 PDT 
Created attachment 344266 [details]
Fixes the crash
Comment 2 Ryosuke Niwa 2018-07-04 01:09:36 PDT 
Created attachment 344267 [details]
Fixes the crash
Comment 3 EWS Watchlist 2018-07-04 01:12:09 PDT 
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.
Comment 4 EWS Watchlist 2018-07-04 07:06:36 PDT 
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
Comment 5 EWS Watchlist 2018-07-04 07:06:48 PDT 
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
Comment 6 Antti Koivisto 2018-07-04 09:27:54 PDT 
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).
Comment 7 Eric Carlson 2018-07-05 07:15:22 PDT 
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).
Comment 8 Ryosuke Niwa 2018-07-05 13:33:42 PDT 
(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.
Comment 9 Ryosuke Niwa 2018-07-05 14:02:50 PDT 
Committed r233539: <https://trac.webkit.org/changeset/233539>
Comment 10 WebKit Commit Bot 2018-07-05 16:08:49 PDT 
Re-opened since this is blocked by bug 187368
Comment 11 Ryosuke Niwa 2018-07-05 17:06:33 PDT 
Created attachment 344382 [details]
Patch for landing
Comment 12 Ryosuke Niwa 2018-07-05 17:10:13 PDT 
Comment on attachment 344382 [details]
Patch for landing

Wait for EWS
Comment 13 EWS Watchlist 2018-07-05 17:12:29 PDT 
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.
Comment 14 Ryosuke Niwa 2018-07-05 19:30:46 PDT 
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.
Comment 15 Ryosuke Niwa 2018-07-05 20:20:22 PDT 
Committed r233560: <https://trac.webkit.org/changeset/233560>