724072 - If using multiple processes, display pid along with NS_WARNINGs

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

Debugging multiprocess Firefox is complicated by the fact that NS_WARNINGs are not tagged with a pid.

We could unconditionally display the pid, but then that's added noise for the majority of people, who aren't developing for e10s.

As a compromise, I propose displaying the pid only when we've spawned a subprocess.

Does the plugin-container process output NS_WARNINGs?

Comment 1

[Benjamin Smedberg]

They could, but since they don't use XPCOM I don't think they actually do.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What you propose sounds hard.  jduell made us process-specific NSPR log files, which should satisfy your use case here.

Comment 3

[Justin Lebar (not reading bugmail)]

> jduell made us process-specific NSPR log files, which should satisfy your use case here.

NS_Warning doesn't appear to be written via PR_Log, so this doesn't help much, unless I misunderstand.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #3)
> > jduell made us process-specific NSPR log files, which should satisfy your use case here.
> 
> NS_Warning doesn't appear to be written via PR_Log, so this doesn't help
> much, unless I misunderstand.

Nope, you don't.  My understanding was wrong.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 594797 [details] [diff] [review]
Patch v1

Looks good.  Use GetCurrentProcId() from base/process_util.h for the tagging.

Comment 7

[Justin Lebar (not reading bugmail)]

After working with this for a few days, I want to display "Parent" or "Child" along with the PID.  But doing that requires calling ClearOnShutdown from this function I'd like to be thread-safe (so we don't leak the description string).  Which requires making ClearOnShutdown thread-safe.

Thus, bug 725519.

Comment 8

[Benjamin Smedberg]

Why is that necessary, can't those be static strings?

Comment 9

[Justin Lebar (not reading bugmail)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Why is that necessary, can't those be static strings?

We could require that the char* passed for the string be statically-allocated.  I can't enforce that with the type system, so I figured I wouldn't add a footgun opportunity to the API.  But maybe it's worth the hack.

We should probably make ClearOnShutdown thread-safe anyway.

Comment 10

[Justin Lebar (not reading bugmail)]

Let's just make nsDebugImpl::SetMultiprocessMode main-thread only.  That'll simply things.

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2.1

Remove unnecessary include.

Comment 13

[Benjamin Smedberg]

Comment on attachment 613369 [details] [diff] [review]
Patch v2.1

So this will not have any effects for plugin parent/child relations, only content? That's ok, although it seems a bit surprising that simply creating a ContentParent will change the output of NS_WARNING in general.

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/91bbeb1c5187