Bug 225650 – [WTF][GStreamer] Add RAII lockers for 3rd party locks

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Alicia Boya García]

This patch introduces RAII locker classes that wrap GST_OBJECT_LOCK
and GST_PAD_STREAM_LOCK to match the style, safety and convenience of
locks from WTF.

This patch introduces no behavior changes.

[Alicia Boya García]

Created attachment 428265 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 428265 [details]
Patch

Art.

[Xabier Rodríguez Calvar]

Comment on attachment 428265 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:323
> +    explicit ExternalLocker(T* lockable) : m_lockable(lockable) { lock(); }

I wonder if we should ASSERT(lockable)...

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:332
> +    T* lockable() { return m_lockable; }
> +
> +    explicit operator bool() const { return !!m_lockable; }

Why do you need these?

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:387
> +inline GstObjectLocker holdGstObjectLock(void* gstObject) WARN_UNUSED_RETURN;
> +inline GstObjectLocker holdGstObjectLock(void* gstObject)
> +{
> +    return GstObjectLocker(gstObject);
> +}

Can't we collapse declaration and implementation?

Independently of asserting above, it could be interesting ASSERT(GST_IS_OBJECT(gstObject))

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:393
> +inline GstPadStreamLocker holdGstPadStreamLock(GstPad*) WARN_UNUSED_RETURN;
> +inline GstPadStreamLocker holdGstPadStreamLock(GstPad* pad)
> +{
> +    return GstPadStreamLocker(pad);
> +}

Ditto.

Ditto.

[Alicia Boya García]

Comment on attachment 428265 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:323
>> +    explicit ExternalLocker(T* lockable) : m_lockable(lockable) { lock(); }
> 
> I wonder if we should ASSERT(lockable)...

We definitely can.

>> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:332
>> +    explicit operator bool() const { return !!m_lockable; }
> 
> Why do you need these?

It's here just for completeness to match WTF::Locker, even though we don't need all the fancy features at the moment (such as moving locks). This tells you if this locker still holds a lock: it doesn't if it has been moved.

>> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:387
>> +}
> 
> Can't we collapse declaration and implementation?
> 
> Independently of asserting above, it could be interesting ASSERT(GST_IS_OBJECT(gstObject))

We can't collapse declaration and implementation because WARN_UNUSED_RETURN can only be used in declarations, not in function definitions.

[Xabier Rodríguez Calvar]

Comment on attachment 428265 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:332
>>> +    explicit operator bool() const { return !!m_lockable; }
>> 
>> Why do you need these?
> 
> It's here just for completeness to match WTF::Locker, even though we don't need all the fancy features at the moment (such as moving locks). This tells you if this locker still holds a lock: it doesn't if it has been moved.

You can keep them.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:384
> +// We can't pass macros as template parameters, so we need to wrap them in inline functions.
> +inline void gstObjectLock(void* object) { GST_OBJECT_LOCK(object); }
> +inline void gstObjectUnlock(void* object) { GST_OBJECT_UNLOCK(object); }
> +inline void gstPadStreamLock(GstPad* pad) { GST_PAD_STREAM_LOCK(pad); }
> +inline void gstPadStreamUnlock(GstPad* pad) { GST_PAD_STREAM_UNLOCK(pad); }
> +
> +using GstObjectLocker = ExternalLocker<void, gstObjectLock, gstObjectUnlock>;
> +using GstPadStreamLocker = ExternalLocker<GstPad, gstPadStreamLock, gstPadStreamUnlock>;
> +
> +inline GstObjectLocker holdGstObjectLock(void* gstObject) WARN_UNUSED_RETURN;
> +inline GstObjectLocker holdGstObjectLock(void* gstObject)

I would also be tempted of moving ExternalLocker to WTF and just keeping these functions here. WDYT? (It would need a new review, easy I hope) Besides, I think you can just use holdLock as a name and let function overload do its job. WDYT?

[Alicia Boya García]

Created attachment 428817 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 428817 [details]
Patch

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

> Source/WTF/wtf/Locker.h:188
> +    explicit ExternalLocker(T* lockable) : m_lockable(lockable) { lock(); }

You missed this ASSERT.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:331
> +    return GstObjectLocker(gstObject);

Ditto.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:337
> +    return GstPadStreamLocker(pad);

Ditto.

[Chris Dumez]

Can you clarify the difference between this locker and the WTF::Locker we already have? As far as I know WTF supports all types of locks, not just WTF::Lock.

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> Can you clarify the difference between this locker and the WTF::Locker we
> already have? As far as I know WTF supports all types of locks, not just
> WTF::Lock.

I see, you need custom lock()/unlock() functions.

[Chris Dumez]

Comment on attachment 428817 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:329
> +inline GstObjectLocker holdGstObjectLock(void* gstObject)

What do those hold functions bring? Why not call the constructor directly at call sites like we now do for the regular Locker:
GstObjectLocker locker { object };

[Alicia Boya García]

Comment on attachment 428817 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:329
>> +inline GstObjectLocker holdGstObjectLock(void* gstObject)
> 
> What do those hold functions bring? Why not call the constructor directly at call sites like we now do for the regular Locker:
> GstObjectLocker locker { object };

They were there for consistency with holdLock(), which was used all across WebKit, until it stopped being used last Friday because the clang thread safety checker couldn't support it. https://bugs.webkit.org/show_bug.cgi?id=226119

Now I have to rework this patch a bit to adapt it to the new checker.

[Chris Dumez]

(In reply to Alicia Boya García from comment #11)
> Comment on attachment 428817 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428817&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:329
> >> +inline GstObjectLocker holdGstObjectLock(void* gstObject)
> > 
> > What do those hold functions bring? Why not call the constructor directly at call sites like we now do for the regular Locker:
> > GstObjectLocker locker { object };
> 
> They were there for consistency with holdLock(), which was used all across
> WebKit, until it stopped being used last Friday because the clang thread
> safety checker couldn't support it.
> https://bugs.webkit.org/show_bug.cgi?id=226119
> 
> Now I have to rework this patch a bit to adapt it to the new checker.

I don’t believe clang thread safety analysis will work with GstObject. However, I think it would still be nice to be as consistent as possible with the rest of WebKit. So I think the patch is fine as is but I would just remove the hold functions. I would also have moved the new ExternalLocker to its own header probably.

[Xabier Rodríguez Calvar]

(In reply to Chris Dumez from comment #12)
> I don’t believe clang thread safety analysis will work with GstObject.
> However, I think it would still be nice to be as consistent as possible with
> the rest of WebKit. So I think the patch is fine as is but I would just
> remove the hold functions. I would also have moved the new ExternalLocker to
> its own header probably.

LGTM

[Alicia Boya García]

Created attachment 430851 [details]
Patch

[EWS]

Committed r278655 (238637@main): <https://commits.webkit.org/238637@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/79068061>

[Alicia Boya García]

> I don’t believe clang thread safety analysis will work with GstObject.

You're right, as even though it would be possible to add lock/unlock annotations, all library GStreamer code would not contain any require_lock annotations.