Summary: | Event listeners registered with 'once' option may get garbage collected too soon | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||
Component: | Bindings | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | ap, cdumez, esprehn+autocc, ews-watchlist, ggaren, hi, joepeck, kangil.han, keith_miller, mark.lam, msaboff, rniwa, saam, tsavell, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=209445 | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 209552 | ||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2020-03-24 14:55:56 PDT
Created attachment 394427 [details]
Patch
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? 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? 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? 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? (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 (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 :-) (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. (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 Created attachment 394436 [details]
Patch
Created attachment 394438 [details]
Patch
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? 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 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. (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. 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); (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. Created attachment 394450 [details]
Patch
(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. 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. Also, would this crash in a more reduced test when slowPathAllocsBetweenGCs is used? (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> Created attachment 394510 [details]
Patch
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. Created attachment 394518 [details]
Patch
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? 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. (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. > > 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.
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 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; } (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. Created attachment 394521 [details]
Patch
(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? > > 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.
Comment on attachment 394521 [details]
Patch
Looks like the output of some tests have changed. I will investigate.
Created attachment 394530 [details]
Patch
Attachment 394530 [details] does not apply
Created attachment 394544 [details]
Patch
Comment on attachment 394544 [details] Patch Clearing flags on attachment: 394544 Committed r259009: <https://trac.webkit.org/changeset/259009> All reviewed patches have been landed. Closing bug. 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 (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? (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>. |