[webkit-reviews] review granted: [Bug 201222] [GStreamer] Do not ref the player count from background threads. : [Attachment 377451] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Charlie Turner
<cturner at igalia.com>'s request for review:
Bug 201222: [GStreamer] Do not ref the player count from background threads.
https://bugs.webkit.org/show_bug.cgi?id=201222

Attachment 377451: Patch

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




--- Comment #2 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 377451
  --> https://bugs.webkit.org/attachment.cgi?id=377451
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:30
8
> +    Lock m_protectionMutex; // Guards access to m_handledProtectionEvents

.