1047442 - Locking assumption in webrtc.org Encoded() callback invalid with async codecs

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

The webrtc.org code for encoding via video_sender.cc/AddFrame and generic_encoder assumes that the Encoded() callback will happen from *within* the Encode() call (i.e. with the video_sender's _sendCritSect locked).

This is invalid for any codec that calls Encoded() asynchronously, such as GMP codecs.

This causes on windows (if IPC issues are bypassed) crashes (rarely; minutes or many minutes into a call) in media_optimization from within Encoded().

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: reacquire _sendCritSect within webrtc Encoded() callback (upstream patch)

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add thread to resolve deadlock on sendCritSect caused by sync Encode dispatch to GMPThread

This works (4 hours in Linux and Win32 no crashes) but I prefer to find a solution that avoids the Sync dispatch, and so avoids adding threads

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Note: the dangerous deadlock here leading to the extra thread is that the caller to Encode() holds the _sendCritSect, and if GMPThread is in Encoded() and tries to grab _sendCritSect it will stall, and thus never service the sender's Encode() runnable.

The obvious solution to avoiding an extra thread (not having the Encode() call block on GMPThread) means we need to transfer the frame-to-be-encoded to a shared-mem-backed-i420 frame, and send that to GMPThread asynchronously.  This fails since we can't allocate Shmems for use with GMP except on, you guessed it, GMPThread!  We can't even ask GMPThread for some, since it may not be servicing requests.

Grrr.  Perhaps more work in the caller could relax the restriction somehow.  I don't seem to be able to drop the critsect before calling Encode() safely (tried that)

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a3810931af
https://hg.mozilla.org/integration/mozilla-inbound/rev/27ab005a03f7

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Unified build bustage fix rs=bustage,kwierso
https://hg.mozilla.org/integration/mozilla-inbound/rev/09425d87715d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4a3810931af
https://hg.mozilla.org/mozilla-central/rev/27ab005a03f7
https://hg.mozilla.org/mozilla-central/rev/09425d87715d

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8466234 [details] [diff] [review]
reacquire _sendCritSect within webrtc Encoded() callback (upstream patch)

For both patches:
Approval Request Comment
[Feature/regressing bug #]: openh264/GMP

[User impact if declined]: random crashes (especially on windows) and UAFs

[Describe test coverage new/current, TBPL]: Thread-order issues can't generally be tested in tbpl, and this may require long calls to hit (5-20 minutes).  On nightly for some time now; tested in multiple 6-hour-plus calls on Windows.

[Risks and why]: moderately low risk; we're modifying locking code, but the locking required is fairly clear.  The additional thread is annoying, but needed to avoid deadlocks.

[String/UUID change made/needed]: none.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e4abfe41b9d5
https://hg.mozilla.org/releases/mozilla-aurora/rev/149710661ef8

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8466234 [details] [diff] [review]
reacquire _sendCritSect within webrtc Encoded() callback (upstream patch)

Review of attachment 8466234 [details] [diff] [review]:
-----------------------------------------------------------------

While we haven't identified a guaranteed regression yet, 2.0 OMX encoders appear to have the same problem (since we call Encoded() on another thread).

Comment 10

[Diego Wilson [:diego]]

Randell,

I was trying to cherry pick these patches to the FFOS v2.0 branch to see if they fix bug 1052169. They don't apply cleanly. Can I get your help by rebasing them?

I can try it myself but maybe it'll be trivial for you.

Comment 11

[bhavana bajaj [:bajaj]]

(In reply to Diego Wilson [:diego] from comment #10)
> Randell,
> 
> I was trying to cherry pick these patches to the FFOS v2.0 branch to see if
> they fix bug 1052169. They don't apply cleanly. Can I get your help by
> rebasing them?
> 
> I can try it myself but maybe it'll be trivial for you.

spoke offline with :jesup, diego was able to apply the patch cleanly, we're now waiting for results to confirm this fixes report in 1052169

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/10f504e92bfc