Bug 205460 – Update TrackBase to store m_mediaElement as a WeakPtr

• Status: REOPENED
• Component: Media

[Doug Kelly]

Update TrackBase and associated classes to store their HTMLMediaElement as a WeakPtr instead of a raw pointer.  This is optional for these classes, but if set, should be a valid HTMLMediaElement.

[Doug Kelly]

Created attachment 386116 [details]
Patch

[Ryosuke Niwa]

Committed r253809: <https://trac.webkit.org/changeset/253809>

[Radar WebKit Bug Importer]

<rdar://problem/58099071>

[Darin Adler]

Comment on attachment 386116 [details]
Patch

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

Copying a WeakPtr is expensive so it’s better if we transfer ownership when we can rather than making copies.

> Source/WebCore/html/track/AudioTrack.h:68
> +    void setMediaElement(WeakPtr<HTMLMediaElement>) override;

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/AudioTrackList.h:38
> +    static Ref<AudioTrackList> create(WeakPtr<HTMLMediaElement> owner, ScriptExecutionContext* context)

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/AudioTrackList.h:54
> +    AudioTrackList(WeakPtr<HTMLMediaElement>, ScriptExecutionContext*);

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/InbandTextTrack.h:52
> +    void setMediaElement(WeakPtr<HTMLMediaElement>) override;

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/TextTrackList.h:39
> +    static Ref<TextTrackList> create(WeakPtr<HTMLMediaElement> element, ScriptExecutionContext* context)

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/TextTrackList.h:63
> +    TextTrackList(WeakPtr<HTMLMediaElement>, ScriptExecutionContext*);

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/TrackBase.h:52
> +    virtual void setMediaElement(WeakPtr<HTMLMediaElement>);

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/TrackBase.h:53
> +    WeakPtr<HTMLMediaElement> mediaElement() { return m_mediaElement; }

Might be better to use const WeakPtr& for the return type. Or even leave the return type a raw pointer. Not sure which idiom is best. Returning a copy of the WeakPtr is costly.

> Source/WebCore/html/track/TrackBase.h:86
> +    WeakPtr<HTMLMediaElement> m_mediaElement { nullptr };

No need for the "{ nullptr }" when it’s a smart pointer. Only raw pointers need that initialization.

> Source/WebCore/html/track/TrackListBase.h:60
> +    WeakPtr<HTMLMediaElement> mediaElement() const { return m_element; }

Might be better to use const WeakPtr& for the return type. Or even leave the return type a raw pointer. Not sure which idiom is best. Returning a copy of the WeakPtr is costly.

> Source/WebCore/html/track/TrackListBase.h:69
> +    TrackListBase(WeakPtr<HTMLMediaElement>, ScriptExecutionContext*);

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/VideoTrack.h:76
> +    void setMediaElement(WeakPtr<HTMLMediaElement>) override;

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/VideoTrackList.h:38
> +    static Ref<VideoTrackList> create(WeakPtr<HTMLMediaElement> owner, ScriptExecutionContext* context)

This should be WeakPtr&& so we can transfer ownership.

> Source/WebCore/html/track/VideoTrackList.h:55
> +    VideoTrackList(WeakPtr<HTMLMediaElement>, ScriptExecutionContext*);

This should be WeakPtr&& so we can transfer ownership.

[Doug Kelly]

Reopened to address Darin's comments

[Darin Adler]

Will need to use WTFMove in various places along with changing the type to WeakPtr&& to get the moving to happen, but it should not be super-challenging.

[Doug Kelly]

Thanks for the tip -- Jer had also mentioned some of this in passing, and I just didn't put it into the review here before it landed.

[Doug Kelly]

Created attachment 386231 [details]
Patch

[Jer Noble]

Comment on attachment 386231 [details]
Patch

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

r=me with a lone nit

> Source/WebCore/html/track/TrackListBase.cpp:44
>      , m_element(element)

This could be a WTFMove();

[Doug Kelly]

Created attachment 386233 [details]
Patch

[Doug Kelly]

Created attachment 386241 [details]
Patch

[Darin Adler]

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.

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

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

I am sure we don’t want to create a temporary WeakPtr and then copy it into a permanent WeakPtr. That is excessively inefficient. But I am not sure that WeakPtr&& is right either. What is the right type for arguments in cases like this?

I’d love to just supply the answer, but I am not sure!

[Chris Dumez]

Comment on attachment 386241 [details]
Patch

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

> Source/WebCore/html/track/TrackListBase.cpp:44
> +    , m_element(WTFMove(element))

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.

[Darin Adler]

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

[Chris Dumez]

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

I admit I have not usually been following Ryosuke's proposal myself. I will let Ryosuke comment on this then :) Maybe we'd pass a Ref<>&& / RefPtr<>&& in parameter and convert that to a WeakPtr internally as needed for storage?

[Ryosuke Niwa]

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

[Ryosuke Niwa]

The "proposal" for the rule is still in flux but hoping to post something on webkit-dev soon.

[Darin Adler]

So then I guess the arguments should be raw pointers or references and the return value types from the getters should be RefPtr.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #18)
> So then I guess the arguments should be raw pointers or references and the
> return value types from the getters should be RefPtr.

It could be or the caller of those functions could make RefPtr/Ref. One additional exception we’re considering is to allow chaining like: m_document.frame().page() in the case member function is trivial.