Closed Bug 995380 Opened 6 years ago Closed 6 years ago

Signaling unittests assert on M-C.

Categories

(Core :: WebRTC: Signaling, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: ehugg, Assigned: ehugg)

References

Details

Attachments

(1 file, 5 obsolete files)

Because we don't use the main thread on these unittests until bug 819549, we are hitting an assert that checks mainthread.
Assignee: nobody → ethanhugg
Status: NEW → ASSIGNED
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 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-
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.
Attachment #8405552 - Attachment is obsolete: true
Attachment #8408987 - Attachment is obsolete: true
Attachment #8408993 - Attachment is obsolete: true
Attachment #8408996 - Attachment is obsolete: true
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 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+
Attachment #8409000 - Attachment is obsolete: true
Comment on attachment 8409990 [details] [diff] [review]
Signaling unittests should use the real main thread.

Bringing forward r+ from Jesup.
Attachment #8409990 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e7160b5750b5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 819549
You need to log in before you can comment on or make changes to this bug.