Bug 208642 – REGRESSION: (r257905) [ Mac wk2 Debug ] ASSERTION FAILED: !m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction

• Component: Media
• Status: RESOLVED FIXED

[Jason Lawrence]

media/modern-media-controls/tracks-support/tracks-support-no-tracks.html

Description:
This test is flaky crashing on Mac wk2 Debug. It appears to have a regression point of r257905.

History:
https://results.webkit.org/?suite=layout-tests&test=media%2Fmodern-media-controls%2Ftracks-support%2Ftracks-support-no-tracks.html&style=debug&limit=50000

Crash log attached;
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000821882b2e WTFCrash + 14 (Assertions.cpp:305)
1   com.apple.WebCore             	0x0000000804d70b5b WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x000000080706142a WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext&) const + 426 (JSEventListener.h:111)
3   com.apple.WebCore             	0x000000080706080b WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) + 219 (JSEventListener.cpp:113)
4   com.apple.WebCore             	0x000000080771857c WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase) + 956 (EventTarget.cpp:315)
5   com.apple.WebCore             	0x00000008077147f2 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) + 354 (EventTarget.cpp:252)
6   com.apple.WebCore             	0x000000080771815d WebCore::EventTarget::dispatchEvent(WebCore::Event&) + 333 (EventTarget.cpp:211)
7   com.apple.WebCore             	0x0000000807737310 WebCore::MainThreadGenericEventQueue::dispatchOneEvent() + 608 (GenericEventQueue.cpp:73)
8   com.apple.WebCore             	0x000000080773e011 decltype(*(std::__1::forward<WebCore::MainThreadGenericEventQueue*&>(fp0)).*fp()) std::__1::__invoke<void (WebCore::MainThreadGenericEventQueue::*&)(), WebCore::MainThreadGenericEventQueue*&, void>(void (WebCore::MainThreadGenericEventQueue::*&)(), WebCore::MainThreadGenericEventQueue*&) + 113 (type_traits:4302)
9   com.apple.WebCore             	0x000000080773df70 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<>&&) + 64 (functional:2644)
10  com.apple.WebCore             	0x000000080773df1c 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()<>() + 60 (functional:2677)
11  com.apple.WebCore             	0x000000080773deb9 WTF::Detail::CallableWrapper<std::__1::__bind<void (WebCore::MainThreadGenericEventQueue::*)(), WebCore::MainThreadGenericEventQueue*>, void>::call() + 25 (Function.h:52)
12  com.apple.WebCore             	0x0000000804d8353a WTF::Function<void ()>::operator()() const + 138 (Function.h:84)
13  com.apple.WebCore             	0x0000000805061aeb WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'()::operator()() const + 171 (GenericTaskQueue.h:108)
14  com.apple.WebCore             	0x0000000805061939 WTF::Detail::CallableWrapper<WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'(), void>::call() + 25 (Function.h:52)
15  com.apple.WebCore             	0x0000000804d8353a WTF::Function<void ()>::operator()() const + 138 (Function.h:84)
16  com.apple.WebCore             	0x0000000808610f4e WebCore::TaskDispatcher<WebCore::Timer>::dispatchOneTask() + 222 (GenericTaskQueue.cpp:111)
17  com.apple.WebCore             	0x0000000808610c00 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimerFired() + 256 (GenericTaskQueue.cpp:86)
18  com.apple.WebCore             	0x00000008086170c1 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_1::operator()() const + 17 (GenericTaskQueue.cpp:60)
19  com.apple.WebCore             	0x0000000808617089 WTF::Detail::CallableWrapper<WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_1, void>::call() + 25 (Function.h:52)
20  com.apple.WebCore             	0x0000000804d8353a WTF::Function<void ()>::operator()() const + 138 (Function.h:84)
21  com.apple.WebCore             	0x0000000804e37269 WebCore::Timer::fired() + 25 (Timer.h:127)
22  com.apple.WebCore             	0x000000080866018a WebCore::ThreadTimers::sharedTimerFiredInternal() + 650 (ThreadTimers.cpp:129)
23  com.apple.WebCore             	0x000000080866a5f1 WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0::operator()() const + 33 (ThreadTimers.cpp:69)
24  com.apple.WebCore             	0x000000080866a5a9 WTF::Detail::CallableWrapper<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0, void>::call() + 25 (Function.h:52)
25  com.apple.WebCore             	0x0000000804d8353a WTF::Function<void ()>::operator()() const + 138 (Function.h:84)
26  com.apple.WebCore             	0x000000080862a397 WebCore::MainThreadSharedTimer::fired() + 135 (MainThreadSharedTimer.cpp:84)
27  com.apple.WebCore             	0x00000008086d2486 WebCore::timerFired(__CFRunLoopTimer*, void*) + 38 (MainThreadSharedTimerCF.cpp:75)
28  com.apple.CoreFoundation      	0x00007fff36015804 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
29  com.apple.CoreFoundation      	0x00007fff360153be __CFRunLoopDoTimer + 859
30  com.apple.CoreFoundation      	0x00007fff36014e9e __CFRunLoopDoTimers + 317
31  com.apple.CoreFoundation      	0x00007fff35ff9aed __CFRunLoopRun + 2213
32  com.apple.CoreFoundation      	0x00007fff35ff8bd3 CFRunLoopRunSpecific + 499
33  com.apple.Foundation          	0x00007fff3869c1a8 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 212
34  com.apple.Foundation          	0x00007fff3874fd8b -[NSRunLoop(NSRunLoop) run] + 76
35  libxpc.dylib                  	0x00007fff6d8ed0e1 _xpc_objc_main.cold.4 + 49
36  libxpc.dylib                  	0x00007fff6d8ed027 _xpc_objc_main + 559
37  libxpc.dylib                  	0x00007fff6d8ecb5a xpc_main + 377
38  com.apple.WebKit              	0x00000007f889321c WebKit::XPCServiceMain(int, char const**) + 1276 (XPCServiceMain.mm:166)
39  com.apple.WebKit              	0x00000007f9bdcc4b WKXPCServiceMain + 27 (WKMain.mm:33)
40  com.apple.WebKit.WebContent   	0x00000001061e2ec2 main + 34 (AuxiliaryProcessMain.cpp:30)
41  libdyld.dylib                 	0x00007fff6d69e7fd start + 1

