Closed Bug 825785 Opened 11 years ago Closed 11 years ago

PeerConnectionCtx shutdown needs to be able to block until SIPcc's shutdown is complete

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jesup, Assigned: ehugg)

References

Details

(Whiteboard: [WebRTC], [blocking-webrtc+], [qa-])

Attachments

(1 file, 7 obsolete files)

Occasionally on win32 debug, we leak SIPcc and the associated threads...  

We recently added shutdown of PeerConnectionCtx from XPCOM_SHUTDOWN in bug 825514.  However, I suspect the errors on try are due to SIPcc shutdown being inherently asynchronous, and on windows debug sometimes it doesn't finish before the rest of XPCOM exits (or so it seems).

Is there a good way to know (and sub-threads) are all down?  Or can we make one?
Flags: needinfo?(ethanhugg)
It looks like something that is particular to our Windows implementation.  The Linux/OSX/Android implementations do this in cprDestroyThread

            pthread_exit(NULL);

But the Windows impl does this

			result = PostThreadMessage(((cpr_thread_t *)thread)->threadId, WM_CLOSE, 0, 0);
			if(result) {
				waitrc = WaitForSingleObject(hThread, 60000);
			}

Since cprDestroyThread is called by the thread that will be destroyed it seems odd that we would be posting a WM_CLOSE message.  There must be a way to make the Windows impl more like the others here.
Flags: needinfo?(ethanhugg)
Attachment #697112 - Attachment is obsolete: true
Comment on attachment 697124 [details] [diff] [review]
cprDestroyThread on Windows should kill thread immediately like other OS impls


Not sure yet if this solves everything, but this should make the Windows threads exit immediately like the other OS targets.
Attachment #697124 - Flags: review?(rjesup)
Comment on attachment 697124 [details] [diff] [review]
cprDestroyThread on Windows should kill thread immediately like other OS impls

Review of attachment 697124 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_threads.c
@@ +233,5 @@
>  
> +            /*
> +             * Make sure thread is trying to destroy itself.
> +             */
> +            if (hThread == GetCurrentThreadId()) {

This check isn't right, trying one more time.
Attachment #697124 - Flags: review?(rjesup)
Attachment #697124 - Attachment is obsolete: true
Attachment #697166 - Attachment is obsolete: true
Attachment #697172 - Flags: review?(rjesup)
Comment on attachment 697172 [details] [diff] [review]
cprDestroyThread on Windows should kill thread immediately like other OS impls

Review of attachment 697172 [details] [diff] [review]:
-----------------------------------------------------------------

This may wallpaper the problem (though I'd prefer to see something stronger than a error debug if it's told to kill another thread - assertion/etc).

I don't think this solves the problem in that we have no feedback that the thread died.  This basically makes threads told to die do so immediately, but we still have not verification back to the caller that the callee killed itself (and I realize that's tough).

We should land this and see how it works.
Attachment #697172 - Flags: review?(rjesup) → review+
I will land this with MOZ_ASSERTs added next to the error messages.  At least now the different targets will have the same behavior.  I will re-open the bug after it gets landed and put in something for feedback on thread status.  I have a couple ideas of how to solve, but nothing particularly brilliant.

Try run is here - https://tbpl.mozilla.org/?tree=Try&rev=160d94f5268b
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [leave-open]
Blocks: 796463
Attachment #697999 - Attachment is obsolete: true
Comment on attachment 698007 [details] [diff] [review]
Signaling - monitor threads for shutdown


This patch has each thread setting its threadId in the monitor to zero right before the call to pthread_exit() or ExitThread().  Then ccUnload loops until they are all zero.  

This solution works without using any new locks or OS-specific code.  TSan may not like the threadIds being read and written at the same time, but worst case I think is 10ms more in the loop waiting for all zero.

Also removed the trailing whitespace from the gyp, it was missed in the earlier patch of Bug 800611.
Attachment #698007 - Flags: review?(rjesup)
Assignee: rjesup → ethanhugg
Comment on attachment 698007 [details] [diff] [review]
Signaling - monitor threads for shutdown


Changed my mind already on how to do this.  I'd like to avoid the loop.
Attachment #698007 - Attachment is obsolete: true
Attachment #698007 - Flags: review?(rjesup)
Comment on attachment 698058 [details] [diff] [review]
Signaling - monitor threads for shutdown

Review of attachment 698058 [details] [diff] [review]:
-----------------------------------------------------------------

Was able to clear some of the blockage between my brain and the WinAPI and created a cprJoinThread to sync shutdown.  The three or four threads that get created are kept on a list and all joined on shutdown.
Attachment #698058 - Flags: review?(rjesup)
Comment on attachment 698007 [details] [diff] [review]
Signaling - monitor threads for shutdown

Review of attachment 698007 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - inherently there's still a minor race between calling the "i'm exiting" function and actually exiting; dealing with that is (as I think you say) OS-specific and probably a major pain.

::: media/webrtc/signaling/src/sipcc/core/common/thread_monitor.c
@@ +9,5 @@
> +
> +/*
> + * If thread is running, should have a non-zero entry here that is the threadId
> + */
> +static uint32_t thread_id_list[THREADMON_MAX] = {0, 0, 0, 0};

statics default to 0, and doing the init is fragile if it did something useful (i.e. if THREADMON_MAX ever changed you'd need to change this.
Attachment #698007 - Flags: review+
My bad for creating so much bugspam on this yesterday., but his was a review of an obsoleted patch.  Please let me know if you prefer the current one that has cprJoinThread and joins on shutdown.
Comment on attachment 698058 [details] [diff] [review]
Signaling - monitor threads for shutdown

Review of attachment 698058 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Go with this one.
Attachment #698058 - Flags: review?(rjesup) → review+
Attachment #697172 - Attachment is obsolete: true
Did you intend to obsolete the patch that already landed on M-C (in comment 11)?

Also, do you think its important for this to make the FF20 uplift, or should I wait until after to push?
Attachment #698058 - Attachment is obsolete: true
Comment on attachment 698324 [details] [diff] [review]
Signaling - monitor threads for shutdown


Fixed a syntax error in Windows, and changed static init per EKR's suggestion.

Bringing forward the r+ from Jesup.

Try Build - https://tbpl.mozilla.org/?tree=Try&rev=e2cb198c28f2
Attachment #698324 - Flags: review+
Still one syntax error per the Try.  Can we iterate one more time and land this?  Thanks!
I ran one with the fix yesterday as well 

https://tbpl.mozilla.org/?tree=Try&rev=651b16fc4c4f
Whiteboard: [WebRTC], [blocking-webrtc+], [leave-open] → [WebRTC], [blocking-webrtc+]
https://hg.mozilla.org/mozilla-central/rev/de6f3b7fc084
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
You need to log in before you can comment on or make changes to this bug.