Bug 187284 – Nullptr crash accessing Document in GenericEventQueue::dispatchOneEvent()

• Component: Media
• Status: RESOLVED FIXED

[Ryosuke Niwa]

e.g.

Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed:
0   WebCore                       	0x000000018aa74988 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>) + 76 (Document.h:1933)
1   WebCore                       	0x000000018aa74984 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>) + 72 (EventTarget.cpp:258)
2   WebCore                       	0x000000018aa707ec WebCore::EventTarget::fireEventListeners(WebCore::Event&) + 596 (EventTarget.cpp:231)
3   WebCore                       	0x000000018aa74920 WebCore::EventTarget::dispatchEvent(WebCore::Event&) + 116 (EventTarget.cpp:190)
4   WebCore                       	0x000000018aa77744 WebCore::GenericEventQueue::dispatchOneEvent() + 168 (GenericEventQueue.cpp:68)
5   WebCore                       	0x000000018ae5876c WebCore::TaskDispatcher<WebCore::Timer>::sharedTimerFired() + 208 (Function.h:56)
6   WebCore                       	0x0000000189e0c044 WebCore::ThreadTimers::sharedTimerFiredInternal() + 352 (ThreadTimers.cpp:118)
7   WebCore                       	0x0000000189e0bed0 WebCore::timerFired(__CFRunLoopTimer*, void*) + 28 (MainThreadSharedTimerCF.cpp:74)
8   CoreFoundation                	0x0000000181c84aa8 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 28 (CFRunLoop.c:1832)
9   CoreFoundation                	0x0000000181c8476c __CFRunLoopDoTimer + 864 (CFRunLoop.c:2415)
10  CoreFoundation                	0x0000000181c84010 __CFRunLoopDoTimers + 248 (CFRunLoop.c:2562)
11  CoreFoundation                	0x0000000181c81b60 __CFRunLoopRun + 2168 (CFRunLoop.c:0)
12  CoreFoundation                	0x0000000181ba1da8 CFRunLoopRunSpecific + 552 (CFRunLoop.c:3245)
13  Foundation                    	0x000000018261a464 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 304 (NSRunLoop.m:367)
14  Foundation                    	0x000000018266c0cc -[NSRunLoop(NSRunLoop) run] + 88 (NSRunLoop.m:389)
15  libxpc.dylib                  	0x000000018194db54 _xpc_objc_main + 516 (main.m:167)
16  libxpc.dylib                  	0x000000018194fc28 xpc_main + 180 (init.c:1476)
17  com.apple.WebKit.WebContent   	0x0000000102ef35ac main + 380 (XPCServiceMain.mm:148)
18  libdyld.dylib                 	0x0000000181635fc0 start + 4

<rdar://problem/38184148>

[Ryosuke Niwa]

Created attachment 344236 [details]
Fixes the crash

[Eric Carlson]

Comment on attachment 344236 [details]
Fixes the crash

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

As mentioned in irc, I think you also need to block HTMLMediaElement::prepareForLoad() and HTMLMediaElement::prepareToPlay() so setting video.src and calling video.load() don't cause media loading and related events.

It would be nice to have a test of a media element inside a template element, although that can be added later if you prefer.

> Source/WebCore/ChangeLog:56
> +        inside a stopped document, which should never is never correct and causes this crash down the line.

Nit: "which should never is never correct"

