848401 - Raw PRLocks in MediaEngineWebRTCVideo.cpp are sadmaking (lots of deadlock potential)

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

content/media/webrtc/MediaEngineWebRTCVideo.cpp is full of raw PRLock and PR_[Un]Lock calls.  This code is a poster child for the benefits of RAII.  Conversion to mozilla::Mutex/Monitor and the associated RAII helpers would be very nice.

Comment 1

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

Attachment: switch from ReentrantMonitors and PR_Lock/Condvar to Monitor. Fix {picture:true} for desktop

Comment 2

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

Comment on attachment 724121 [details] [diff] [review]
switch from ReentrantMonitors and PR_Lock/Condvar to Monitor.  Fix {picture:true} for desktop

khuey for the locking; dao for the trivial change to the UI to fix "picture:true".

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 724121 [details] [diff] [review]
switch from ReentrantMonitors and PR_Lock/Condvar to Monitor.  Fix {picture:true} for desktop

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

r=me but note my comment

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +139,5 @@
>    // mMonitor protects mImage access/changes, and transitions of mState
>    // from kStarted to kStopped (which are combined with EndTrack() and
>    // image changes).  Note that mSources is not accessed from other threads
> +  // for video and is not protected.  mSnapshotCondVar is also protected
> +  // with this monitor.

mSnapshotCondVar is no longer a thing, so don't add it to the comment.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +370,5 @@
>  
> +  {
> +    MonitorAutoLock lock(mMonitor);
> +    mInSnapshotMode = true;
> +  }

Is it safe to release the lock before running all of the following code?  Somebody who actually knows this code (and not just the semantics of our locks) should sign off on this.

Comment 4

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)

> ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> @@ +370,5 @@
> >  
> > +  {
> > +    MonitorAutoLock lock(mMonitor);
> > +    mInSnapshotMode = true;
> > +  }
> 
> Is it safe to release the lock before running all of the following code? 
> Somebody who actually knows this code (and not just the semantics of our
> locks) should sign off on this.

Yes, it's safe, as we're just using this as a signal that the capture code has delivered at least one frame.  The code following this starts the capture than waits (with the monitor) for something to be captured and sets the value back to false.

Thanks

Comment 5

[Dão Gottwald [:dao]]

(In reply to Randell Jesup [:jesup] from comment #2)
> dao for the trivial change to the UI to fix "picture:true".

What does that change have to do with this bug?

Comment 6

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

I couldn't test this fix without first fixing the picture:true feature for desktop.  Picture:true is going to be replaced with (or deprecated by) something defined in the media capture task force, but that will be a while.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b65f38865b9

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7b65f38865b9