WebrtcAudioConduit uses preferences off the main thread

RESOLVED FIXED in mozilla27

Status

()

Core
WebRTC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: khuey, Assigned: jesup)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webrtc])

Attachments

(1 attachment)

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
(Assignee)

Comment 1

4 years ago
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?
Component: WebRTC: Audio/Video → WebRTC
Flags: needinfo?(ekr)
Any movement here?
Hey Randell, can we get someone to take this?
Flags: needinfo?(rjesup)
(Assignee)

Updated

4 years ago
Assignee: nobody → rjesup
Flags: needinfo?(rjesup)
Whiteboard: [webrtc]
Target Milestone: --- → mozilla27
(Assignee)

Comment 4

4 years ago
Created attachment 809591 [details] [diff] [review]
don't read prefs off "main" thread in unittests
(Assignee)

Comment 5

4 years ago
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))
Attachment #809591 - Flags: review?(khuey)
Comment on attachment 809591 [details] [diff] [review]
don't read prefs off "main" thread in unittests

Somebody familiar with this code should review this.
Attachment #809591 - Flags: review?(khuey)
(Assignee)

Updated

4 years ago
Attachment #809591 - Flags: review?(adam)

Comment 7

4 years ago
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.
Attachment #809591 - Flags: review?(adam) → review+
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8afb1865a1
Flags: needinfo?(ekr)
https://hg.mozilla.org/mozilla-central/rev/fa8afb1865a1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.