910810 - WebrtcAudioConduit uses preferences off the main thread

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

Webrtc gets and uses the preferences service off the main thread, but prefs are not threadsafe.

http://hg.mozilla.org/mozilla-central/annotate/21b5af569ca2/media/webrtc/signaling/src/media-conduit/AudioConduit.cpp#l324

https://tbpl.mozilla.org/php/getParsedLog.php?id=27184302&tree=Try#error0

Comment 1

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

This is caused by the unit tests having a faked-up "MainThread".  Actual browser code doesn't have this problem; the code is called from vcmTxStartICE_m(), which (as the _m implies in the webrtc_signaling code) runs on MainThread.

ekr, do you remember what the issues were that caused us to use a fake mainthread in the unit tests?

Comment 2

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

Any movement here?

Comment 3

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

Hey Randell, can we get someone to take this?

Comment 4

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

Attachment: don't read prefs off "main" thread in unittests

Comment 5

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

Comment on attachment 809591 [details] [diff] [review]
don't read prefs off "main" thread in unittests

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

Patch got uploaded but forgot to ask for review on it.  Pretty simple....  (Larger issue of not running these tests off-mainthread in c++ unit tests will be dealt with elsewhere (bug 925675))

Comment 6

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

Comment on attachment 809591 [details] [diff] [review]
don't read prefs off "main" thread in unittests

Somebody familiar with this code should review this.

Comment 7

[Adam Roach [:abr]]

Comment on attachment 809591 [details] [diff] [review]
don't read prefs off "main" thread in unittests

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

Seems reasonable, given that the constructor sets these to sane defaults.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8afb1865a1

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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