Bug 185288

Summary: Release assert in ScriptController::canExecuteScripts via HTMLMediaElement::~HTMLMediaElement()
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: MediaAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, jer.noble, koivisto, sabouhallawa, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=185284
Bug Depends on:    
Bug Blocks: 203600    
Attachments:
Description Flags
Speculative fix jer.noble: review+

Description Ryosuke Niwa 2018-05-03 20:09:13 PDT
e.g.

Thread 0 Crashed:
0   WebCore                       	0x000000018af6e574 WebCore::ScriptController::canExecuteScripts(WebCore::ReasonForCallingCanExecuteScripts) + 552 (Source/WebCore/bindings/js/ScriptController.cpp:672)
1   WebCore                       	0x000000018af6e380 WebCore::ScriptController::canExecuteScripts(WebCore::ReasonForCallingCanExecuteScripts) + 52 (Source/WebCore/bindings/js/ScriptController.cpp:672)
2   WebCore                       	0x000000018b9a0b0c WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) + 316 (Source/WebCore/bindings/js/JSEventListener.cpp:113)
3   WebCore                       	0x000000018bbdd674 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>) + 760 (Source/WebCore/dom/EventTarget.cpp:289)
4   WebCore                       	0x000000018bbd922c WebCore::EventTarget::fireEventListeners(WebCore::Event&) + 596 (Source/WebCore/dom/EventTarget.cpp:231)
5   WebCore                       	0x000000018bf12140 WebCore::DOMWindow::dispatchEvent(WebCore::Event&, WebCore::EventTarget*) + 284 (Source/WebCore/page/DOMWindow.cpp:2053)
6   WebCore                       	0x000000018aff6b78 WebCore::DOMWindow::dispatchLoadEvent() + 160 (Source/WebCore/page/DOMWindow.cpp:2005)
7   WebCore                       	0x000000018afb3218 WebCore::Document::implicitClose() + 440 (Source/WebCore/dom/Document.cpp:4326)
8   WebCore                       	0x000000018afb282c WebCore::FrameLoader::checkCompleted() + 476 (Source/WebCore/loader/FrameLoader.cpp:910)
9   WebCore                       	0x000000018bef2e80 WebCore::CachedResourceLoader::loadDone(bool) + 84 (Source/WebCore/loader/cache/CachedResourceLoader.cpp:1287)
10  WebCore                       	0x000000018afe401c WebCore::SubresourceLoader::didCancel(WebCore::ResourceError const&) + 128 (Source/WebCore/loader/SubresourceLoader.cpp:699)
11  WebCore                       	0x000000018afe39d4 WebCore::ResourceLoader::cancel(WebCore::ResourceError const&) + 492 (Source/WebCore/loader/ResourceLoader.cpp:642)
12  WebCore                       	0x000000018afe3744 WebCore::ResourceLoader::cancel() + 64 (Source/WebCore/loader/ResourceLoader.cpp:598)
13  WebCore                       	0x000000018beec9b4 WebCore::CachedResource::removeClient(WebCore::CachedResourceClient&) + 264 (Source/WebCore/loader/cache/CachedResource.cpp:573)
14  WebCore                       	0x000000018beb0fec WebCore::MediaResource::~MediaResource() + 64 (Source/WebCore/loader/MediaResourceLoader.cpp:132)
15  WebCore                       	0x000000018beb109c WebCore::MediaResource::~MediaResource() + 12 (Source/WebCore/loader/MediaResourceLoader.cpp:122)
...
25  WebCore                       	0x000000018b165774 WebCore::MediaPlayerPrivateAVFoundationObjC::~MediaPlayerPrivateAVFoundationObjC() + 336 (Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:539)
26  WebCore                       	0x000000018b166274 WebCore::MediaPlayerPrivateAVFoundationObjC::~MediaPlayerPrivateAVFoundationObjC() + 12 (Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:521)
27  WebCore                       	0x000000018c04bd54 WebCore::MediaPlayer::~MediaPlayer() + 244 (/BuildRoot/Applications/Xcode.app/Contents/Developer/Toolchains/iOS11.4.xctoolchain/usr/include/c++/v1/memory:2239)
28  WebCore                       	0x000000018c04bdbc WebCore::MediaPlayer::~MediaPlayer() + 12 (Source/WebCore/platform/graphics/MediaPlayer.cpp:376)
29  WebCore                       	0x000000018bd32a68 WebCore::HTMLMediaElement::~HTMLMediaElement() + 1316 (/BuildRoot/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS11.4.Internal.sdk/usr/local/include/wtf/RefCounted.h:145)
30  WebCore                       	0x000000018bd9192c WebCore::HTMLVideoElement::~HTMLVideoElement() + 176 (Source/WebCore/html/HTMLVideoElement.h:38)
Comment 1 Ryosuke Niwa 2018-05-03 20:19:57 PDT
<rdar://problem/39738580>
Comment 2 Ryosuke Niwa 2018-05-03 20:29:38 PDT
Created attachment 339514 [details]
Speculative fix
Comment 3 Said Abou-Hallawa 2018-05-04 09:32:54 PDT
Comment on attachment 339514 [details]
Speculative fix

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

