[webkit-reviews] review granted: [Bug 240204] Add helper functions to queue task on Node with GCReachableRef : [Attachment 458996] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 240204: Add helper functions to queue task on Node with GCReachableRef
https://bugs.webkit.org/show_bug.cgi?id=240204

Attachment 458996: Patch

https://bugs.webkit.org/attachment.cgi?id=458996&action=review




--- Comment #5 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 458996
  --> https://bugs.webkit.org/attachment.cgi?id=458996
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.