Bug 229462 – [ Catalina EWS ] media/track/track-disabled-addcue.html is flaky crashing

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

media/track/track-disabled-addcue.html

Is flaky crashing on macOS-Catalina-Debug-WK1-Tests-EWS

The flaky crash is not showing up on the open source: https://results.webkit.org/?suite=layout-tests&test=media/track/track-disabled-addcue.html

It seems to have started being flaky at this build https://ews-build.webkit.org/#/builders/56/builds/10449

Crash log: 

No crash log found for DumpRenderTree:5432.

stdout:

stderr:
ASSERTION FAILED: m_wrapper
./bindings/js/JSEventListener.h(128) : JSC::JSObject *WebCore::JSEventListener::ensureJSFunction(WebCore::ScriptExecutionContext &) const
1   0x10853d2f9 WTFCrash
2   0x128abea1b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x12b6ca62f WebCore::JSEventListener::ensureJSFunction(WebCore::ScriptExecutionContext&) const
4   0x12b6c9994 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)
5   0x12beb0357 WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::RawPtrTraits<WebCore::RegisteredEventListener>, WTF::DefaultRefDerefTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase)
6   0x12beafdb4 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase)
7   0x12be82d5a WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const
8   0x12be83c25 WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&)
9   0x12be834c2 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&)
10  0x12bf37b3d WebCore::Node::dispatchEvent(WebCore::Event&)
11  0x12c43e217 WebCore::HTMLTrackElement::didCompleteLoad(WebCore::HTMLTrackElement::LoadStatus)
12  0x12c6acc45 WebCore::LoadableTextTrack::loadTimerFired()
13  0x12c6c5ac7 decltype(*(std::__1::forward<WebCore::LoadableTextTrack*&>(fp0)).*fp()) std::__1::__invoke<void (WebCore::LoadableTextTrack::*&)(), WebCore::LoadableTextTrack*&, void>(void (WebCore::LoadableTextTrack::*&)(), WebCore::LoadableTextTrack*&)
14  0x12c6c5a40 std::__1::__bind_return<void (WebCore::LoadableTextTrack::*)(), std::__1::tuple<WebCore::LoadableTextTrack*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::LoadableTextTrack::*)(), std::__1::tuple<WebCore::LoadableTextTrack*>, std::__1::tuple<> >::value>::type std::__1::__apply_functor<void (WebCore::LoadableTextTrack::*)(), std::__1::tuple<WebCore::LoadableTextTrack*>, 0ul, std::__1::tuple<> >(void (WebCore::LoadableTextTrack::*&)(), std::__1::tuple<WebCore::LoadableTextTrack*>&, std::__1::__tuple_indices<0ul>, std::__1::tuple<>&&)
15  0x12c6c59f9 std::__1::__bind_return<void (WebCore::LoadableTextTrack::*)(), std::__1::tuple<WebCore::LoadableTextTrack*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::LoadableTextTrack::*)(), std::__1::tuple<WebCore::LoadableTextTrack*>, std::__1::tuple<> >::value>::type std::__1::__bind<void (WebCore::LoadableTextTrack::*&)(), WebCore::LoadableTextTrack*>::operator()<>()
16  0x12c6c597e WTF::Detail::CallableWrapper<std::__1::__bind<void (WebCore::LoadableTextTrack::*&)(), WebCore::LoadableTextTrack*>, void>::call()
17  0x128ad4312 WTF::Function<void ()>::operator()() const
18  0x128b1462e WebCore::Timer::fired()
19  0x12cfdcee4 WebCore::ThreadTimers::sharedTimerFiredInternal()
20  0x12cfea031 WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0::operator()() const
21  0x12cfe9fbe WTF::Detail::CallableWrapper<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0, void>::call()
22  0x128ad4312 WTF::Function<void ()>::operator()() const
23  0x12cf9489b WebCore::MainThreadSharedTimer::fired()
24  0x12d063c76 WebCore::timerFired(__CFRunLoopTimer*, void*)
25  0x7fff378a6468 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
26  0x7fff378a5fce __CFRunLoopDoTimer
27  0x7fff378a5ab9 __CFRunLoopDoTimers
28  0x7fff3788a70d __CFRunLoopRun
29  0x7fff37889953 CFRunLoopRunSpecific
30  0x100cdc1d8 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)
31  0x100cdb47a runTestingServerLoop()

