Closed
Bug 995380
Opened 10 years ago
Closed 10 years ago
Signaling unittests assert on M-C.
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: ehugg, Assigned: ehugg)
References
Details
Attachments
(1 file, 5 obsolete files)
12.47 KB,
patch
|
ehugg
:
review+
|
Details | Diff | Splinter Review |
Because we don't use the main thread on these unittests until bug 819549, we are hitting an assert that checks mainthread.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ethanhugg
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8405552 [details] [diff] [review] Remove mainthread check to unbreak the signaling unittests Can't use MOZ_INTERNAL_API on this one since it's not in the signaling code.
Attachment #8405552 -
Flags: review?(rjesup)
Comment 3•10 years ago
|
||
Comment on attachment 8405552 [details] [diff] [review] Remove mainthread check to unbreak the signaling unittests Review of attachment 8405552 [details] [diff] [review]: ----------------------------------------------------------------- We can't remove the thread-safety check from libpref. Not without a dozen knives from khuey in your back. The code will have to be wrapped with *something* that avoids touching prefs in the cpp tests, even if it's "if (IsMainThread()) {" Or refactor them to be on mainthread finally.
Attachment #8405552 -
Flags: review?(rjesup) → review-
Assignee | ||
Comment 4•10 years ago
|
||
Well, it was worth a try. If 819549 proves difficult to finish soon, I'll propose an IsMainThread solution somewhere in the signaling code. In the meantime, I'll leave this patch up so anyone trying to run the unittests can still get them to work.
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8405552 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8408987 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8408993 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8408996 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8409000 [details] [diff] [review] Signaling unittests should use the real main thread. I think I figured out a solution. Instead of creating a pseudo-main thread I am using the real main thread and passing that into PeerConnectionImpl::Initialize. This means we need to spawn a separate thread for running the GTests since they do things like ASSERT_TRUE_WAIT. Also this means we need to add our own event handling loop to the main thread which in turn needs its own shutdown mechanism since it was not created with nsThread::Init. This is what is suggested as solution #2 in Bug 819549. It only goes halfway to solving that bug however. The calls into PeerConnection need to also be dispatched to the main thread in order to fully fix what is described there. I tried to keep this patch small to get the unittests working again but here is my list of more work to be done in the signaling tests after this patch: 1. Finish Bug 819549 by dispatching the calls into PC. 2. Remove the #ifdef MOZILLA_INTERNAL_API bits around main thread asserts from the signaling code which are no longer needed. 3. Remove some redundant or unused code in signaling_unittests.cpp. Especially the stuff that passes around threads.
Attachment #8409000 -
Flags: review?(rjesup)
Comment 10•10 years ago
|
||
Comment on attachment 8409000 [details] [diff] [review] Signaling unittests should use the real main thread. Review of attachment 8409000 [details] [diff] [review]: ----------------------------------------------------------------- r+ with one minor mod and one addition. Please add to your plans trying using ScopedXPCOM xpcom("webrtc signaling unittests"); in main(). See TestThreadUtils.cpp or netwerk/test/TestUDPSocket.cpp ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +3688,5 @@ > + // Set the global shutdown flag and tickle the main thread > + // The main thread did not go through Init() so calling Shutdown() > + // on it will not work. > + gTestsComplete = true; > + gMainThread->Dispatch(WrapRunnableNM(tests_complete), NS_DISPATCH_SYNC); Instead, set gTestsComplete on the mainthread in tests_complete @@ +3746,5 @@ > + // When the GTest thread is complete it will set gTestsComplete and > + // send one more dispatch. > + nsresult rv = NS_OK; > + bool next_event_ret; > + while (!gTestsComplete && rv == NS_OK) { A minor issue in this context is that gTestsComplete access from multiple threads is unsafe (TSAN). However, easily fixed, see comment above. @@ +3747,5 @@ > + // send one more dispatch. > + nsresult rv = NS_OK; > + bool next_event_ret; > + while (!gTestsComplete && rv == NS_OK) { > + rv = gMainThread->ProcessNextEvent(true, &next_event_ret); or NS_ProcessNextEvent(gMainThread or nullptr, true); Equivalent; all work @@ +3748,5 @@ > + nsresult rv = NS_OK; > + bool next_event_ret; > + while (!gTestsComplete && rv == NS_OK) { > + rv = gMainThread->ProcessNextEvent(true, &next_event_ret); > + } gTestThread->Shutdown();
Attachment #8409000 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8409000 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8409990 [details] [diff] [review] Signaling unittests should use the real main thread. Bringing forward r+ from Jesup.
Attachment #8409990 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7160b5750b5
Keywords: checkin-needed
Target Milestone: --- → mozilla31
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e7160b5750b5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•