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)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: jesup, Assigned: ehugg)
References
Details
(Whiteboard: [WebRTC], [blocking-webrtc+], [qa-])
Attachments
(1 file, 7 obsolete files)
29.06 KB,
patch
|
ehugg
:
review+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•11 years ago
|
Flags: needinfo?(ethanhugg)
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #697112 -
Attachment is obsolete: true
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Comment 6•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #697124 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #697166 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #697172 -
Flags: review?(rjesup)
Reporter | ||
Comment 8•11 years ago
|
||
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+
Assignee | ||
Comment 9•11 years ago
|
||
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
Assignee | ||
Comment 10•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/b5c0b31b73ba
Reporter | ||
Updated•11 years ago
|
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [leave-open]
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #697999 -
Attachment is obsolete: true
Assignee | ||
Comment 14•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: rjesup → ethanhugg
Assignee | ||
Comment 15•11 years ago
|
||
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)
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Comment 17•11 years ago
|
||
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)
Reporter | ||
Comment 18•11 years ago
|
||
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+
Assignee | ||
Comment 19•11 years ago
|
||
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.
Reporter | ||
Comment 20•11 years ago
|
||
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+
Reporter | ||
Updated•11 years ago
|
Attachment #697172 -
Attachment is obsolete: true
Assignee | ||
Comment 21•11 years ago
|
||
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?
Assignee | ||
Comment 22•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #698058 -
Attachment is obsolete: true
Assignee | ||
Comment 23•11 years ago
|
||
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+
Reporter | ||
Comment 24•11 years ago
|
||
Still one syntax error per the Try. Can we iterate one more time and land this? Thanks!
Reporter | ||
Comment 25•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=acdc2da1913d With a trivial syntax fix
Assignee | ||
Comment 26•11 years ago
|
||
I ran one with the fix yesterday as well https://tbpl.mozilla.org/?tree=Try&rev=651b16fc4c4f
Reporter | ||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/de6f3b7fc084
Target Milestone: --- → mozilla20
Reporter | ||
Updated•11 years ago
|
Whiteboard: [WebRTC], [blocking-webrtc+], [leave-open] → [WebRTC], [blocking-webrtc+]
Comment 28•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de6f3b7fc084
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•