Bug 187284

Created attachment 344236 [details]
Fixes the crash

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?

(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.

Created attachment 344243 [details]
Patch for landing

Comment on attachment 344243 [details]
Patch for landing

Wait for EWS

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

(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

(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

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

(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.

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

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.

(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 ?

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.

(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.

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>

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.

Created attachment 346060 [details]
Patch

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

Comment on attachment 346060 [details]
Patch

Clearing flags on attachment: 346060

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

All reviewed patches have been landed.  Closing bug.