Bug 226158 - Add CheckedPtr
Summary: Add CheckedPtr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-23 13:51 PDT by Ryosuke Niwa
Modified: 2022-01-27 10:25 PST (History)
8 users (show)

See Also:


Attachments
Patch (29.07 KB, patch)
2021-05-23 18:59 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (10.88 KB, patch)
2021-05-23 21:32 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (27.26 KB, patch)
2021-05-24 00:36 PDT, Ryosuke Niwa
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2021-05-23 13:51:48 PDT
Add a new smart pointer type, CheckedPtr, which increments & decrements the internal counter of an object like RefPtr
but simply release asserts that there is no outstanding pointer (i.e. count is 0) instead of keeping the object alive
when the count is greater than 0.
Comment 1 Ryosuke Niwa 2021-05-23 18:59:20 PDT
Created attachment 429494 [details]
Patch
Comment 2 Ryosuke Niwa 2021-05-23 21:32:39 PDT
Created attachment 429500 [details]
Patch
Comment 3 Darin Adler 2021-05-23 21:41:48 PDT
Comment on attachment 429500 [details]
Patch

The name CheckedPtr doesn’t seem descriptive enough. But maybe I’ll get used to it?
Comment 4 Darin Adler 2021-05-23 21:41:49 PDT
Comment on attachment 429500 [details]
Patch

The name CheckedPtr doesn’t seem descriptive enough. But maybe I’ll get used to it?
Comment 5 Ryosuke Niwa 2021-05-24 00:36:44 PDT
Created attachment 429508 [details]
Patch
Comment 6 Fujii Hironori 2021-05-24 14:18:09 PDT
This seems an unique concept. I'd like to know the use case of this.
Which existing classes do you plan to apply CanMakeCheckedPtr?
Comment 7 Chris Dumez 2021-05-24 14:21:44 PDT
(In reply to Fujii Hironori from comment #6)
> This seems an unique concept. I'd like to know the use case of this.
> Which existing classes do you plan to apply CanMakeCheckedPtr?

I admit I am also a bit confused about what this smart pointer does (even after reading the changelog) and how we plan to use this. I am sure I'll get it once I see it used in actual code but for now it is difficult.
Comment 8 Ryosuke Niwa 2021-05-24 17:02:12 PDT
(In reply to Chris Dumez from comment #7)
> (In reply to Fujii Hironori from comment #6)
> > This seems an unique concept. I'd like to know the use case of this.
> > Which existing classes do you plan to apply CanMakeCheckedPtr?
> 
> I admit I am also a bit confused about what this smart pointer does (even
> after reading the changelog) and how we plan to use this. I am sure I'll get
> it once I see it used in actual code but for now it is difficult.

CheckedPtr is useful when we have an object which is kept alive by unique_ptr or it's simply a member of another class. Right now, there is no a way to safely return a pointer to such an object because the object which holds unique_ptr could clear it. In the case of a member class variable, the whole object could be deleted. It's not trivial to find the "real" owner and ensure that the owner outlives all references to it since unique_ptr and class variable membership could nest.
Comment 9 Chris Dumez 2021-05-25 08:24:52 PDT
(In reply to Ryosuke Niwa from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to Fujii Hironori from comment #6)
> > > This seems an unique concept. I'd like to know the use case of this.
> > > Which existing classes do you plan to apply CanMakeCheckedPtr?
> > 
> > I admit I am also a bit confused about what this smart pointer does (even
> > after reading the changelog) and how we plan to use this. I am sure I'll get
> > it once I see it used in actual code but for now it is difficult.
> 
> CheckedPtr is useful when we have an object which is kept alive by
> unique_ptr or it's simply a member of another class. Right now, there is no
> a way to safely return a pointer to such an object because the object which
> holds unique_ptr could clear it. In the case of a member class variable, the
> whole object could be deleted. It's not trivial to find the "real" owner and
> ensure that the owner outlives all references to it since unique_ptr and
> class variable membership could nest.

One pattern that already exists in the code is to use UniqueRef<> for the data member, and have the data member forward its RefCounting to its owning class (for example Document & DOMImplementation). I am not sure if it can apply to the cases you had in mind as you said unique_ptr.
Comment 10 Ryosuke Niwa 2021-05-25 12:37:39 PDT
(In reply to Chris Dumez from comment #9)
>
> One pattern that already exists in the code is to use UniqueRef<> for the
> data member, and have the data member forward its RefCounting to its owning
> class (for example Document & DOMImplementation). I am not sure if it can
> apply to the cases you had in mind as you said unique_ptr.

That works so long as such a forwarding is possible & it's permissible, which isn't always the case.

Consider the following scenario.

class A : public RefCounted<A> {
...
   SharedMember& sharedMember() { return m_sharedMember; }
...
private:
    unsigned m_value;
    SharedMember m_sharedMember;
}

class B : public RefCounted<B> {
...
   SharedMember& sharedMember() { return m_sharedMember; }
...
private:
    String m_name;
    String m_value;
    SharedMember m_sharedMember;
}

In this scenario, there isn't really a way for SharedMember to easily forward ref/deref to A and B without A & B having to subclass SharedMember with their virtual ref/deref overrides.
Comment 11 Radar WebKit Bug Importer 2021-05-30 13:52:16 PDT
<rdar://problem/78667625>
Comment 12 Antti Koivisto 2021-06-02 01:07:50 PDT
Comment on attachment 429508 [details]
Patch

Seems like a good idea to me. It can replace existing defensive use of WeakPtr in a number of places (see bug 226369 for example) while having tighter semantics and being more efficient.
Comment 13 Ryosuke Niwa 2021-06-02 01:35:37 PDT
(In reply to Antti Koivisto from comment #12)
> Comment on attachment 429508 [details]
> Patch
> 
> Seems like a good idea to me. It can replace existing defensive use of
> WeakPtr in a number of places (see bug 226369 for example) while having
> tighter semantics and being more efficient.

Yeah, I suspect many uses of WeakPtr can be replaced with CheckedPtr now that we have it.
Comment 14 Ryosuke Niwa 2021-06-02 01:43:17 PDT
Committed r278344 (238376@main): <https://commits.webkit.org/238376@main>
Comment 15 Antti Koivisto 2021-06-02 01:48:48 PDT
Comment on attachment 429508 [details]
Patch

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

> Source/WTF/wtf/CheckedPtr.h:207
> +    uint16_t m_count { 0 };

Any particular reason to limit this to a relatively small count?
Comment 16 Ryosuke Niwa 2021-06-02 01:53:26 PDT
(In reply to Antti Koivisto from comment #15)
> Comment on attachment 429508 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429508&action=review
> 
> > Source/WTF/wtf/CheckedPtr.h:207
> > +    uint16_t m_count { 0 };
> 
> Any particular reason to limit this to a relatively small count?

Well, my reasoning is that I'd expect CheckedPtr instance to have a relatively small number of instances since unlike Ref/RefPtr, you're relying on the correct object lifetime management across multiple objects. I guess it's possible to have something like Vector<T> where T all refers to the same CheckedPtr object but that sounds more of an edge case than anything.
Comment 17 Antti Koivisto 2021-06-02 02:20:16 PDT
Atomic version of the base class would also be good to have.
Comment 18 Ryosuke Niwa 2021-06-02 02:38:52 PDT
(In reply to Antti Koivisto from comment #17)
> Atomic version of the base class would also be good to have.

Right, also CheckedRef.