[Radar WebKit Bug Importer]

<rdar://problem/82302915>

Updated expectations: https://trac.webkit.org/changeset/281509/webkit

[Eric Carlson]

Created attachment 436652 [details]
Patch

[Darin Adler]

Comment on attachment 436652 [details]
Patch

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

> Source/WTF/wtf/SetForScope.h:64
> +        , m_originalValue(restoreValue)

Should this be std::forward<U>(restoreValue)?

[Eric Carlson]

(In reply to Darin Adler from comment #4)
> Comment on attachment 436652 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436652&action=review
> 
> > Source/WTF/wtf/SetForScope.h:64
> > +        , m_originalValue(restoreValue)
> 
> Should this be std::forward<U>(restoreValue)?

Good point, thanks.

[Darin Adler]

Comment on attachment 436652 [details]
Patch

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

Looks good assuming we have enough test coverage. I see various interesting cases here in the code we are refactoring and altering subtly and not really sure we have tests for all of them

>>> Source/WTF/wtf/SetForScope.h:64
>>> +        , m_originalValue(restoreValue)
>> 
>> Should this be std::forward<U>(restoreValue)?
> 
> Good point, thanks.

Was thinking further and, for flexibility, I also think that we also could use two different types for newValue and restoreValue. The stored values will still be of type T, but the arguments could be two different types convertible to T.

    template<typename U, typename V>
    SetForScope(T& scopedVariable, U&& newValue, V&& restoreValue)

> Source/WebCore/html/HTMLTrackElement.cpp:196
> +        URL url = getNonEmptyURLAttribute(srcAttr);

I find the name "url" distasteful, and try to dodge it, so I would have named this "trackURL".

> Source/WebCore/html/HTMLTrackElement.cpp:202
> +        if (url.isEmpty() || !canLoadURL(url)) {

Do we need an empty check here? Maybe canLoadURL is guaranteed to return false for empty?

> Source/WebCore/html/HTMLTrackElement.cpp:-209
>  
> -    track().scheduleLoad(url);

Stray blank line here we could delete.

[Eric Carlson]

Comment on attachment 436652 [details]
Patch

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

>>>> Source/WTF/wtf/SetForScope.h:64
>>>> +        , m_originalValue(restoreValue)
>>> 
>>> Should this be std::forward<U>(restoreValue)?
>> 
>> Good point, thanks.
> 
> Was thinking further and, for flexibility, I also think that we also could use two different types for newValue and restoreValue. The stored values will still be of type T, but the arguments could be two different types convertible to T.
> 
>     template<typename U, typename V>
>     SetForScope(T& scopedVariable, U&& newValue, V&& restoreValue)

Great idea, changed. I also renamed `m_originalValue` to `m_valueToRestore` for clarity

>> Source/WebCore/html/HTMLTrackElement.cpp:196
>> +        URL url = getNonEmptyURLAttribute(srcAttr);
> 
> I find the name "url" distasteful, and try to dodge it, so I would have named this "trackURL".

Changed.

>> Source/WebCore/html/HTMLTrackElement.cpp:202
>> +        if (url.isEmpty() || !canLoadURL(url)) {
> 
> Do we need an empty check here? Maybe canLoadURL is guaranteed to return false for empty?

canLoadURL does return false for an empty url, so this check isn't necessary.

>> Source/WebCore/html/HTMLTrackElement.cpp:-209
>> -    track().scheduleLoad(url);
> 
> Stray blank line here we could delete.

Removed.

[Eric Carlson]

Created attachment 436673 [details]
Patch for landing

[EWS]

Committed r281727 (241069@main): <https://commits.webkit.org/241069@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436673 [details].