Bug 201222 – [GStreamer] Do not ref the player count from background threads.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Charlie Turner]

[GStreamer] Do not ref the player count from background threads.

[Charlie Turner]

Created attachment 377451 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 377451 [details]
Patch

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

Good! Only a couple of things to tweak before landing.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:353
> +        if (m_cdmAttachmentSemaphore.waitFor(4_s)
> +            && m_notifier->isValid() // Check the player is not being destroyed.
> +            && !m_cdmInstance->keySystem().isEmpty()) {

We need to consider here, to avoid any other crashes, that the semaphore can be either signaled by the destructor or that the timeout can be hit, both without the instance being set. It looks like the third clause with crash is m_cdmInstance is still null.

Besides, all this can be one line.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:308
> +    Lock m_protectionMutex; // Guards access to m_handledProtectionEvents

.

[Xabier Rodríguez Calvar]

Comment on attachment 377451 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:307
> +

And you can remove this line

[Charlie Turner]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 377451 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377451&action=review
> 
> Good! Only a couple of things to tweak before landing.
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:353
> > +        if (m_cdmAttachmentSemaphore.waitFor(4_s)
> > +            && m_notifier->isValid() // Check the player is not being destroyed.
> > +            && !m_cdmInstance->keySystem().isEmpty()) {
> 
> We need to consider here, to avoid any other crashes, that the semaphore can
> be either signaled by the destructor or that the timeout can be hit, both
> without the instance being set. It looks like the third clause with crash is
> m_cdmInstance is still null.

There are two cases,
  1) Signal from cdmInstanceAttached. In this case, we know the cdmInstance is not null
  2) Signal from dtor, in this case the notifier will have been cancelled in the dtor already, and the second conditional will bail out.

So we don't need a further check imo.

> Besides, all this can be one line.

We then lose the informative comment about why isValid() is there, it's much easier to read conditionals vertically aligned like this and to comment on their conditions.

[Xabier Rodríguez Calvar]

(In reply to Charlie Turner from comment #4)
> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:353
> > > +        if (m_cdmAttachmentSemaphore.waitFor(4_s)
> > > +            && m_notifier->isValid() // Check the player is not being destroyed.
> > > +            && !m_cdmInstance->keySystem().isEmpty()) {
> > 
> > We need to consider here, to avoid any other crashes, that the semaphore can
> > be either signaled by the destructor or that the timeout can be hit, both
> > without the instance being set. It looks like the third clause with crash is
> > m_cdmInstance is still null.
> 
> There are two cases,
>   1) Signal from cdmInstanceAttached. In this case, we know the cdmInstance
> is not null
>   2) Signal from dtor, in this case the notifier will have been cancelled in
> the dtor already, and the second conditional will bail out.

3) waitFor timeout runs out without being signalled from anywhere.


> So we don't need a further check imo.
> 
> > Besides, all this can be one line.
> 
> We then lose the informative comment about why isValid() is there, it's much
> easier to read conditionals vertically aligned like this and to comment on
> their conditions.

Right.

[Charlie Turner]

(In reply to Xabier Rodríguez Calvar from comment #5)
> (In reply to Charlie Turner from comment #4)
> > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:353
> > > > +        if (m_cdmAttachmentSemaphore.waitFor(4_s)
> > > > +            && m_notifier->isValid() // Check the player is not being destroyed.
> > > > +            && !m_cdmInstance->keySystem().isEmpty()) {
> > > 
> > > We need to consider here, to avoid any other crashes, that the semaphore can
> > > be either signaled by the destructor or that the timeout can be hit, both
> > > without the instance being set. It looks like the third clause with crash is
> > > m_cdmInstance is still null.
> > 
> > There are two cases,
> >   1) Signal from cdmInstanceAttached. In this case, we know the cdmInstance
> > is not null
> >   2) Signal from dtor, in this case the notifier will have been cancelled in
> > the dtor already, and the second conditional will bail out.
> 
> 3) waitFor timeout runs out without being signalled from anywhere.

And in that case, waitFor() returns false.

> 
> 
> > So we don't need a further check imo.
> > 
> > > Besides, all this can be one line.
> > 
> > We then lose the informative comment about why isValid() is there, it's much
> > easier to read conditionals vertically aligned like this and to comment on
> > their conditions.
> 
> Right.

[Xabier Rodríguez Calvar]

We good then.

[Charlie Turner]

Created attachment 377692 [details]
Patch for landing

[Charlie Turner]

Created attachment 377693 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 377693 [details]
Patch for landing

Clearing flags on attachment: 377693

Committed r249321: <https://trac.webkit.org/changeset/249321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.