Add helper functions to queue an event look task for Node using GCReachableRef.
Created attachment 458995 [details] Patch
Created attachment 458996 [details] Patch
Comment on attachment 458996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458996&action=review I like the idea but have one concern. > Source/WebCore/dom/Node.cpp:1316 > + document().eventLoop().queueTask(source, [protectedThis = GCReachableRef(*this), task = WTFMove(task)] () { Shouldn't this capture `protectedThis = Ref { *this }` too (like ActiveDOMObject::queueTaskKeepingObjectAlive() does)? The function name says it is keeping the Node alive. Currently, what it does is keeping the JS wrapper alive (which it should definitely do). It is true that if the JS wrapper is alive then the impl object would be alive too. However, we've run into trouble making such assumptions in the past because there may not be a wrapper alive when calling queueTaskKeepingThisNodeAlive() and thus constructing the GCReachableRef. Right now, I think the function name is a little bit of a lie. It keeps this node's wrapper alive, not necessarily this node. > Source/WebCore/dom/Node.cpp:1323 > + queueTaskKeepingThisNodeAlive(source, [protectedThis = Ref { *this }, event = WTFMove(event)]() { Assuming queueTaskKeepingThisNodeAlive() is updated to capture protectedThis, I suggest we only capture |this| here. > Source/WebCore/html/HTMLDetailsElement.cpp:150 > + queueTaskKeepingThisNodeAlive(TaskSource::DOMManipulation, [protectedThis = Ref { *this }] { Seems like we shouldn't need to capture protectedThis when calling a function named queueTaskKeepingThisNodeAlive(), if the function is really keeping the Node alive, capturing |this| should suffice. > Source/WebCore/html/HTMLDialogElement.cpp:116 > + queueTaskKeepingThisNodeAlive(TaskSource::UserInteraction, [protectedThis = Ref { *this }] { ditto.
(In reply to Chris Dumez from comment #3) > Comment on attachment 458996 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458996&action=review > > I like the idea but have one concern. > > > Source/WebCore/dom/Node.cpp:1316 > > + document().eventLoop().queueTask(source, [protectedThis = GCReachableRef(*this), task = WTFMove(task)] () { > > Shouldn't this capture `protectedThis = Ref { *this }` too (like > ActiveDOMObject::queueTaskKeepingObjectAlive() does)? The function name says > it is keeping the Node alive. Currently, what it does is keeping the JS > wrapper alive (which it should definitely do). > > It is true that if the JS wrapper is alive then the impl object would be > alive too. However, we've run into trouble making such assumptions in the > past because there may not be a wrapper alive when calling > queueTaskKeepingThisNodeAlive() and thus constructing the GCReachableRef. > > Right now, I think the function name is a little bit of a lie. It keeps this > node's wrapper alive, not necessarily this node. Oh, GCReachableRef<Node> contains RefPtr<Node> so it does BOTH. > > Source/WebCore/dom/Node.cpp:1323 > > + queueTaskKeepingThisNodeAlive(source, [protectedThis = Ref { *this }, event = WTFMove(event)]() { > > Assuming queueTaskKeepingThisNodeAlive() is updated to capture > protectedThis, I suggest we only capture |this| here. Yeah, I debated between the two. If we have a compiler validation of captured variables, also explicitly ref'ing here makes sense. As is, I'm triggering a redundant ref call.
Comment on attachment 458996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458996&action=review I'd prefer if we avoided the unnecessary Ref<> captures personally. >>> Source/WebCore/dom/Node.cpp:1316 >>> + document().eventLoop().queueTask(source, [protectedThis = GCReachableRef(*this), task = WTFMove(task)] () { >> >> Shouldn't this capture `protectedThis = Ref { *this }` too (like ActiveDOMObject::queueTaskKeepingObjectAlive() does)? The function name says it is keeping the Node alive. Currently, what it does is keeping the JS wrapper alive (which it should definitely do). >> >> It is true that if the JS wrapper is alive then the impl object would be alive too. However, we've run into trouble making such assumptions in the past because there may not be a wrapper alive when calling queueTaskKeepingThisNodeAlive() and thus constructing the GCReachableRef. >> >> Right now, I think the function name is a little bit of a lie. It keeps this node's wrapper alive, not necessarily this node. > > Oh, GCReachableRef<Node> contains RefPtr<Node> so it does BOTH. Oh, you're totally right, I missed that when I looked at the class earlier.
(In reply to Chris Dumez from comment #5) > Comment on attachment 458996 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458996&action=review > > I'd prefer if we avoided the unnecessary Ref<> captures personally. Okay, let's just capture "this" for now.
Created attachment 459006 [details] Patch for landing
Committed r293955 (250401@main): <https://commits.webkit.org/250401@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 459006 [details].
<rdar://problem/92918315>