Bug 226158 – Add CheckedPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 429494 [details]
Patch

[Ryosuke Niwa]

Created attachment 429500 [details]
Patch

[Darin Adler]

Comment on attachment 429500 [details]
Patch

The name CheckedPtr doesn’t seem descriptive enough. But maybe I’ll get used to it?

[Darin Adler]

Comment on attachment 429500 [details]
Patch

The name CheckedPtr doesn’t seem descriptive enough. But maybe I’ll get used to it?

[Ryosuke Niwa]

Created attachment 429508 [details]
Patch

[Fujii Hironori]

This seems an unique concept. I'd like to know the use case of this.
Which existing classes do you plan to apply CanMakeCheckedPtr?

[Chris Dumez]

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

[Ryosuke Niwa]

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

[Chris Dumez]

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

[Ryosuke Niwa]

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

[Radar WebKit Bug Importer]

<rdar://problem/78667625>

[Antti Koivisto]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Committed r278344 (238376@main): <https://commits.webkit.org/238376@main>

[Antti Koivisto]

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?

[Ryosuke Niwa]

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

[Antti Koivisto]

Atomic version of the base class would also be good to have.

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #17)
> Atomic version of the base class would also be good to have.

Right, also CheckedRef.