WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
209504
Event listeners registered with 'once' option may get garbage collected too soon
https://bugs.webkit.org/show_bug.cgi?id=209504
Summary
Event listeners registered with 'once' option may get garbage collected too soon
Chris Dumez
Reported
2020-03-24 14:55:56 PDT
Event listeners registered with 'once' option may get garbage collected too soon, before they get a chance to run.
Attachments
Patch
(5.21 KB, patch)
2020-03-24 15:46 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(38.09 KB, patch)
2020-03-24 16:34 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(38.22 KB, patch)
2020-03-24 16:38 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(37.89 KB, patch)
2020-03-24 17:42 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(38.72 KB, patch)
2020-03-25 10:01 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(39.03 KB, patch)
2020-03-25 10:59 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(39.35 KB, patch)
2020-03-25 11:54 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(43.31 KB, patch)
2020-03-25 12:55 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(43.34 KB, patch)
2020-03-25 14:15 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2020-03-24 14:56:11 PDT
<
rdar://problem/60541567
>
Chris Dumez
Comment 2
2020-03-24 15:46:33 PDT
Created
attachment 394427
[details]
Patch
Saam Barati
Comment 3
2020-03-24 15:48:42 PDT
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 4
2020-03-24 15:50:01 PDT
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 5
2020-03-24 15:52:36 PDT
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 6
2020-03-24 15:53:59 PDT
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
Comment 7
2020-03-24 15:54:18 PDT
(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
Comment 8
2020-03-24 15:55:01 PDT
(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
Comment 9
2020-03-24 15:56:06 PDT
(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
Comment 10
2020-03-24 16:00:28 PDT
(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
Comment 11
2020-03-24 16:34:46 PDT
Created
attachment 394436
[details]
Patch
Chris Dumez
Comment 12
2020-03-24 16:38:04 PDT
Created
attachment 394438
[details]
Patch
Saam Barati
Comment 13
2020-03-24 16:44:12 PDT
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 14
2020-03-24 16:47:41 PDT
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 15
2020-03-24 17:12:07 PDT
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
Comment 16
2020-03-24 17:23:58 PDT
(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 17
2020-03-24 17:24:19 PDT
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
Comment 18
2020-03-24 17:30:11 PDT
(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
Comment 19
2020-03-24 17:42:34 PDT
Created
attachment 394450
[details]
Patch
Chris Dumez
Comment 20
2020-03-24 17:43:15 PDT
(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 21
2020-03-24 21:12:45 PDT
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
Comment 22
2020-03-24 23:36:04 PDT
Also, would this crash in a more reduced test when slowPathAllocsBetweenGCs is used?
Chris Dumez
Comment 23
2020-03-25 08:39:49 PDT
(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
Comment 24
2020-03-25 10:01:46 PDT
Created
attachment 394510
[details]
Patch
Yusuke Suzuki
Comment 25
2020-03-25 10:49:33 PDT
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
Comment 26
2020-03-25 10:59:06 PDT
Created
attachment 394518
[details]
Patch
Saam Barati
Comment 27
2020-03-25 11:20:38 PDT
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 28
2020-03-25 11:21:20 PDT
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
Comment 29
2020-03-25 11:24:23 PDT
(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
Comment 30
2020-03-25 11:40:29 PDT
> > 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
Comment 31
2020-03-25 11:44:56 PDT
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
Comment 32
2020-03-25 11:46:55 PDT
(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
Comment 33
2020-03-25 11:51:31 PDT
(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
Comment 34
2020-03-25 11:54:34 PDT
Created
attachment 394521
[details]
Patch
Chris Dumez
Comment 35
2020-03-25 11:56:04 PDT
(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
Comment 36
2020-03-25 12:14:19 PDT
> > 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 37
2020-03-25 12:41:26 PDT
Comment on
attachment 394521
[details]
Patch Looks like the output of some tests have changed. I will investigate.
Chris Dumez
Comment 38
2020-03-25 12:55:20 PDT
Created
attachment 394530
[details]
Patch
EWS
Comment 39
2020-03-25 14:05:46 PDT
Attachment 394530
[details]
does not apply
Chris Dumez
Comment 40
2020-03-25 14:15:10 PDT
Created
attachment 394544
[details]
Patch
Chris Dumez
Comment 41
2020-03-25 14:16:20 PDT
Comment on
attachment 394544
[details]
Patch Clearing flags on attachment: 394544 Committed
r259009
: <
https://trac.webkit.org/changeset/259009
>
Chris Dumez
Comment 42
2020-03-25 14:16:23 PDT
All reviewed patches have been landed. Closing bug.
Truitt Savell
Comment 43
2020-03-26 09:37:41 PDT
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
Comment 44
2020-03-26 09:43:00 PDT
(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
Comment 45
2020-03-26 09:52:41 PDT
(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
>.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug