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()
<rdar://problem/82302915>
Updated expectations: https://trac.webkit.org/changeset/281509/webkit
Created attachment 436652 [details] Patch
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)?
(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.
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.
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.
Created attachment 436673 [details] Patch for landing
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].