Bug 209655

Summary: [StressGC] ASSERTION FAILED: m_wrapper under WebCore::MainThreadGenericEventQueue::dispatchOneEvent
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, jer.noble, kondapallykalyan, philipj, rniwa, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2020-03-27 09:12:14 PDT 
[StressGC] ASSERTION FAILED: m_wrapper under WebCore::MainThreadGenericEventQueue::dispatchOneEvent:
ASSERTION FAILED: m_wrapper
./bindings/js/JSEventListener.h(123) : JSC::JSObject *WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext &) const
1   0x1091b70b9 WTFCrash
2   0x11ee6388b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x12101e9f8 WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext&) const
4   0x12101dd1b WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)
5   0x12166b87c WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase)
6   0x121667b12 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase)
7   0x12166b45d WebCore::EventTarget::dispatchEvent(WebCore::Event&)
8   0x1216865f0 WebCore::MainThreadGenericEventQueue::dispatchOneEvent()
9   0x12168b8a1 decltype(*(std::__1::forward<WebCore::MainThreadGenericEventQueue*&>(fp0)).*fp()) std::__1::__invoke<void (WebCore::MainThreadGenericEventQueue::*&)(), WebCore::MainThreadGenericEventQueue*&, void>(void (WebCore::MainThreadGenericEventQueue::*&&&)(), WebCore::MainThreadGenericEventQueue*&&&)
10  0x12168b820 std::__1::__bind_return<void (WebCore::MainThreadGenericEventQueue::*)(), std::__1::tuple<WebCore::MainThreadGenericEventQueue*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::MainThreadGenericEventQueue::*)(), std::__1::tuple<WebCore::MainThreadGenericEventQueue*>, std::__1::tuple<> >::value>::type std::__1::__apply_functor<void (WebCore::MainThreadGenericEventQueue::*)(), std::__1::tuple<WebCore::MainThreadGenericEventQueue*>, 0ul, std::__1::tuple<> >(void (WebCore::MainThreadGenericEventQueue::*&)(), std::__1::tuple<WebCore::MainThreadGenericEventQueue*>&, std::__1::__tuple_indices<0ul>, std::__1::tuple<>&&)
11  0x12168b7cc std::__1::__bind_return<void (WebCore::MainThreadGenericEventQueue::*)(), std::__1::tuple<WebCore::MainThreadGenericEventQueue*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::MainThreadGenericEventQueue::*)(), std::__1::tuple<WebCore::MainThreadGenericEventQueue*>, std::__1::tuple<> >::value>::type std::__1::__bind<void (WebCore::MainThreadGenericEventQueue::*)(), WebCore::MainThreadGenericEventQueue*>::operator()<>()
12  0x12168b769 WTF::Detail::CallableWrapper<std::__1::__bind<void (WebCore::MainThreadGenericEventQueue::*)(), WebCore::MainThreadGenericEventQueue*>, void>::call()
13  0x11ee710fa WTF::Function<void ()>::operator()() const
14  0x11f133a20 WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'()::operator()() const
15  0x11f133869 WTF::Detail::CallableWrapper<WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'(), void>::call()
16  0x11ee710fa WTF::Function<void ()>::operator()() const
17  0x12242071e WebCore::TaskDispatcher<WebCore::Timer>::dispatchOneTask()
18  0x1224203c5 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimerFired()
19  0x122424c61 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_1::operator()() const
20  0x122424c29 WTF::Detail::CallableWrapper<WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_1, void>::call()
21  0x11ee710fa WTF::Function<void ()>::operator()() const
22  0x11ef29ee9 WebCore::Timer::fired()
23  0x1224682ea WebCore::ThreadTimers::sharedTimerFiredInternal()
24  0x12246fec1 WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0::operator()() const
25  0x12246fe79 WTF::Detail::CallableWrapper<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0, void>::call()
26  0x11ee710fa WTF::Function<void ()>::operator()() const
27  0x122435ff7 WebCore::MainThreadSharedTimer::fired()
28  0x1224ca486 WebCore::timerFired(__CFRunLoopTimer*, void*)
29  0x7fff4f3805d5 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
30  0x7fff4f380181 __CFRunLoopDoTimer
31  0x7fff4f37fcba __CFRunLoopDoTimers
Comment 1 Chris Dumez 2020-03-27 09:12:21 PDT 
<rdar://problem/60541442>
Comment 2 Chris Dumez 2020-03-27 09:15:20 PDT 
Created attachment 394721 [details]
Patch
Comment 3 Geoffrey Garen 2020-03-27 09:19:32 PDT 
Comment on attachment 394721 [details]
Patch

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

r=me

> Source/WebCore/html/track/VideoTrackList.h:41
> +        list->suspendIfNeeded();

Can these suspend calls happen in the constructors instead?
Comment 4 Chris Dumez 2020-03-27 09:23:42 PDT 
(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 394721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394721&action=review
> 
> r=me
> 
> > Source/WebCore/html/track/VideoTrackList.h:41
> > +        list->suspendIfNeeded();
> 
> Can these suspend calls happen in the constructors instead?

I am trying to remember why but I am 100% sure that what you are suggesting is bad practice because it does not work in all cases and leads to assertions. The good practice is definitely to do it in the factory like I did. I definitely had to write patches to move the suspendIfNeeded() calls to the constructor call site to address crashes in the past.
Comment 5 Chris Dumez 2020-03-27 09:25:24 PDT 
(In reply to Chris Dumez from comment #4)
> (In reply to Geoffrey Garen from comment #3)
> > Comment on attachment 394721 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=394721&action=review
> > 
> > r=me
> > 
> > > Source/WebCore/html/track/VideoTrackList.h:41
> > > +        list->suspendIfNeeded();
> > 
> > Can these suspend calls happen in the constructors instead?
> 
> I am trying to remember why but I am 100% sure that what you are suggesting
> is bad practice because it does not work in all cases and leads to
> assertions. The good practice is definitely to do it in the factory like I
> did. I definitely had to write patches to move the suspendIfNeeded() calls
> to the constructor call site to address crashes in the past.

Ok, found an example:
http://trac.webkit.org/r252497

From my changelog:
"""
    Call suspendIfNeeded() in the factory and not in the constructor. It is never safe to call
    suspendIfNeeded() from inside the constructor because it may call the suspend() method, which
    may ref |this|.
"""
Comment 6 Geoffrey Garen 2020-03-27 09:33:46 PDT 
Okeedokee.
Comment 7 EWS 2020-03-27 09:53:45 PDT 
Committed r259122: <https://trac.webkit.org/changeset/259122>

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