Event listeners registered with 'once' option may get garbage collected too soon

[Chris Dumez]

Event listeners registered with 'once' option may get garbage collected too soon, before they get a chance to run.

[Chris Dumez]

<rdar://problem/60541567>

[Chris Dumez]

Created attachment 394427 [details]
Patch

[Saam Barati]

Comment on attachment 394427 [details]
Patch

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

> Source/WebCore/dom/EventTarget.cpp:334
> +        std::unique_ptr<ListenerGCProtector> listenerGCProtector;

why not optional?

[Saam Barati]

Comment on attachment 394427 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        No new tests, covered by http/tests/inspector/network/har/har-page.html which crashes
> +        with aggressive GC.

do we have a version of this test that runs with aggressive GC already checked in?

[Chris Dumez]

Comment on attachment 394427 [details]
Patch

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

>> Source/WebCore/ChangeLog:20
>> +        with aggressive GC.
> 
> do we have a version of this test that runs with aggressive GC already checked in?

I mean we have this test which gets run on the stress GC bot and currently crashes, thus the radar. Is there a way to make a single test use aggressive GC on the regular bots?

>> Source/WebCore/dom/EventTarget.cpp:334
>> +        std::unique_ptr<ListenerGCProtector> listenerGCProtector;
> 
> why not optional?

Why is Optional better?

[Chris Dumez]

Comment on attachment 394427 [details]
Patch

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

>>> Source/WebCore/dom/EventTarget.cpp:334
>>> +        std::unique_ptr<ListenerGCProtector> listenerGCProtector;
>> 
>> why not optional?
> 
> Why is Optional better?

I guess because we would not need to heap-allocate then?

[Saam Barati]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 394427 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394427&action=review
> 
> >>> Source/WebCore/dom/EventTarget.cpp:334
> >>> +        std::unique_ptr<ListenerGCProtector> listenerGCProtector;
> >> 
> >> why not optional?
> > 
> > Why is Optional better?
> 
> I guess because we would not need to heap-allocate then?

Not heap allocated

[Saam Barati]