> Source/WebCore/Modules/mediasource/MediaSource.cpp:696
> +    ASSERT(scriptExecutionContext());
> +    if (!scriptExecutionContext()->activeDOMObjectsAreStopped()) {

Nit: I can't tell from the diff, but can you change this to an early return?

[Ryosuke Niwa]

(In reply to Eric Carlson from comment #2)
> Comment on attachment 344236 [details]
> Fixes the crash
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344236&action=review
> 
> As mentioned in irc, I think you also need to block
> HTMLMediaElement::prepareForLoad() and HTMLMediaElement::prepareToPlay() so
> setting video.src and calling video.load() don't cause media loading and
> related events.

Will do.

> It would be nice to have a test of a media element inside a template
> element, although that can be added later if you prefer.

We do have a test already: LayoutTests/media/track/track-display-before-controls-crash.html

> > Source/WebCore/ChangeLog:56
> > +        inside a stopped document, which should never is never correct and causes this crash down the line.
> 
> Nit: "which should never is never correct"

Fixed.

> > Source/WebCore/Modules/mediasource/MediaSource.cpp:696
> > +    ASSERT(scriptExecutionContext());
> > +    if (!scriptExecutionContext()->activeDOMObjectsAreStopped()) {
> 
> Nit: I can't tell from the diff, but can you change this to an early return?

That would mean I'd have to duplicate calls to m_activeSourceBuffers->remove(buffer); and m_sourceBuffers->remove(buffer); so I'd keep this as is.

[Ryosuke Niwa]

Created attachment 344243 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 344243 [details]
Patch for landing

Wait for EWS

[Ryosuke Niwa]

Committed r233496: <https://trac.webkit.org/changeset/233496>

[Dawei Fenton (:realdawei)]

(In reply to Ryosuke Niwa from comment #6)
> Committed r233496: <https://trac.webkit.org/changeset/233496>

Looks like this change is causing crashes on Sierra Debug WK1

Test: http/tests/security/contentSecurityPolicy/audio-redirect-allowed.html

Crash Log:
https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r233517%20(7131)/http/tests/security/contentSecurityPolicy/audio-redirect-allowed-crash-log.txt


Test: http/tests/security/contentSecurityPolicy/video-redirect-allowed.html
Crash Log:
https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r233517%20(7131)/http/tests/security/contentSecurityPolicy/video-redirect-allowed-crash-log.txt

[Dawei Fenton (:realdawei)]

(In reply to David Fenton (:realdawei) from comment #7)
> (In reply to Ryosuke Niwa from comment #6)
> > Committed r233496: <https://trac.webkit.org/changeset/233496>
> 
> Looks like this change is causing crashes on Sierra Debug WK1
> 
> Test: http/tests/security/contentSecurityPolicy/audio-redirect-allowed.html
> 
> Crash Log:
> https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/
> r233517%20(7131)/http/tests/security/contentSecurityPolicy/audio-redirect-
> allowed-crash-log.txt
> 
> 
> Test: http/tests/security/contentSecurityPolicy/video-redirect-allowed.html
> Crash Log:
> https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/
> r233517%20(7131)/http/tests/security/contentSecurityPolicy/video-redirect-
> allowed-crash-log.txt

Actually on Sierra Debug WK1 and WK2

[Ryosuke Niwa]

Weird. This assertion seems to fail only on Sierra :(

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #9)
> Weird. This assertion seems to fail only on Sierra :(

How I have a machine with macOS Sierra. Waiting for WebKit to build.

[Ryosuke Niwa]

Tracking the bug assertion failure in https://bugs.webkit.org/show_bug.cgi?id=187378.

[youenn fablet]

Some WebRTC tests are crashing according flakiness dashboard with the following trace:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000349512874 WebCore::TrackPrivateBase::~TrackPrivateBase() + 4 (TrackPrivateBase.h:57)
1   com.apple.JavaScriptCore      	0x000000034d33dfde WTF::dispatchFunctionsFromMainThread() + 318 (MainThread.cpp:132)
2   com.apple.JavaScriptCore      	0x000000034d33ecdf WTF::timerFired(__CFRunLoopTimer*, void*) + 31 (MainThreadMac.mm:110)
3   com.apple.CoreFoundation      	0x00007fff41f9b704 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
4   com.apple.CoreFoundation      	0x00007fff41f9b384 __CFRunLoopDoTimer + 1108
5   com.apple.CoreFoundation      	0x00007fff41f9ae7a __CFRunLoopDoTimers + 346
6   com.apple.CoreFoundation      	0x00007fff41f9261b __CFRunLoopRun + 2427
7   com.apple.CoreFoundation      	0x00007fff41f91a07 CFRunLoopRunSpecific + 487
8   com.apple.HIToolbox           	0x00007fff4126fd96 RunCurrentEventLoopInMode + 286
9   com.apple.HIToolbox           	0x00007fff4126fb06 ReceiveNextEventCommon + 613
10  com.apple.HIToolbox           	0x00007fff4126f884 _BlockUntilNextEventMatchingListInModeWithFilter + 64
11  com.apple.AppKit              	0x00007fff3f522a73 _DPSNextEvent + 2085
12  com.apple.AppKit              	0x00007fff3fcb8e34 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 3044
13  com.apple.AppKit              	0x00007fff3f517885 -[NSApplication run] + 764
14  com.apple.AppKit              	0x00007fff3f4e6a72 NSApplicationMain + 804
15  libxpc.dylib                  	0x00007fff6a690f57 _xpc_objc_main + 580
16  libxpc.dylib                  	0x00007fff6a68fbaa xpc_main + 417
17  com.apple.WebKit.WebContent   	0x0000000109fd56b5 main + 485
18  libdyld.dylib                 	0x00007fff6a336015 start + 1

It started to happen very soon after this patch.

[Ryosuke Niwa]

(In reply to youenn fablet from comment #12)
> Some WebRTC tests are crashing according flakiness dashboard with the
> following trace:

Is it still happening after https://bugs.webkit.org/show_bug.cgi?id=187378 ?

[youenn fablet]

https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r233795%20(4095)/webrtc/video-addTransceiver-crash-log.txt is showing this crash trace.

https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r233853%20(5654)/webrtc/video-with-receiver-crash-log.txt is also a recent crash.

webrtc/video-addTrack.html and webrtc/video-with-receiver.html might allow reproing the crash.

[Chris Dumez]

(In reply to youenn fablet from comment #14)
> https://build.webkit.org/results/
> Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r233795%20(4095)/webrtc/video-
> addTransceiver-crash-log.txt is showing this crash trace.
> 
> https://build.webkit.org/results/
> Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r233853%20(5654)/webrtc/
> video-with-receiver-crash-log.txt is also a recent crash.
> 
> webrtc/video-addTrack.html and webrtc/video-with-receiver.html might allow
> reproing the crash.

Youenn's latest crash traces are from r233853. The fix for Bug 187378 went into r233571 so I believe we still have a regression from r233496.

[Chris Dumez]

Reverted r233496 and r233571 for reason:

Likely cause of <rdar://problem/42160890> and <rdar://problem/42329658> as ActiveDOMObjects can now be constructed / destroyed while we are iterating over them.

Committed r234177: <https://trac.webkit.org/changeset/234177>

[Chris Dumez]

Addressing crashes caused by ActiveDOMObject creation/destruction while iterating over them via Bug 188094 & Bug 188025.

What will remain to re-land this patch will be to fix the following use-after free: rdar://problem/42558823.

[Chris Dumez]

Created attachment 346060 [details]
Patch

[Chris Dumez]

Re-landing Ryosuke's patch now that all known issues should have been fixed via dependency bugs.

[WebKit Commit Bot]

Comment on attachment 346060 [details]
Patch

Clearing flags on attachment: 346060

Committed r234374: <https://trac.webkit.org/changeset/234374>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.