[Radar WebKit Bug Importer]

<rdar://problem/60084741>

[Jason Lawrence]

Created attachment 392571 [details]
tracks-support-no-tracks-crash-log

[Jason Lawrence]

I can reproduce this issue with r257915 (debug) and r257905 (debug), but cannot reproduce the issue with r257904 (debug).

run-webkit-tests --iterations 1000 --exit-after-n-failures 5 --force -f --no-retry --debug media/modern-media-controls/tracks-support/tracks-support-no-tracks.html

r257915:
[51/1000] media/modern-media-controls/tracks-support/tracks-support-no-tracks.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=49295])
[51/1000] media/modern-media-controls/tracks-support/tracks-support-no-tracks.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=49293])
[61/1000] media/modern-media-controls/tracks-support/tracks-support-no-tracks.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=49288])
[89/1000] media/modern-media-controls/tracks-support/tracks-support-no-tracks.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=49297])
[91/1000] media/modern-media-controls/tracks-support/tracks-support-no-tracks.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=49412])
Exiting early after 5 failures. 88 tests run.
83 tests ran as expected, 5 didn't (912 didn't run):

r257905 (I switched from 5 to 3 failures here):
[67/1000] media/modern-media-controls/tracks-support/tracks-support-no-tracks.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=50171])
[76/1000] media/modern-media-controls/tracks-support/tracks-support-no-tracks.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=50165])
[78/1000] media/modern-media-controls/tracks-support/tracks-support-no-tracks.html failed unexpectedly (com.apple.WebKit.WebContent.Development crashed [pid=50174])
Exiting early after 3 failures. 75 tests run.
72 tests ran as expected, 3 didn't (925 didn't run):

r257904:
All 1000 tests ran as expected.

[Ryan Haddad]

ASSERTION FAILED: !m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction
./bindings/js/JSEventListener.h(111) : JSC::JSObject *WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext &) const
1   0x7d16190b9 WTFCrash
2   0x7b4ca328b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x7b6f982ea WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext&) const

[Jason Lawrence]

I have rolled out r257905 here: https://trac.webkit.org/changeset/257922/webkit

[Yusuke Suzuki]

This assertion looks wrong. It is assuming that, if m_wrapper goes away, m_jsFunction should go away too. That is not guaranteed by JSC GC.

[Yusuke Suzuki]

It is saying,

1. If the world is normal.
2. If m_wrapper is gone.
3. If m_jsFunction is not gone.

Then, the assertion hits. But this is wrong. We have no guarantee that m_jsFunction is gone when m_wrapper is gone. We have conservative GC. We have generational GC. Everything makes it possible that m_jsFunction is live while m_wrapper is gone.
I'll remove this assertion.

[Alexey Proskuryakov]

DOM is supposed to guarantee that wrappers stay alive for event listeners to fire. Otherwise, code in the listeners will or will not run semi-randomly depending on whether GC happened, which would be incorrect behavior. GC must not be observable from user code.

