Closed Bug 819549 Opened 7 years ago Closed 6 years ago

Make signaling_unittests manipulate PC on main thread

Categories

(Core :: WebRTC: Signaling, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: ekr, Assigned: ehugg)

References

Details

(Whiteboard: [WebRTC][blocking-webrtc-])

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → jib
Whiteboard: [WebRTC][blocking-webrtc-]
Assignee: jib → ethanhugg
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.
Attachment #8409125 - Attachment is obsolete: true
Attachment #8414006 - Attachment is obsolete: true
Attachment #8414012 - Attachment is obsolete: true
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)
Depends on: 995380
Blocks: 1002890
Attachment #8414025 - Flags: review?(rjesup) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3dc0e538254d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.