[webkit-reviews] review canceled: [Bug 197558] REGRESSION(r243197): [GStreamer] Web process hangs when scrolling twitter timeline which contains HLS videos : [Attachment 369484] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has canceled  review:
Bug 197558: REGRESSION(r243197): [GStreamer] Web process hangs when scrolling
twitter timeline which contains HLS videos
https://bugs.webkit.org/show_bug.cgi?id=197558

Attachment 369484: Patch

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




--- Comment #12 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 369484
  --> https://bugs.webkit.org/attachment.cgi?id=369484
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:91
> +	       m_synchronousNotificationCondition.notifyOne();

While answering your comment below I realized this could be an issue as well
because of having now shared lock and condition. Imagine you have two pending
notifications, you could be waking the wrong one here, couldn't you? I think a
conditional wait below could fix this as well, couldn't it?

>>>> Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:97
>>>> +		 
m_synchronousNotificationCondition.wait(m_synchronousNotificationMutex);
>>> 
>>> Considering we are waking up everybody below, shouldn't we check for the
notification flag to be still active? Maybe a conditional wait?
>> 
>> Hum, I'm not sure ... I'll try.
> 
> Actually I'm not sure I understood what you meant. By notification flag, do
you mean m_pendingNotifications? If so, I think the existing !isMainThread()
test before the wait is more than enough already :)

Yes, I mean to check the m_pendingNotifications flags. The use case I am
thinking of is you cancel a certain notification type but then you get notified
for all other types and wake them here and shouldn't they be still waiting?