846942 - Add thread names to WebRTC signaling system log messages

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Adam Roach [:abr]]

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.

Comment 1

[Adam Roach [:abr]]

Attachment: Add thread names to log messages

Comment 2

[Adam Roach [:abr]]

Here's a try (along with a patch for bug 845523):

https://tbpl.mozilla.org/?tree=Try&rev=a44a035255ff

Comment 3

[Adam Roach [:abr]]

Re-try after compile fixes for Linux & Windows:
https://tbpl.mozilla.org/?tree=Try&rev=5157500f9c63

Comment 4

[Ethan Hugg [:ehugg]]

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.

Comment 5

[Adam Roach [:abr]]

Attachment: Add thread names to log messages

Comment 6

[Adam Roach [:abr]]

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

Comment 7

[Adam Roach [:abr]]

Re-try to fix #include issue on Linux:
https://tbpl.mozilla.org/?tree=Try&rev=1bab7d63227d

Comment 8

[Adam Roach [:abr]]

Attachment: Add thread names to log messages

Comment 9

[Adam Roach [:abr]]

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.

Comment 10

[Adam Roach [:abr]]

Attachment: Add thread names to log messages

Comment 11

[Adam Roach [:abr]]

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.

Comment 12

[Ethan Hugg [:ehugg]]

Comment on attachment 720350 [details] [diff] [review]
Add thread names to log messages

lgtm

Comment 13

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db2af0e84527

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/db2af0e84527