[Webkit-unassigned] [Bug 205460] Update TrackBase to store m_mediaElement as a WeakPtr

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=205460

--- Comment #16 from Ryosuke Niwa <rniwa at webkit.org> ---
(In reply to Darin Adler from comment #12)
> Comment on attachment 386241 [details]
> Patch
> 
> OK, now looking at the new patch and thinking about the long term for all of
> WebKit I think we are still probably doing this wrong. Generally speaking:
> 
> - Data members are long lived, so when the object is reference counted they
> should be WeakPtr, RefPtr, or Ref. It’s important that we use WeakPtr rather
> than RefPtr or Ref to avoid creating reference cycles and generally to avoid
> keeping objects alive too long. But we usually don’t want to use raw
> pointers or raw references because it’s too easy to get it wrong.

Agreed.

> - Local variables, arguments, and return values are less long lived. They
> should probably never be WeakPtr because WeakPtr is too expensive to create
> and destroy and it’s almost always fine to keep an object alive a little
> longer by using RefPtr instead. But for safety these should probably be
> RefPtr or Ref and not raw pointers or raw references.

There are cases we need to hold a reference to std::unique_ptr stored somewhere.
In those cases, we may not have a choice but to use WeakPtr.
I'm discussing with Geoff (Garren) on how to safe guard these cases.

> - It might be safe to sometimes use const& or && to avoid unnecessary
> copying. However, it may not be worth it. These are a bit dangerous because
> they depend on the lifetime of the place where the value is coming from. For
> example, if you return a const& to a data member and then the object it was
> returned from is deleted then there could be a problem if we still haven’t
> copied the value out of the referenced location. Not sure if this same
> argument applies to values passed in.

Right. I think the general rule is that whenever you call a member function on an object, then we should have a Ref/RefPtr to that object in the stack. It gets a bit more complicated with std::unique_ptr in a data member or WeakPtr since they can be cleared at any moment.

(In reply to Darin Adler from comment #14)
> (In reply to Chris Dumez from comment #13)
> > My opinions is that methods should take an an HTMLMediaElement& or a
> > HTMLMediaElement* in parameter. Then, here when saving it as a data member,
> > we would call makeWeakPtr().
> > It should be up to the class that store the data to decide which pointer
> > type to use for storage, not the call site IMHO.
> 
> I agree.
> 
> With one outstanding question:
> 
> How does that relate to Ryosuke's proposal to always use RefPtr and Ref for
> local variables from now on instead of raw pointers and references? Aren't
> arguments just local variables?

So I think the simple rule we've come up so far is that:
1. Caller keeps keep all arguments (including "this" pointer) alive or pass the ownership via Ref&& / RefPtr&&.
2. Arguments can be pointers or references because the caller keeps them alive.
3. Exception to (1) are arguments passed as pointers or references because (2) guarantees your caller will keep them alive.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20191220/cbb222a6/attachment.htm>