[Ryosuke Niwa]

The assertion is saying that if m_jsFunction is not null, then m_wrapper shouldn't be null. This must be true because any event target with a valid JS event listener should have had a wrapper when the event listener was added, and the wrapper should never be collected as long as the event listener can be invoked.

[Ryosuke Niwa]

In other words, removing the assertion is not the right solution here. It's an indicative of a bug that we're collecting a wrapper of an event target even when an event can be dispatched in the future on it.

[Yusuke Suzuki]

> The assertion is saying that if m_jsFunction is not null, then m_wrapper shouldn't be null. This must be true because any event target with a valid JS event listener should have had a wrapper when the event listener was added, and the wrapper should never be collected as long as the event listener can be invoked.

No. m_jsFunction can be any value basically if m_wrapper is null. This can be collected. But this can be retained because of conservative GC, because user puts listener function to somewhere, and the other any reasons.
So, m_jsFunction in this assertion is completely unrelated to what we should test.
The correct assertion should be at least,

ASSERT(!m_isolatedWorld->isNormal() || m_wrapper);

[Yusuke Suzuki]

Currently, I'm checking two possibilities,

1. My patch had a bug. And this allows m_wrapper collected by GC. I'm now checking this possibility.
2. This was pre-existing bug. We are not hitting the crash just because m_jsFunction was dead too.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #11)
> > The assertion is saying that if m_jsFunction is not null, then m_wrapper shouldn't be null. This must be true because any event target with a valid JS event listener should have had a wrapper when the event listener was added, and the wrapper should never be collected as long as the event listener can be invoked.
> 
> No. m_jsFunction can be any value basically if m_wrapper is null. This can
> be collected. But this can be retained because of conservative GC, because
> user puts listener function to somewhere, and the other any reasons.
> So, m_jsFunction in this assertion is completely unrelated to what we should
> test.
> The correct assertion should be at least,
> 
> ASSERT(!m_isolatedWorld->isNormal() || m_wrapper);

Maybe, we can also ensure that,

if (m_wrapper)
    ASSERT(m_jsFunction);

But current assertion is saying,

if (!m_wrapper)
    ASSERT(!m_jsFunction);

