Fix races in LibWebRTCCodecs introduced in r276214

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Fix races in LibWebRTCCodecs introduced in r276214.

[Chris Dumez]

Created attachment 426418 [details]
Patch

[youenn fablet]

Comment on attachment 426418 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:262
> +    ensureGPUProcessConnectionAndDispatchToThread([this, decoderIdentifier = decoder.identifier] {

Maybe add a comment in ensureGPUProcessConnectionAndDispatchToThread that the m_connection lock needs to be kept for the dispatchToThread call in ensureGPUProcessConnectionAndDispatchToThread/ensureOnMainRunLoop.

Also, let's look at the following case:
- createEncoder goes to main thread as connection is null.
- initializeEncoder goes to main thread as connection is null.
- createEncoder callback is in main thread, creates the connection, goes to the codec queue.
- releaseEncoder goes to the codec queue as connection is no longer null.
- initializeEncoder callback is in the main thread and goes to the codec queue.

We probably can have initializeEncoder happening after releaseEncoder in the codec queue.

We could accept the raciness of initializeEncoder, and add a if(auto* encoder...) in its implementation.
We could also store a goThroughMainThread state in encoder/decoder structures, less subtle and probably stronger.

[Chris Dumez]

(In reply to youenn fablet from comment #2)
> Comment on attachment 426418 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426418&action=review
> 
> > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:262
> > +    ensureGPUProcessConnectionAndDispatchToThread([this, decoderIdentifier = decoder.identifier] {
> 
> Maybe add a comment in ensureGPUProcessConnectionAndDispatchToThread that
> the m_connection lock needs to be kept for the dispatchToThread call in
> ensureGPUProcessConnectionAndDispatchToThread/ensureOnMainRunLoop.
> 
> Also, let's look at the following case:
> - createEncoder goes to main thread as connection is null.
> - initializeEncoder goes to main thread as connection is null.
> - createEncoder callback is in main thread, creates the connection, goes to
> the codec queue.
> - releaseEncoder goes to the codec queue as connection is no longer null.
> - initializeEncoder callback is in the main thread and goes to the codec
> queue.
> 
> We probably can have initializeEncoder happening after releaseEncoder in the
> codec queue.
> 
> We could accept the raciness of initializeEncoder, and add a if(auto*
> encoder...) in its implementation.

Even that wouldn't be safe I think. You could imagine createDecoder getting called first, then createEncoder, then initializeEncoder in your scenario. initializeEncoder would get received before createEncoder which would be bad.

> We could also store a goThroughMainThread state in encoder/decoder
> structures, less subtle and probably stronger.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> (In reply to youenn fablet from comment #2)
> > Comment on attachment 426418 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=426418&action=review
> > 
> > > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:262
> > > +    ensureGPUProcessConnectionAndDispatchToThread([this, decoderIdentifier = decoder.identifier] {
> > 
> > Maybe add a comment in ensureGPUProcessConnectionAndDispatchToThread that
> > the m_connection lock needs to be kept for the dispatchToThread call in
> > ensureGPUProcessConnectionAndDispatchToThread/ensureOnMainRunLoop.
> > 
> > Also, let's look at the following case:
> > - createEncoder goes to main thread as connection is null.
> > - initializeEncoder goes to main thread as connection is null.
> > - createEncoder callback is in main thread, creates the connection, goes to
> > the codec queue.
> > - releaseEncoder goes to the codec queue as connection is no longer null.
> > - initializeEncoder callback is in the main thread and goes to the codec
> > queue.
> > 
> > We probably can have initializeEncoder happening after releaseEncoder in the
> > codec queue.
> > 
> > We could accept the raciness of initializeEncoder, and add a if(auto*
> > encoder...) in its implementation.
> 
> Even that wouldn't be safe I think. You could imagine createDecoder getting
> called first, then createEncoder, then initializeEncoder in your scenario.
> initializeEncoder would get received before createEncoder which would be bad.

I have a proposal that would address that. I am polishing the patch and will upload shortly.

[Chris Dumez]

Created attachment 426433 [details]
Patch

[youenn fablet]

Comment on attachment 426433 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:202
> +    if (m_connection) {

I would put that first since this is the fast path.

> Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:213
> +            dispatchToThread(m_tasksToDispatchAfterEstablishingConnection.takeFirst());

Seems like we could use a Vector.

[Chris Dumez]

Created attachment 426437 [details]
Patch

[EWS]

Committed r276263 (236745@main): <https://commits.webkit.org/236745@main>

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

[Ling Ho]

rdar://76852677