Drop unnecessary EventTarget::isFiringEventListeners()

[cdumez]

b337028

Drop unnecessary EventTarget::isFiringEventListeners()
https://bugs.webkit.org/show_bug.cgi?id=240578

Reviewed by Geoffrey Garen.

Drop unnecessary EventTarget::isFiringEventListeners() flag. This flag was checked by
our GC logic to avoid collecting wrappers while they are firing events. However, those
checks are in hot code paths and it should be up to the code dispatching the event
to keep the wrapper alive, either via ActiveDOMObject or GCReachableRef.

* Source/WebCore/bindings/js/JSAbortSignalCustom.cpp:
(WebCore::JSAbortSignalOwner::isReachableFromOpaqueRoots):
* Source/WebCore/bindings/js/JSNodeCustom.cpp:
(WebCore::JSNodeOwner::isReachableFromOpaqueRoots):
* Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:
(GenerateImplementation):
* Source/WebCore/dom/EventTarget.cpp:
(WebCore::EventTarget::fireEventListeners):
* Source/WebCore/dom/EventTarget.h:
(WebCore::EventTarget::isFiringEventListeners const): Deleted.

Canonical link: https://commits.webkit.org/250712@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294431 268f45cc-cd09-0410-ab3c-d52691b4dbfc

[geoffreygaren]

it should be up to the code dispatching the event to keep the wrapper alive

I agree with this in theory -- I hope it's true in practice? 😬

[geoffreygaren]

r=me

[cdumez]

it should be up to the code dispatching the event to keep the wrapper alive

I agree with this in theory -- I hope it's true in practice? 😬

At least, it is passing the tests. If we do find cases where we don't, we can always fix those.

[geoffreygaren]

I guess the comment inside EventTarget::innerInvokeEventListeners demonstrates that the isFiringEventListeners flag was insufficient to cover all edge cases anyway, at least in the case of an event listener with the 'once' flag.

[rniwa]

We could deploy GCReachableRef in EventPath object if we want to be conservative.

[cdumez]

We could deploy GCReachableRef in EventPath object if we want to be conservative.

We could but it would only work for Node, not other EventTargets.

[rniwa]

We could deploy GCReachableRef in EventPath object if we want to be conservative.

We could but it would only work for Node, not other EventTargets.

I guess the alternative is to do that in fireEventListeners but I sort of agree with the current approach of removing this code and looking for places that need this check instead. Can we change the debug assertion that checks the existence of JS wrapper to a release assertion temporarily? That should allow fuzzers & crash reports to catch cases where JS wrappers are incorrectly collected

[cdumez]

We could deploy GCReachableRef in EventPath object if we want to be conservative.

We could but it would only work for Node, not other EventTargets.

I guess the alternative is to do that in fireEventListeners but I sort of agree with the current approach of removing this code and looking for places that need this check instead.

What I meant is that GCReachableRef only works for Nodes AFAIK, not other types of objects. My comment wasn't about the location of the GCReachableRef.

Can we change the debug assertion that checks the existence of JS wrapper to a release assertion temporarily? That should allow fuzzers & crash reports to catch cases where JS wrappers are incorrectly collected

We could. My worries are that it is a risky time to be adding release assertions. Also, I bet the assertion would hit a lot with fuzzers, even before my patch. Last I checked, it wasn't hard to find such failures on the stress GC bot.

[rniwa]

What I meant is that GCReachableRef only works for Nodes AFAIK, not other types of objects. My comment wasn't about the location of the GCReachableRef.

oh sure, we'd have to change it to support all event targets. Now that I think about it, that could in turn incurr new perf cost so maybe not the best idea.

We could. My worries are that it is a risky time to be adding release assertions. Also, I bet the assertion would hit a lot with fuzzers, even before my patch. Last I checked, it wasn't hard to find such failures on the stress GC bot.

Doesn't that mean this patch will likely introduce a new regression?? Maybe we can somehow turn this assertion for just fuzzers.

I'm worried that this change will lead to flaky JS failures or flaky website behavior and very hard to decipher / diagnose bug reports without some due diligence like that.

[cdumez]

What I meant is that GCReachableRef only works for Nodes AFAIK, not other types of objects. My comment wasn't about the location of the GCReachableRef.

oh sure, we'd have to change it to support all event targets. Now that I think about it, that could in turn incurr new perf cost so maybe not the best idea.

We could. My worries are that it is a risky time to be adding release assertions. Also, I bet the assertion would hit a lot with fuzzers, even before my patch. Last I checked, it wasn't hard to find such failures on the stress GC bot.

Doesn't that mean this patch will likely introduce a new regression?? Maybe we can somehow turn this assertion for just fuzzers.

I'm worried that this change will lead to flaky JS failures or flaky website behavior and very hard to decipher / diagnose bug reports without some due diligence like that.

Geoff and I are following up on Slack to see what coverage we have in terms of fuzzing or stress GC here. To be clear though, if this patch introduces a bug, I think it means the existing code was already unsafe.

You should always be keeping your JS wrapper alive as long as you may fire events on it in the future. This means that you're supposed to already have a mechanism to keep your wrapper alive at the point you dispatch your event. If not, just keeping the wrapper alive during event firing is at best a band-aid and the flakiness you're worried about could already happen if the wrapper was not kept alive right before dispatching the event.

My understanding is that this patch might make pre-existing bugs a bit more visible (which is not necessarily a bad thing). It is also not clear to be how much more likely a wrapper might get collecting during event firing vs before firing the event.

[rniwa]

My understanding is that this patch might make pre-existing bugs a bit more visible (which is not necessarily a bad thing). It is also not clear to be how much more likely a wrapper might get collecting during event firing vs before firing the event.

Yeah, I understand that. But flaky failure happening more frequently is not great anyhow.

[webkit-early-warning-system]

Committed r294431 (250712@main): https://commits.webkit.org/250712@main

Reviewed commits have been landed. Closing PR #723 and removing active labels.