This is not guaranteed (you can consider conservative GC. Any objects can be marked as live).

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #13)
> (In reply to Yusuke Suzuki from comment #11)
> > > The assertion is saying that if m_jsFunction is not null, then m_wrapper shouldn't be null. This must be true because any event target with a valid JS event listener should have had a wrapper when the event listener was added, and the wrapper should never be collected as long as the event listener can be invoked.
> > 
> > No. m_jsFunction can be any value basically if m_wrapper is null. This can
> > be collected. But this can be retained because of conservative GC, because
> > user puts listener function to somewhere, and the other any reasons.
> > So, m_jsFunction in this assertion is completely unrelated to what we should
> > test.
> > The correct assertion should be at least,
> > 
> > ASSERT(!m_isolatedWorld->isNormal() || m_wrapper);
> 
> Maybe, we can also ensure that,
> 
> if (m_wrapper)
>     ASSERT(m_jsFunction);
> 
> But current assertion is saying,
> 
> if (!m_wrapper)
>     ASSERT(!m_jsFunction);
> 
> This is not guaranteed (you can consider conservative GC. Any objects can be
> marked as live).

And I ensured that nobody is clearing m_jsFunction when m_wrapper is gone. So the current assertion's `!m_jsFunction` part is incorrect.

[Ryosuke Niwa]

Given the backtrace, it looks like someone using GenericEventQueue isn't keeping event target's wrapper alive.
GenericEventQueue is problematic because it doesn't keep event target's wrappers alive automatically.
Each event target which gets added to GenericEventQueue needs to keep its wrapper alive.

[Yusuke Suzuki]

Several updates.

1. The assertion's normal-world check looks also wrong. We are skipping event emission if the world is not normal, this does not make sense. These events can be used to implement the internal feature, and it could emit user-visible events later.
2. So far, this looks like an existing bug due to https://bugs.webkit.org/show_bug.cgi?id=165713. By using IsoSubspace, we start putting some lower-tier cells in PreciseAllocation, and reuse them. This makes GC behavior different, and exposing the existing bug: we should re-register root if we changed the root after we register the root. I could create a test case which reproduces this assertion failure without my patch.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #16)
> 2. So far, this looks like an existing bug due to
> https://bugs.webkit.org/show_bug.cgi?id=165713. By using IsoSubspace, we
> start putting some lower-tier cells in PreciseAllocation, and reuse them.
> This makes GC behavior different, and exposing the existing bug: we should
> re-register root if we changed the root after we register the root. I could
> create a test case which reproduces this assertion failure without my patch.

Let's describe what is happening.

1. HTMLVideoElement is created in the test under the current Document. So root is Document.
2. Concurrent GC starts working.
3. Marking (1)'s HTMLVideoElement and registering Document as a root.
4. The executed code removes HTMLVideoElement from Document.
5. HTMLVideoTrackList in HTMLVideoElement queries the opaque root. Since the root of HTMLVideoElement is changed to itself, HTMLVideoTrackList says "I'm live if the root set includes HTMLVideoElement". But this is not included since HTMLVideoElement registers Document as a root. And after the root is changed, it is not re-registering the new root.
6. HTMLVideoTrackList is saying I'm not reachable.

[Ryosuke Niwa]

(In reply to Yusuke Suzuki from comment #17)
> (In reply to Yusuke Suzuki from comment #16)
> > 2. So far, this looks like an existing bug due to
> > https://bugs.webkit.org/show_bug.cgi?id=165713. By using IsoSubspace, we
> > start putting some lower-tier cells in PreciseAllocation, and reuse them.
> > This makes GC behavior different, and exposing the existing bug: we should
> > re-register root if we changed the root after we register the root. I could
> > create a test case which reproduces this assertion failure without my patch.
> 
> Let's describe what is happening.
> 
> 1. HTMLVideoElement is created in the test under the current Document. So
> root is Document.
> 2. Concurrent GC starts working.
> 3. Marking (1)'s HTMLVideoElement and registering Document as a root.
> 4. The executed code removes HTMLVideoElement from Document.
> 5. HTMLVideoTrackList in HTMLVideoElement queries the opaque root. Since the
> root of HTMLVideoElement is changed to itself, HTMLVideoTrackList says "I'm
> live if the root set includes HTMLVideoElement". But this is not included
> since HTMLVideoElement registers Document as a root. And after the root is
> changed, it is not re-registering the new root.
> 6. HTMLVideoTrackList is saying I'm not reachable.

In this scenario, HTMLVideoElement needs to have a pending activity in ActiveDOMObjet while HTMLVideoTrackList/HTMLVideoElement sits in GenericEventQueue. That would prevent GC from collecting the JS wrapper since it would be reachableFromOpaqueRoot regardless of where it is.

[Yusuke Suzuki]

(In reply to Ryosuke Niwa from comment #18)
> (In reply to Yusuke Suzuki from comment #17)
> > (In reply to Yusuke Suzuki from comment #16)
> > > 2. So far, this looks like an existing bug due to
> > > https://bugs.webkit.org/show_bug.cgi?id=165713. By using IsoSubspace, we
> > > start putting some lower-tier cells in PreciseAllocation, and reuse them.
> > > This makes GC behavior different, and exposing the existing bug: we should
> > > re-register root if we changed the root after we register the root. I could
> > > create a test case which reproduces this assertion failure without my patch.
> > 
> > Let's describe what is happening.
> > 
> > 1. HTMLVideoElement is created in the test under the current Document. So
> > root is Document.
> > 2. Concurrent GC starts working.
> > 3. Marking (1)'s HTMLVideoElement and registering Document as a root.
> > 4. The executed code removes HTMLVideoElement from Document.
> > 5. HTMLVideoTrackList in HTMLVideoElement queries the opaque root. Since the
> > root of HTMLVideoElement is changed to itself, HTMLVideoTrackList says "I'm
> > live if the root set includes HTMLVideoElement". But this is not included
> > since HTMLVideoElement registers Document as a root. And after the root is
> > changed, it is not re-registering the new root.
> > 6. HTMLVideoTrackList is saying I'm not reachable.
> 
> In this scenario, HTMLVideoElement needs to have a pending activity in
> ActiveDOMObjet while HTMLVideoTrackList/HTMLVideoElement sits in
> GenericEventQueue. That would prevent GC from collecting the JS wrapper
> since it would be reachableFromOpaqueRoot regardless of where it is.

No, it does not fix the issue unfortunately :(
Even if HTMLVideoElement is live, HTMLVideoTrackList goes away since HTMLVideoTrackList is live only if it can find an opaque root, which is not properly registered by HTMLVideoElement due to race condition.
Fundamental fix should be, in some way, re-registering an root & saying "Marking" happens if the root is changed. This is some form of write-barrier.

This issue itself exists for 4 years, and this issue is hidden by this wrong assertion I think (this happens frequently for HTMLVideoElement, but this assertion does not hit due to `m_jsFunction` check and world-is-normal check since this pattern is used in non-normal world (media controls).

[Ryosuke Niwa]

(In reply to Yusuke Suzuki from comment #19)
> (In reply to Ryosuke Niwa from comment #18)
>
> > In this scenario, HTMLVideoElement needs to have a pending activity in
> > ActiveDOMObjet while HTMLVideoTrackList/HTMLVideoElement sits in
> > GenericEventQueue. That would prevent GC from collecting the JS wrapper
> > since it would be reachableFromOpaqueRoot regardless of where it is.
> 
> No, it does not fix the issue unfortunately :(
> Even if HTMLVideoElement is live, HTMLVideoTrackList goes away since
> HTMLVideoTrackList is live only if it can find an opaque root, which is not
> properly registered by HTMLVideoElement due to race condition.

Okay, so VideoTrackList needs to be an ActiveDOMObject itself and have a pending activity whenever it's in HTMLVideoElement's m_asyncEventQueue.

> Fundamental fix should be, in some way, re-registering an root & saying
> "Marking" happens if the root is changed. This is some form of write-barrier.

Hm... that sounds like a brittle approach but if that's what we need to do, then dispatchChildRemovalEvents in ContainerNode.cpp has a code to force creating JS wrapper on the removed node so perhaps we can emit a write barrier there.

Alternatively, can we use MarkingConstraint to relate these objects?

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #20)
> (In reply to Yusuke Suzuki from comment #19)
> > (In reply to Ryosuke Niwa from comment #18)
> >
> > > In this scenario, HTMLVideoElement needs to have a pending activity in
> > > ActiveDOMObjet while HTMLVideoTrackList/HTMLVideoElement sits in
> > > GenericEventQueue. That would prevent GC from collecting the JS wrapper
> > > since it would be reachableFromOpaqueRoot regardless of where it is.
> > 
> > No, it does not fix the issue unfortunately :(
> > Even if HTMLVideoElement is live, HTMLVideoTrackList goes away since
> > HTMLVideoTrackList is live only if it can find an opaque root, which is not
> > properly registered by HTMLVideoElement due to race condition.
> 
> Okay, so VideoTrackList needs to be an ActiveDOMObject itself and have a
> pending activity whenever it's in HTMLVideoElement's m_asyncEventQueue.

Let another approach here is to use CustomIsReachable and write a custom JSVideoTrackList::isReachableFromOpaqueRoots, or add a new variant of GenerateIsReachable like ImplMediaElementRoot which checks not just that the root of the media element is an opaque root but also if the media element has a pending activity or not.

[Yusuke Suzuki]

I think this is more fundamental problem which is not tied to HTMLVideoElement case.
For any node, this race condition happens if,

1. Node (A) is attached to the tree (TA).
2. (A)'s wrapper is marked and (TA) is registered as an opaque root.
3. The user code removes (A) from (TA).
4. Node (B), which is under (A), is invoking is-reachable-from-opaque-root query.
5. Node (B)'s root is (A) since (A) is detached from the tree (TA).
6. (4) cannot find the (A) from the set of opaque roots.

Let's consider about the fundamental fix. Every time we change the root, we need to emit "write-barrier" to this node.
Then, GC rescans it, re-register the updated root. So, the problem is how to emit "write-barrier" while we only have C++ node.

So the question is how to achieve this effect. One idea I have right now is, (1) when changing the root, (2) if GC is not running right now, noop, (2) otherwise, let's check the old root is included in the opaque root, (3) if it is included, we put the new root into the opaque root as something like remember-set mechanism. Then, conservatively we collect the necessary opaque roots. And once GC cycles pass, opaque root set is anyway cleared again. This works at least for main thread since we can access these data via commonVM().

[Ryosuke Niwa]

(In reply to Yusuke Suzuki from comment #22)
> I think this is more fundamental problem which is not tied to
> HTMLVideoElement case.
> For any node, this race condition happens if,
> 
> 1. Node (A) is attached to the tree (TA).
> 2. (A)'s wrapper is marked and (TA) is registered as an opaque root.
> 3. The user code removes (A) from (TA).
> 4. Node (B), which is under (A), is invoking is-reachable-from-opaque-root query.
> 5. Node (B)'s root is (A) since (A) is detached from the tree (TA).
> 6. (4) cannot find the (A) from the set of opaque roots.

That indeed sounds like a real bug.

> Let's consider about the fundamental fix. Every time we change the root, we
> need to emit "write-barrier" to this node.
> Then, GC rescans it, re-register the updated root. So, the problem is how to
> emit "write-barrier" while we only have C++ node.
> 
> So the question is how to achieve this effect. One idea I have right now is,
> (1) when changing the root,

This would be ContainerNode::removeAllChildrenWithScriptAssertion, ContainerNode::removeNodeWithScriptAssertion, and executeNodeInsertionWithScriptAssertion.

> (2) if GC is not running right now, noop, (2)
> otherwise, let's check the old root is included in the opaque root, (3) if
> it is included, we put the new root into the opaque root as something like
> remember-set mechanism.

Hm... I'm thinking if we can re-use something like GCReachableRefMap or add more logic to isReachableFromDOM in JSNodeCustom.cpp but I don't think we could because the issue isn't so much about the root node, which is an opaque root but rather anyone which uses a node has an opaque root.

For that matter, this seems like an issue with anything that can dynamically change its opaque root, not just Node.

[Yusuke Suzuki]

Or, super easy way (I'm not sure this is good for performance for now), I'm now considering: let's register opaque roots in output constraints!

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #24)
> Or, super easy way (I'm not sure this is good for performance for now), I'm
> now considering: let's register opaque roots in output constraints!

Not sure, some wiring should be required. But this sounds the cleanest solution.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #25)
> (In reply to Yusuke Suzuki from comment #24)
> > Or, super easy way (I'm not sure this is good for performance for now), I'm
> > now considering: let's register opaque roots in output constraints!
> 
> Not sure, some wiring should be required. But this sounds the cleanest
> solution.

Need to check. It would be possible that I'm missing something.

[Ryosuke Niwa]

(In reply to Yusuke Suzuki from comment #24)
> Or, super easy way (I'm not sure this is good for performance for now), I'm
> now considering: let's register opaque roots in output constraints!

When you say register opaque roots in output constraints, do you mean to do that for all opaque roots?

[Yusuke Suzuki]

Ooops, maybe I found the bug.
I'm now checking but maybe this is caused by my silly mistake. Sorry.
I'm rolling out the patch for now. And ensuring the new patch is fixing this issue. But assertion is anyway wrong so I'll create a different patch for fixing the assertion too.

[Yusuke Suzuki]

OK, I found the super silly bug in my original patch https://bugs.webkit.org/show_bug.cgi?id=205107#c13. Really sorry :(
But at the same time, this assertion is still wrong. I'll use this bug for fixing the assertion to the correct form: ASSERT(m_wrapper && m_jsFunction); since,

1. Whether the world is normal or not does not matter to event emission. This is introduced to suppress some assertion failures originally, this is wrong.
2. The original assertion was saying, "if m_wrapper is dead, m_jsFunction must be dead". This is wrong since m_jsFunction can be alive due to whatever reasons. For GC managed objects, we cannot say "This is dead since it is not referenced from anywhere" since conservative GC can find it even if it is not logically referenced. Only thing we can say is, "if m_wrapper is live, m_jsFunction should be live".

[Ryosuke Niwa]

Do we still have the issue with opaque roots dynamically updating? Or was it just red herring due to that bug?

[Yusuke Suzuki]

Created attachment 392702 [details]
Patch

[Yusuke Suzuki]

(In reply to Ryosuke Niwa from comment #30)
> Do we still have the issue with opaque roots dynamically updating? Or was it
> just red herring due to that bug?

I've checked that and as I thought we are already emitting opaque root registering in the output-constraints. So this works correctly :)

[Yusuke Suzuki]

Comment on attachment 392702 [details]
Patch

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

> Source/WebCore/bindings/js/JSEventListener.h:110
> +    ASSERT(!m_isolatedWorld->isNormal() || m_wrapper);

We could even say, `ASSERT(!m_isolatedWorld->isNormal() || (m_wrapper && m_jsFunction));` but not sure whether we have a case that m_jsFunction is null initialized while the m_wrapper is non-null initialized.
And ideal ASSERT would be  `ASSERT(m_wrapper && m_jsFunction)` since normal-world or not does not matter whether we should skip the event handlers.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #29)
> OK, I found the super silly bug in my original patch
> https://bugs.webkit.org/show_bug.cgi?id=205107#c13. Really sorry :(

So if we don’t visit any of our output constraint spaces, we only have a single test that fails in a flaky manner?

Sounds like we need better tests :-)

> But at the same time, this assertion is still wrong. I'll use this bug for
> fixing the assertion to the correct form: ASSERT(m_wrapper && m_jsFunction);
> since,
> 
> 1. Whether the world is normal or not does not matter to event emission.
> This is introduced to suppress some assertion failures originally, this is
> wrong.
> 2. The original assertion was saying, "if m_wrapper is dead, m_jsFunction
> must be dead". This is wrong since m_jsFunction can be alive due to whatever
> reasons. For GC managed objects, we cannot say "This is dead since it is not
> referenced from anywhere" since conservative GC can find it even if it is
> not logically referenced. Only thing we can say is, "if m_wrapper is live,
> m_jsFunction should be live".

[Alexey Proskuryakov]

Comment on attachment 392702 [details]
Patch

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

Since the change only makes the assertion stronger, it is good as far as I'm concerned. But tests are now asserting, so mechanically r-.

> Source/WebCore/ChangeLog:12
> +           GC, we cannot say such an condition. Even if m_wrapper is dead, m_jsFunction can be alive by various reasons: conservative
> +           GC finds it, user code stores this function somewhere reachable from the root, etc. The correct thing is `m_wrapper` must

There are a couple things here that make me a little skeptical, from a position of someone who only touched this a long time ago and is absolutely not an expert.

1. If this assertion can misfire due to conservative GC finding it, why haven't we ever seen it fire without a real bug?

2. The assertion would fire if user code stored the function somewhere, so it's essentially saying that user code must not do that. That isn't a bug in the assertion necessarily. Is there even a reason for user code to do this?

3. Checking m_jsFunction for being null is not the same as checking whether the function is alive. Once can just assign null to the member, and arguably should do this without waiting for GC. Is it even right that m_jsFunction is a weak pointer?

> Source/WebCore/ChangeLog:15
> +        2. The comments about "zombie m_jsFunctions" is wrong. We are using JSC::Weak<>. So if the object gets collected, it returns
> +           nullptr, not getting a zombie pointer.

Yes, must have been written before these were weak pointers.

> Source/WebCore/ChangeLog:17
> +        3. We are emitting write-barrier in a wrong order. In the heavily stressed scenario, it is possible that concurrent marking
> +           scans JSEventListener just after we emit the write-barrier, and this marking misses the assigned value. We must emit

Great find! Is there any test that hits this, at least in StressGC configuration?

> Source/WebCore/ChangeLog:21
> +        Still keeping normal world check. But this looks also wrong. The assertion is allowing non-normal world to get dead m_wrapper.

Yes, pretty sure it's been understood to be a dirty hack when added. Interaction between worlds and event listeners was bogus back then, and I don't know if it has been improved since.

[Yusuke Suzuki]

Comment on attachment 392702 [details]
Patch

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

The test failure is saying that we invoke JSEventListener::jsFunction with `!m_wrapper` legally.
This sounds like we are also using `m_wrapper` for whether it is initialized or not. This sounds like wrong too.

>> Source/WebCore/ChangeLog:12
>> +           GC finds it, user code stores this function somewhere reachable from the root, etc. The correct thing is `m_wrapper` must
> 
> There are a couple things here that make me a little skeptical, from a position of someone who only touched this a long time ago and is absolutely not an expert.
> 
> 1. If this assertion can misfire due to conservative GC finding it, why haven't we ever seen it fire without a real bug?
> 
> 2. The assertion would fire if user code stored the function somewhere, so it's essentially saying that user code must not do that. That isn't a bug in the assertion necessarily. Is there even a reason for user code to do this?
> 
> 3. Checking m_jsFunction for being null is not the same as checking whether the function is alive. Once can just assign null to the member, and arguably should do this without waiting for GC. Is it even right that m_jsFunction is a weak pointer?

1.
This is because this assertion is using `!m_jsFuncion` wrong purpose. We are *not* firing if `m_jsFunction` is successfully collected even if `m_wrapper` is collected. This weakens the assertion so we could miss the real bugs due to this wrong condition.
And again, checking `m_jsFunction`'s liveness here is unrelated to the validity of JSEventListener. I think the key is that, previously this m_jsFunction is not Weak<>. So, when we are evaluating `!m_jsFuncion`, we are not checking the liveness of this. Rather we are checking whether we initialized it once before. However, now it is replaced with Weak<> and this `!m_jsFunction` becomes "it is not initialized OR it is collected". This is clearly wrong, and that's why this worked so far. We made this assertion's meaning weak unintentionally when we introduced Weak<> to this JSEventListener.
I guess that this could hide the real bug so far. And making the meaning of this assertion looks very wrong because this is now checking m_jsFunction's liveness too.

The assertion is saying "Even if the m_wrapper is collected, if m_jsFunction  is collected too, this is OK, let's skip event triggering". No, this is not OK, this is the bug.

2.
Yes, it is possible. You can write a code,

function handler() { };
imgElement.onload = handler;

This `handler` is m_jsFunction. You can put it anywhere. So making `m_jsFunction` alive is very easy. The key here is that, the original intention of `m_jsFunction` check is not looking into the liveness. It is checking whether we initialized this field once (because we were using WriteBarrier<>).
But now, this is changed to Weak<>, and the meaning becomes broken.

3.
Yes. And checking `m_jsFunction` for being null is not the same as checking it is not initialized too.
But this assertion is using `m_jsFunction` condition to check wether it is initialized. This is wrong. Liveness should not be checked.

[Yusuke Suzuki]

Comment on attachment 392702 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +           scans JSEventListener just after we emit the write-barrier, and this marking misses the assigned value. We must emit
> 
> Great find! Is there any test that hits this, at least in StressGC configuration?

It is possible that if we stress this GC situation enough, we could eventually hit GC crash. But not sure whether our GC bots have the tests which stresses this path.

> Source/WebCore/bindings/js/JSEventListener.h:101
>      if (!m_jsFunction) {

Now, I think this condition is also wrong. I think this is originally inserted as "this listener is not initialized yet". But now, this `!m_jsFunction`'s meaning is different from this.
Initializing function again if the function is gone sounds wrong.

[Yusuke Suzuki]

Created attachment 392888 [details]
Patch

[Yusuke Suzuki]

Created attachment 392892 [details]
Patch

[Yusuke Suzuki]

Created attachment 392893 [details]
Patch

[Yusuke Suzuki]

Created attachment 392895 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 392895 [details]
Patch

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

> Source/WebCore/bindings/js/JSEventListener.cpp:56
>          m_jsFunction = JSC::Weak<JSC::JSObject>(function);

I think the assertion is saying this can be nullptr.

[Yusuke Suzuki]

Created attachment 392911 [details]
Patch

[Yusuke Suzuki]

I’m now thinking that corrected assertion found the real issue.
https://ews-build.webkit.org/results/macOS-Mojave-Debug-WK1-Tests-EWS/r392911-4549/results.html

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #44)
> I’m now thinking that corrected assertion found the real issue.
> https://ews-build.webkit.org/results/macOS-Mojave-Debug-WK1-Tests-EWS/
> r392911-4549/results.html

LoadableTextTrack should be kept alive while it is loading.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #45)
> (In reply to Yusuke Suzuki from comment #44)
> > I’m now thinking that corrected assertion found the real issue.
> > https://ews-build.webkit.org/results/macOS-Mojave-Debug-WK1-Tests-EWS/
> > r392911-4549/results.html
> 
> LoadableTextTrack should be kept alive while it is loading.

LoadableTextTrack's HTMLTrackElement's wrapper should be kept alive.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #46)
> (In reply to Yusuke Suzuki from comment #45)
> > (In reply to Yusuke Suzuki from comment #44)
> > > I’m now thinking that corrected assertion found the real issue.
> > > https://ews-build.webkit.org/results/macOS-Mojave-Debug-WK1-Tests-EWS/
> > > r392911-4549/results.html
> > 
> > LoadableTextTrack should be kept alive while it is loading.
> 
> LoadableTextTrack's HTMLTrackElement's wrapper should be kept alive.

If LoadableTextTrack is loading a track, and firing events, if HTMLTrackElement is removed from the tree and nobody is looking into it (while event listener is set), I think this assertion hits.

[Darin Adler]

Comment on attachment 392911 [details]
Patch

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

> Source/WebCore/bindings/js/JSEventListener.cpp:57
> +    if (!wrapper) {
>          ASSERT(!function);
> +        return;
> +    }

Inconsistent to use early return for (!wrapper), but a normal if statement for (function). I suggest we write this instead:

    if (function) {
        ASSERT(wrapper);
        ...

[Yusuke Suzuki]

Comment on attachment 392911 [details]
Patch

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

>> Source/WebCore/bindings/js/JSEventListener.cpp:57
>> +    }
> 
> Inconsistent to use early return for (!wrapper), but a normal if statement for (function). I suggest we write this instead:
> 
>     if (function) {
>         ASSERT(wrapper);
>         ...

Thanks, fixed.

[Yusuke Suzuki]

Committed r258189: <https://trac.webkit.org/changeset/258189>

[Enrique Ocaña]

This patch is solving an issue we found downstream on wpe-2.22 (and 2.28 but not on 2.30) on ARM 32bit devices (at least, as that's what we use).

We were directly accessing mediaSource.sourceBuffers from JavaScript to register an event listener on the addsourcebuffer event, but when the event happened, the event listener had been declared "dead" by the garbage collector and never ran. If we held mediaSource.sourceBuffers in a temporary variable in an outer scope, the behaviour didn't happen.

It took a long bisect to locate this fix. Thanks a lot for it!