> Source/WebCore/ChangeLog:12
> +        In long term, ResourceLoader::cancel should never fire load event synchronously: webkit.org/b/185284.

This is confusing. How does cancelling loading the resource fire the load event? Should not it fire the abort event?

> Source/WebCore/html/HTMLMediaElement.cpp:584
> +    return !!s_destructorCount;

I think there is no need for !!.

> Source/WebCore/html/HTMLMediaElement.cpp:589
> +    HTMLMediaElementDestructorScope() { ++s_destructorCount; }

Do we expect s_destructorCount to be more than 1? Should not s_destructorCount be a bool, e.g. s_inHTMLMediaElementDestructor? Will the destructor of HTMLMediaElement call the destructor of itself or other HTMLMediaElements? I think neither should ever happen.
Comment 4 Ryosuke Niwa 2018-05-04 13:54:30 PDT
(In reply to Said Abou-Hallawa from comment #3)
> Comment on attachment 339514 [details]
> Speculative fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339514&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        In long term, ResourceLoader::cancel should never fire load event synchronously: webkit.org/b/185284.
> 
> This is confusing. How does cancelling loading the resource fire the load
> event? Should not it fire the abort event?

When the last resource that had been blocking document's load event gets canceled, we fire load event on the document.

> > Source/WebCore/html/HTMLMediaElement.cpp:589
> > +    HTMLMediaElementDestructorScope() { ++s_destructorCount; }
> 
> Do we expect s_destructorCount to be more than 1? Should not
> s_destructorCount be a bool, e.g. s_inHTMLMediaElementDestructor? Will the
> destructor of HTMLMediaElement call the destructor of itself or other
> HTMLMediaElements? I think neither should ever happen.

Hopefully that's the case but the media code is so complicated that such a guarantee is not self-evident. Because we're trying to create a very safe fix here, I'm opt'ing to use the safest approach, which is to use a counter.
Comment 5 Jer Noble 2018-05-04 15:13:45 PDT
Comment on attachment 339514 [details]
Speculative fix

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

r=me with nit.

> Source/WebCore/loader/FrameLoader.cpp:811
> +    if (HTMLMediaElement::isRunningDestructor()) {
> +        scheduleCheckCompleted();

We could really use an ASSERT_NOT_REACHED() here so that it crashes in debug builds.
Comment 6 Ryosuke Niwa 2018-05-04 15:26:17 PDT
(In reply to Jer Noble from comment #5)
> Comment on attachment 339514 [details]
> Speculative fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339514&action=review
> 
> r=me with nit.
> 
> > Source/WebCore/loader/FrameLoader.cpp:811
> > +    if (HTMLMediaElement::isRunningDestructor()) {
> > +        scheduleCheckCompleted();
> 
> We could really use an ASSERT_NOT_REACHED() here so that it crashes in debug
> builds.

Sure, will do.
Comment 7 Ryosuke Niwa 2018-05-04 16:58:06 PDT
Committed r231392: <https://trac.webkit.org/changeset/231392>