1418165 - Protect sVideoCaptureThread with sThreadMonitor

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement, P3)

Comment 1

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1418165 - Protect sVideoCaptureThread with sThreadMonitor.

Review commit: https://reviewboard.mozilla.org/r/200576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200576/

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8929275 [details]
Bug 1418165 - Protect sVideoCaptureThread with sThreadMonitor.

https://reviewboard.mozilla.org/r/200576/#review205768

Might work, but I'm a bit concerned.

::: dom/media/systemservices/CamerasParent.cpp:341
(Diff revision 1)
> +  MonitorAutoLock lock(*sThreadMonitor);
>    MOZ_ASSERT(sVideoCaptureThread->thread_id() == PlatformThread::CurrentId());

It seems a bit excessive to add a lock here solely to protect a MOZ_ASSERT that tries to assert that we're actually *on* the thread in question.

Since we should always be on the thread, the lock seems unnecessary.

Could we either:

A) put #ifdef DEBUG around it and use {} to limit the scope of the lock to just around the assert? or

B) Remove the assert entirely?

Comment 3

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8929275 [details]
Bug 1418165 - Protect sVideoCaptureThread with sThreadMonitor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200576/diff/1-2/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dec59488ecf1
Protect sVideoCaptureThread with sThreadMonitor. r=jib

Comment 5

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/dec59488ecf1