Closed
Bug 819549
Opened 11 years ago
Closed 10 years ago
Make signaling_unittests manipulate PC on main thread
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: ekr, Assigned: ehugg)
References
Details
(Whiteboard: [WebRTC][blocking-webrtc-])
Attachments
(1 file, 3 obsolete files)
13.38 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
Currently signaling_unittests spawns a new nsThread and passes it to the PC as the "main thread". This is where, for instance, PC events fire. However, the unit tests themselves run on thread 0 (i.e, the one running from main()). That means that we are manipulating the PC on both threads at once which creates potential problems. Note that this is not how things work in ffox because the JS thread (the main thread) is what calls the PC operations. We should do one of two things in the test: 1. Have all PC operations Dispatched onto the main thread 2. Make thread 0 the main thread and figure out how to make the tests run. Strategy 1 is a lot of typing but is conceptually simple and we do it already in a bunch of places. Strategy 1 may be more right, but is going to be a PITA becuse we will then need to service the event loop and make the various WAIT macros work right. I suggest 1 for now but it's worth some study.
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → jib
Updated•11 years ago
|
Whiteboard: [WebRTC][blocking-webrtc-]
Assignee | ||
Updated•10 years ago
|
Assignee: jib → ethanhugg
Assignee | ||
Comment 1•10 years ago
|
||
Unfortunately, strategy 1 from the top comment does not solve our current problem (bug 995380). We need the thread passed into PeerConnection::Initialize() to be the main thread so signaling code dispatches to the real main thread (vcmGetVideoMaxFs in this example). Just doing a NS_GetMainThread and tossing that in there breaks a bunch of other things so investigation is ongoing.
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8409125 -
Attachment is obsolete: true
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8414006 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8414012 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8414025 [details] [diff] [review] Signaling unittests should dispatch to main thread when calling PC Review of attachment 8414025 [details] [diff] [review]: ----------------------------------------------------------------- I put a wrapper around the PeerConnectionImpl that is used in SignalingAgent that dispatches to the main thread for all calls into it. The calls from TestObserver are already on the main thread so I added some asserts to enforce that. In some cases I had to change a parameter from a ref in order to satisfy the WrapRunnable. I also simplified SignalingAgent with a single constructor to avoid adding duplicate code. This in combination with the patch from bug 995380 should fix this bug.
Attachment #8414025 -
Flags: review?(rjesup)
Updated•10 years ago
|
Attachment #8414025 -
Flags: review?(rjesup) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dc0e538254d
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3dc0e538254d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•