Bug 240204 – Add helper functions to queue task on Node with GCReachableRef

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ryosuke Niwa]

Add helper functions to queue an event look task for Node using GCReachableRef.

[Ryosuke Niwa]

Created attachment 458995 [details]
Patch

[Ryosuke Niwa]

Created attachment 458996 [details]
Patch

[Chris Dumez]

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.

[Ryosuke Niwa]

(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.

[Chris Dumez]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 459006 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/92918315>