(In reply to Saam Barati from comment #7)
> (In reply to Chris Dumez from comment #6)
> > Comment on attachment 394427 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=394427&action=review
> > 
> > >>> Source/WebCore/dom/EventTarget.cpp:334
> > >>> +        std::unique_ptr<ListenerGCProtector> listenerGCProtector;
> > >> 
> > >> why not optional?
> > > 
> > > Why is Optional better?
> > 
> > I guess because we would not need to heap-allocate then?
> 
> Not heap allocated

Sorry, was responding to your first comment. Yes, it's not heap allocated :-)

[Chris Dumez]

(In reply to Saam Barati from comment #8)
> (In reply to Saam Barati from comment #7)
> > (In reply to Chris Dumez from comment #6)
> > > Comment on attachment 394427 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=394427&action=review
> > > 
> > > >>> Source/WebCore/dom/EventTarget.cpp:334
> > > >>> +        std::unique_ptr<ListenerGCProtector> listenerGCProtector;
> > > >> 
> > > >> why not optional?
> > > > 
> > > > Why is Optional better?
> > > 
> > > I guess because we would not need to heap-allocate then?
> > 
> > Not heap allocated
> 
> Sorry, was responding to your first comment. Yes, it's not heap allocated :-)

Ok, will fix this now. Let me know if you know how to make a single test use aggressive GC.

[Saam Barati]

(In reply to Chris Dumez from comment #9)
> (In reply to Saam Barati from comment #8)
> > (In reply to Saam Barati from comment #7)
> > > (In reply to Chris Dumez from comment #6)
> > > > Comment on attachment 394427 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=394427&action=review
> > > > 
> > > > >>> Source/WebCore/dom/EventTarget.cpp:334
> > > > >>> +        std::unique_ptr<ListenerGCProtector> listenerGCProtector;
> > > > >> 
> > > > >> why not optional?
> > > > > 
> > > > > Why is Optional better?
> > > > 
> > > > I guess because we would not need to heap-allocate then?
> > > 
> > > Not heap allocated
> > 
> > Sorry, was responding to your first comment. Yes, it's not heap allocated :-)
> 
> Ok, will fix this now. Let me know if you know how to make a single test use
> aggressive GC.

Yeah, at the top of the test, you could do something like we have in other tests:

```
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"><!-- webkit-test-runner [ jscOptions=--slowPathAllocsBetweenGCs=5 ] -->
```

I haven't verified this works myself, so you might want to ensure the test fails without your fix. If that specific option doesn't work, maybe a combination of other options could work better

[Chris Dumez]

Created attachment 394436 [details]
Patch

[Chris Dumez]

Created attachment 394438 [details]
Patch

[Saam Barati]

Comment on attachment 394438 [details]
Patch

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

r=me

> Source/WebCore/dom/EventTarget.cpp:93
> +    RefPtr<EventTarget> m_target;
> +    RefPtr<RegisteredEventListener> m_listener;

why not ref like you had before?

[Saam Barati]

Comment on attachment 394438 [details]
Patch

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

> Source/WebCore/dom/EventTarget.cpp:76
> +        if (!m_target)
> +            return;

if you do below, this branch can be removed

> Source/WebCore/dom/EventTarget.cpp:89
> +    ListenerGCProtector(const ListenerGCProtector&) = delete;
> +    ListenerGCProtector& operator=(const ListenerGCProtector&&) = delete;
> +    ListenerGCProtector(ListenerGCProtector&&) = default;
> +    ListenerGCProtector& operator=(ListenerGCProtector&&) = default;

I think you can just use .emplace instead of using "operator=" in your code below, and all of these can be delete or left unspecified

[Geoffrey Garen]

Comment on attachment 394438 [details]
Patch

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

> Source/WebCore/dom/EventTarget.cpp:341
> +        Optional<ListenerGCProtector> listenerGCProtector;

Are we just trying to keep the function alive within the scope of this function? If so, I think this might be overkill. We can just call ensureStillAliveHere at the end of the function; or create an RAII EnsureStillAliveScope for this purpose.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #15)
> Comment on attachment 394438 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394438&action=review
> 
> > Source/WebCore/dom/EventTarget.cpp:341
> > +        Optional<ListenerGCProtector> listenerGCProtector;
> 
> Are we just trying to keep the function alive within the scope of this
> function? If so, I think this might be overkill. We can just call
> ensureStillAliveHere at the end of the function; or create an RAII
> EnsureStillAliveScope for this purpose.

Yes, we basically just want to extend the lifetime of the function for the scope. I went the visitor way because this is what I understand. ensureStillAliveHere() may be another way to fix this but I don't understand it :) I have asked Yusuke to take a look and comment.

[Yusuke Suzuki]

Comment on attachment 394438 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        In EventTarget::innerInvokeEventListeners, if the listener we're about to call is a one-time
> +        listener (has 'once' flag set), we would first unregister the event listener and then call
> +        it, as per the DOM specification. However, once unregistered, the event listener is no longer
> +        visited for GC purposes and its internal JS Function may get garbage collected before we get
> +        a chance to call it.

Nice find.

>> Source/WebCore/dom/EventTarget.cpp:341
>> +        Optional<ListenerGCProtector> listenerGCProtector;
> 
> Are we just trying to keep the function alive within the scope of this function? If so, I think this might be overkill. We can just call ensureStillAliveHere at the end of the function; or create an RAII EnsureStillAliveScope for this purpose.

Yes. I think we should do something like this would be enough and efficient :)

EnsureStillAliveScope wrapper;
EnsureStillAliveScope function;

if (registeredListener->isOnce()) {
    wrapper = ...->wrapper();
    function = ...->function(); // In some way.
}
...
if (registeredListener->isPassive())
    event.setInPassiveListener(false);

[Chris Dumez]

(In reply to Yusuke Suzuki from comment #17)
> Comment on attachment 394438 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394438&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        In EventTarget::innerInvokeEventListeners, if the listener we're about to call is a one-time
> > +        listener (has 'once' flag set), we would first unregister the event listener and then call
> > +        it, as per the DOM specification. However, once unregistered, the event listener is no longer
> > +        visited for GC purposes and its internal JS Function may get garbage collected before we get
> > +        a chance to call it.
> 
> Nice find.
> 
> >> Source/WebCore/dom/EventTarget.cpp:341
> >> +        Optional<ListenerGCProtector> listenerGCProtector;
> > 
> > Are we just trying to keep the function alive within the scope of this function? If so, I think this might be overkill. We can just call ensureStillAliveHere at the end of the function; or create an RAII EnsureStillAliveScope for this purpose.
> 
> Yes. I think we should do something like this would be enough and efficient
> :)
> 
> EnsureStillAliveScope wrapper;
> EnsureStillAliveScope function;
> 
> if (registeredListener->isOnce()) {
>     wrapper = ...->wrapper();
>     function = ...->function(); // In some way.
> }
> ...
> if (registeredListener->isPassive())
>     event.setInPassiveListener(false);

Hmm, Ok. I will try this alternative tomorrow and confirm it fixes the crash too. Not sure if it is really less work / code, we'll see.

[Chris Dumez]

Created attachment 394450 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #19)
> Created attachment 394450 [details]
> Patch

Uploaded this clean version of the patch reviewed by Saam, in case the alternative approach does not end up working out.

[Alexey Proskuryakov]

Comment on attachment 394450 [details]
Patch

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

> LayoutTests/http/tests/inspector/network/har/har-page-aggressive-gc.html:1
> +<!DOCTYPE html><!-- webkit-test-runner [ jscOptions=--slowPathAllocsBetweenGCs=5 ] -->

How slow does this make the test? Not sure where I would draw the line, but we don’t want stress tests run as part of normal test suite in general.

[Alexey Proskuryakov]

Also, would this crash in a more reduced test when slowPathAllocsBetweenGCs is used?

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #22)
> Also, would this crash in a more reduced test when slowPathAllocsBetweenGCs
> is used?

I tried the following test case but have not been able to reproduce it with a simple test case:

<!DOCTYPE html><!-- webkit-test-runner [ jscOptions=--slowPathAllocsBetweenGCs=5 ] -->
<html>
<body>
<p>This test passes if it does not crash or time out.</p>
<script>
let messagesReceived = 0;
if (window.testRunner) {
    testRunner.dumpAsText();
    testRunner.waitUntilDone();
}

function handleMessageReceived()
{
    messagesReceived++
    if (messagesReceived == 500) {
        if (window.testRunner)
            testRunner.notifyDone();
        return;
    }

    sendMessage();
}

function sendMessage()
{
    window.addEventListener("message", (messageEvent) => { handleMessageReceived(); }, { capture: true, once: true });
    window.postMessage("foo", "*");
}

onload = () => {
    sendMessage();
}

</script>
</body>
</html>

[Chris Dumez]

Created attachment 394510 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 394510 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSCJSValue.h:653
> +class EnsureStillAlive {

Let's put WTF_FORBID_HEAP_ALLOCATION, WTF_MAKE_NONCOPYABLE, and WTF_MAKE_NONMOVABLE.

[Chris Dumez]

Created attachment 394518 [details]
Patch

[Saam Barati]

Comment on attachment 394518 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCJSValue.h:654
> +class EnsureStillAlive {

can we call this EnsureStillAliveScope or EnsureIsLiveScope?

[Keith Miller]

Comment on attachment 394518 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCJSValue.h:654
> +class EnsureStillAlive {

Can we call this EnsureAliveOnStack? It also seems like it would be useful to add a comment about were you would use this.

[Chris Dumez]

(In reply to Keith Miller from comment #28)
> Comment on attachment 394518 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394518&action=review
> 
> > Source/JavaScriptCore/runtime/JSCJSValue.h:654
> > +class EnsureStillAlive {
> 
> Can we call this EnsureAliveOnStack? It also seems like it would be useful
> to add a comment about were you would use this.

Can a JSC person help me come up with this comment. I'd be happy to rename the object and add the comment before landing.

[Geoffrey Garen]

> > Can we call this EnsureAliveOnStack? It also seems like it would be useful
> > to add a comment about were you would use this.
> 
> Can a JSC person help me come up with this comment. I'd be happy to rename
> the object and add the comment before landing.

Let's call this "EnsureStillAliveScope", since the function it calls is named "ensureStillAlive..." and it is bad when one thing becomes two.

For a comment, I would say:

Use EnsureStillAliveScope when you need to remove a JavaScript object from the DOM and then use it in the same scope. For example, a 'once' event listener needs to be removed from the DOM and then fired.

[Geoffrey Garen]

Or maybe:

Use EnsureStillAliveScope when you have a data structure that includes GC pointers, and you need to remove it from the DOM and then use it in the same scope. For example, a 'once' event listener needs to be removed from the DOM and then fired.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #31)
> Or maybe:
> 
> Use EnsureStillAliveScope when you have a data structure that includes GC
> pointers, and you need to remove it from the DOM and then use it in the same
> scope. For example, a 'once' event listener needs to be removed from the DOM
> and then fired.

In case it influences your decision, I am planning to add the following operator to this class in a follow-up so that I can use it in the generated JS DOM bindings:

operator JSValue() const { return m_value; }

[Keith Miller]

(In reply to Geoffrey Garen from comment #31)
> Or maybe:
> 
> Use EnsureStillAliveScope when you have a data structure that includes GC
> pointers, and you need to remove it from the DOM and then use it in the same
> scope. For example, a 'once' event listener needs to be removed from the DOM
> and then fired.

It might also be worth mentioning that you'll need this a GC object points to a malloc'd value and you want to use the malloced value after the last use of the GC'd object.

[Chris Dumez]

Created attachment 394521 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #32)
> (In reply to Geoffrey Garen from comment #31)
> > Or maybe:
> > 
> > Use EnsureStillAliveScope when you have a data structure that includes GC
> > pointers, and you need to remove it from the DOM and then use it in the same
> > scope. For example, a 'once' event listener needs to be removed from the DOM
> > and then fired.
> 
> In case it influences your decision, I am planning to add the following
> operator to this class in a follow-up so that I can use it in the generated
> JS DOM bindings:
> 
> operator JSValue() const { return m_value; }

If we go with a "*Scope" naming, then maybe it should not be an operator but rather an explicit value() getter?

[Geoffrey Garen]

> > operator JSValue() const { return m_value; }
> 
> If we go with a "*Scope" naming, then maybe it should not be an operator but
> rather an explicit value() getter?

Sure, I think that would look nice.

I guess it's also OK to drop "Scope". I was mostly interested in making the "ensure still alive" naming consistent.

[Chris Dumez]

Comment on attachment 394521 [details]
Patch

Looks like the output of some tests have changed. I will investigate.

[Chris Dumez]

Created attachment 394530 [details]
Patch

[EWS]

Attachment 394530 [details] does not apply

[Chris Dumez]

Created attachment 394544 [details]
Patch

[Chris Dumez]

Comment on attachment 394544 [details]
Patch

Clearing flags on attachment: 394544

Committed r259009: <https://trac.webkit.org/changeset/259009>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

The new test http/tests/inspector/network/har/har-page-aggressive-gc.html

is timing out on Mac wk1

History:
https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Finspector%2Fnetwork%2Fhar%2Fhar-page-aggressive-gc.html

[Chris Dumez]

(In reply to Truitt Savell from comment #43)
> The new test http/tests/inspector/network/har/har-page-aggressive-gc.html
> 
> is timing out on Mac wk1
> 
> History:
> https://results.webkit.org/?suite=layout-
> tests&test=http%2Ftests%2Finspector%2Fnetwork%2Fhar%2Fhar-page-aggressive-gc.
> html

Ok, I will investigate. It is a copy of http/tests/inspector/network/har/har-page.html but with aggressive GC enabled. So if http/tests/inspector/network/har/har-page.html is not timing out, then the test is likely just slow because of aggressive GC. Maybe I can make the GC less aggressive or we can mark the test as slow?

[Chris Dumez]

(In reply to Chris Dumez from comment #44)
> (In reply to Truitt Savell from comment #43)
> > The new test http/tests/inspector/network/har/har-page-aggressive-gc.html
> > 
> > is timing out on Mac wk1
> > 
> > History:
> > https://results.webkit.org/?suite=layout-
> > tests&test=http%2Ftests%2Finspector%2Fnetwork%2Fhar%2Fhar-page-aggressive-gc.
> > html
> 
> Ok, I will investigate. It is a copy of
> http/tests/inspector/network/har/har-page.html but with aggressive GC
> enabled. So if http/tests/inspector/network/har/har-page.html is not timing
> out, then the test is likely just slow because of aggressive GC. Maybe I can
> make the GC less aggressive or we can mark the test as slow?

Made gc less aggressive on the test via <https://trac.webkit.org/changeset/259048>.