Bug 214345

Summary: [MSE][GStreamer] Break circular reference between SourceBufferPrivateGStreamer and AppendPipeline
Product: WebKit Reporter: Alicia Boya García <aboya>
Component: WebKitGTKAssignee: Alicia Boya García <aboya>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, cgarcia, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, menard, philipj, pnormand, sergio, vjaquez
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Alicia Boya García 2020-07-15 03:42:17 PDT 
SourceBufferPrivate is ref counted.

AppendPipeline is owned exclusively by SourceBufferPrivateGStreamer:
it's born and destroyed with it, managed by a never-moved unique_ptr.

AppendPipeline needs a reference to SourceBufferPrivateGStreamer to
notify it of essential events like samples having been parsed. This
used to be a Ref<>, thus creating a circular reference leak:

AppendPipeline is only destroyed in SourceBufferPrivateGStreamer
destructor. AppendPipeline holds ref counted reference to
SourceBufferPrivateGStreamer, therefore neither are destroyed.

This patch breaks the cycle by replacing the Ref<> in AppendPipeline
with a plain old reference. This is safe because
SourceBufferPrivateGStreamer owns, and therefore is alive at least
just as long as AppendPipeline.

As a consequence of not using Ref<>, the SourceBufferPrivateGStreamer
constructor does no longer need to relax the adoption requirements and
unique_ptr<AppendPipeline> can be replaced by a UniqueRef<>.
Comment 1 Alicia Boya García 2020-07-15 03:44:51 PDT 
Created attachment 404329 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2020-07-15 04:44:06 PDT 
Comment on attachment 404329 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:57
> -    Ref<SourceBufferPrivateGStreamer> sourceBufferPrivate() { return m_sourceBufferPrivate.get(); }
> +    SourceBufferPrivateGStreamer& sourceBufferPrivate() { return m_sourceBufferPrivate; }
>      GstCaps* appsinkCaps() { return m_appsinkCaps.get(); }
>      RefPtr<WebCore::TrackPrivateBase> track() { return m_track; }
>      MediaPlayerPrivateGStreamerMSE* playerPrivate() { return m_playerPrivate; }

At some point I think we should flag all these that just return object state as const. I think we would need a separate bug but if you feel like doing it, it would be great.

Besides, the original code should have returned a const Ref& as you won't touch the pointer but the object in it.
Comment 3 EWS 2020-07-15 05:56:27 PDT 
Committed r264392: <https://trac.webkit.org/changeset/264392>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404329 [details].