Bug 240202 - Explain how node reference counting works in Introduction.md
Summary: Explain how node reference counting works in Introduction.md
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-06 23:24 PDT by Ryosuke Niwa
Modified: 2022-05-07 23:51 PDT (History)
7 users (show)

See Also:


Attachments
Added (7.74 KB, patch)
2022-05-06 23:27 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated (8.41 KB, patch)
2022-05-07 00:08 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated (8.41 KB, patch)
2022-05-07 00:10 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (18.99 KB, patch)
2022-05-07 21:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2022-05-06 23:24:45 PDT
Explain how node reference counting works.
Comment 1 Ryosuke Niwa 2022-05-06 23:27:15 PDT
Created attachment 458990 [details]
Added
Comment 2 Ryosuke Niwa 2022-05-06 23:34:44 PDT
Comment on attachment 458990 [details]
Added

View in context: https://bugs.webkit.org/attachment.cgi?id=458990&action=review

> Introduction.md:1054
> +It's equally crucial to observe that keeping C++ Node object alive by storing `Ref` or `RefPtr`

This section may need to be revised once I wrote more about active DOM objects but it's good enough for now IMO.
Comment 3 Chris Dumez 2022-05-06 23:54:03 PDT
Comment on attachment 458990 [details]
Added

View in context: https://bugs.webkit.org/attachment.cgi?id=458990&action=review

Nice.

>> Introduction.md:1054
>> +It's equally crucial to observe that keeping C++ Node object alive by storing `Ref` or `RefPtr`
> 
> This section may need to be revised once I wrote more about active DOM objects but it's good enough for now IMO.

Seems like we should mention GCReachableRef as I believe this is a pretty common way to keep a JS Node wrapper alive?

Note that AFAIK, there is not much usage of ActiveDOMObject for Node types:
Source/WebCore/html/HTMLCanvasElement.idl: 
Source/WebCore/html/HTMLImageElement.idl: 
Source/WebCore/html/HTMLMarqueeElement.idl: 
Source/WebCore/html/HTMLMediaElement.idl: 
Source/WebCore/html/HTMLSourceElement.idl: 
Source/WebCore/html/HTMLTrackElement.idl:

Are these really the only ones that can fire events asynchronously?

Here is one example:
```
void HTMLTextFormControlElement::scheduleSelectEvent()
{
    document().eventLoop().queueTask(TaskSource::UserInteraction, [protectedThis = GCReachableRef { *this }] {
        protectedThis->dispatchEvent(Event::create(eventNames().selectEvent, Event::CanBubble::Yes, Event::IsCancelable::No));
    });
} 
```
HTMLTextFormControlElement isn't an ActiveDOMObject but seems to rely on GCReachableRef instead.

> Introduction.md:1071
> +The referencing node count of a document is the total number of Node's whose `ownerDocument` is the document.

Node's -> Nodes ?
Comment 4 Ryosuke Niwa 2022-05-07 00:05:24 PDT
(In reply to Chris Dumez from comment #3)
> Comment on attachment 458990 [details]
> Added
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458990&action=review
> 
> Nice.
> 
> >> Introduction.md:1054
> >> +It's equally crucial to observe that keeping C++ Node object alive by storing `Ref` or `RefPtr`
> > 
> > This section may need to be revised once I wrote more about active DOM objects but it's good enough for now IMO.
> 
> Seems like we should mention GCReachableRef as I believe this is a pretty
> common way to keep a JS Node wrapper alive?

Oh, that's a good point. Totally forgot about that.
Comment 5 Ryosuke Niwa 2022-05-07 00:08:35 PDT
Created attachment 458991 [details]
Updated
Comment 6 Ryosuke Niwa 2022-05-07 00:10:24 PDT
Created attachment 458992 [details]
Updated
Comment 7 Chris Dumez 2022-05-07 10:38:21 PDT
Comment on attachment 458992 [details]
Updated

r=me
Comment 8 Ryosuke Niwa 2022-05-07 10:58:55 PDT
Comment on attachment 458992 [details]
Updated

Clearing flags on attachment: 458992

Committed r293949 (250395@trunk): <https://commits.webkit.org/250395@trunk>
Comment 9 Ryosuke Niwa 2022-05-07 10:58:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2022-05-07 10:59:15 PDT
<rdar://problem/92910315>
Comment 11 Ryosuke Niwa 2022-05-07 21:38:49 PDT
Reopening to attach new patch.
Comment 12 Ryosuke Niwa 2022-05-07 21:38:52 PDT
Created attachment 459005 [details]
Patch for landing
Comment 13 Ryosuke Niwa 2022-05-07 21:39:18 PDT
Comment on attachment 459005 [details]
Patch for landing

Wrong bug.