Closed Bug 846942 Opened 9 years ago Closed 9 years ago

Add thread names to WebRTC signaling system log messages

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: abr, Assigned: abr)

Details

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

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Add thread names to log messages (obsolete) — Splinter Review
Attachment #720150 - Flags: review?(ethanhugg)
Whiteboard: [WebRTC] [blocking-webrtc-]
Here's a try (along with a patch for bug 845523):

https://tbpl.mozilla.org/?tree=Try&rev=a44a035255ff
Re-try after compile fixes for Linux & Windows:
https://tbpl.mozilla.org/?tree=Try&rev=5157500f9c63
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+
Attached patch Add thread names to log messages (obsolete) — Splinter Review
Attachment #720150 - Attachment is obsolete: true
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
Re-try to fix #include issue on Linux:
https://tbpl.mozilla.org/?tree=Try&rev=1bab7d63227d
Attached patch Add thread names to log messages (obsolete) — Splinter Review
Attachment #720328 - Attachment is obsolete: true
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)
Attachment #720347 - Attachment is obsolete: true
Attachment #720347 - Flags: review?(ethanhugg)
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 on attachment 720350 [details] [diff] [review]
Add thread names to log messages

lgtm
Attachment #720350 - Flags: review?(ethanhugg) → review+
https://hg.mozilla.org/mozilla-central/rev/db2af0e84527
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Whiteboard: [WebRTC] [blocking-webrtc-] → [WebRTC] [blocking-webrtc-] [qa-]
You need to log in before you can comment on or make changes to this bug.