Closed
Bug 846942
Opened 12 years ago
Closed 12 years ago
Add thread names to WebRTC signaling system log messages
Categories
(Core :: WebRTC: Signaling, defect)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: abr, Assigned: abr)
Details
(Whiteboard: [WebRTC] [blocking-webrtc-] [qa-])
Attachments
(1 file, 3 obsolete files)
13.85 KB,
patch
|
ehugg
:
review+
|
Details | Diff | Splinter Review |
After slogging through substantial quantities of the logs that we added in Bug 841566, I've concluded that it would be much easier to figure out what's going on here if we included the friendly thread names as part of the log messages.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [WebRTC] [blocking-webrtc-]
Assignee | ||
Comment 2•12 years ago
|
||
Here's a try (along with a patch for bug 845523): https://tbpl.mozilla.org/?tree=Try&rev=a44a035255ff
Assignee | ||
Comment 3•12 years ago
|
||
Re-try after compile fixes for Linux & Windows: https://tbpl.mozilla.org/?tree=Try&rev=5157500f9c63
Comment 4•12 years ago
|
||
Comment on attachment 720150 [details] [diff] [review] Add thread names to log messages Review of attachment 720150 [details] [diff] [review]: ----------------------------------------------------------------- r+, except I think it needs to be tested on Windows before checkin. ::: media/webrtc/signaling/src/common/browser_logging/CSFLog.cpp @@ +85,5 @@ > +} > + > +const char *CSFCurrentThreadName() { > +#ifdef WIN32 > + thread_t handle = nullptr; I'm confused about how this should work on Windows. Wouldn't something like the WINAPI call GetCurrentThreadId() be what you need here? @@ +93,5 @@ > + PR_RWLock_Rlock(maplock); > + threadMap_t::iterator i = threadMap.find(handle); > + PR_RWLock_Unlock(maplock); > + if (i == threadMap.end()) { > + return nullptr; Is this Unlock in the right place? It seems that the iterator could be invalidated by the erase() in the previous function. @@ +114,5 @@ > + if (!threadName) { > + threadName = PR_GetThreadName(PR_GetCurrentThread()); > + } > + if (!threadName) { > + threadName = ""; I'm guessing this is a should-never-happen thing, if so, perhaps we should put some error text here, if not we could make a name out of the ID.
Attachment #720150 -
Flags: review?(ethanhugg) → review+
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #720150 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
I've attached a new patch that *should* handle the Windows thread names. I was trying to index by handle, but couldn't figure out how to get the current thread handle in a way that matched the original handle. It turns out that indexing by Thread ID is a lot easier to figure out. > I'm guessing this is a should-never-happen thing, if so, perhaps we > should put some error text here, if not we could make a name out of the ID. It can and does happen, for a couple of reasons. First, it's perfectly fine to create an NSPR thread without a name; second, third party systems (notably webrtc) are going to create threads using their own system. The WebRTC project uses a linux-specific means of setting thread names, and I've added some code that should allow us to get the thread name for threads created by the WebRTC code if we're on Linux. There's similar code for OSX, but WebRTC doesn't use the OSX-specific thread naming calls (yet). New try run so I can make sure the logs on Linux and Windows look right (and to double-check that I didn't break OSX): https://tbpl.mozilla.org/?tree=Try&rev=3c57d61150dc
Assignee | ||
Comment 7•12 years ago
|
||
Re-try to fix #include issue on Linux: https://tbpl.mozilla.org/?tree=Try&rev=1bab7d63227d
Assignee | ||
Comment 8•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #720328 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
Comment on attachment 720347 [details] [diff] [review] Add thread names to log messages This new patch addresses review comments. Here's a representative log from Windows showing the thread names: https://tbpl.mozilla.org/php/getParsedLog.php?id=20263439&tree=Try I'm asking for re-review due to the number and nature of changes between this and the previous patch.
Attachment #720347 -
Flags: review?(ethanhugg)
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #720347 -
Attachment is obsolete: true
Attachment #720347 -
Flags: review?(ethanhugg)
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 720350 [details] [diff] [review] Add thread names to log messages Sorry for the noise; this revised patch ensures the main thread is labeled "main" (rather than "firefox," which is applied to all unnamed threads) under Linux.
Attachment #720350 -
Flags: review?(ethanhugg)
Comment 12•12 years ago
|
||
Comment on attachment 720350 [details] [diff] [review] Add thread names to log messages lgtm
Attachment #720350 -
Flags: review?(ethanhugg) → review+
Assignee | ||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/db2af0e84527
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/db2af0e84527
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Updated•12 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
•