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
Patch (38.09 KB, patch)
2020-03-24 16:34 PDT, Chris Dumez
no flags
Patch (38.22 KB, patch)
2020-03-24 16:38 PDT, Chris Dumez
no flags
Patch (37.89 KB, patch)
2020-03-24 17:42 PDT, Chris Dumez
no flags
Patch (38.72 KB, patch)
2020-03-25 10:01 PDT, Chris Dumez
no flags
Patch (39.03 KB, patch)
2020-03-25 10:59 PDT, Chris Dumez
no flags
Patch (39.35 KB, patch)
2020-03-25 11:54 PDT, Chris Dumez
no flags
Patch (43.31 KB, patch)
2020-03-25 12:55 PDT, Chris Dumez
no flags
Patch (43.34 KB, patch)
2020-03-25 14:15 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2020-03-24 14:56:11 PDT
Chris Dumez
Comment 2 2020-03-24 15:46:33 PDT
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
Chris Dumez
Comment 12 2020-03-24 16:38:04 PDT
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
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
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
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
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
EWS
Comment 39 2020-03-25 14:05:46 PDT
Chris Dumez
Comment 40 2020-03-25 14:15:10 PDT
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.