Last Comment Bug 724072 - If using multiple processes, display pid along with NS_WARNINGs
: If using multiple processes, display pid along with NS_WARNINGs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on: 725519
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-03 12:29 PST by Justin Lebar (not reading bugmail)
Modified: 2012-04-11 09:17 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (5.89 KB, patch)
2012-02-06 13:47 PST, Justin Lebar (not reading bugmail)
cjones.bugs: feedback+
Details | Diff | Splinter Review
Patch v2 (7.24 KB, patch)
2012-04-09 13:14 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v2.1 (6.91 KB, patch)
2012-04-09 13:17 PDT, Justin Lebar (not reading bugmail)
benjamin: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-02-03 12:29:29 PST
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 [:bsmedberg] 2012-02-03 12:46:17 PST
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 2012-02-03 17:09:59 PST
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) 2012-02-06 13:06:41 PST
> 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) 2012-02-06 13:47:22 PST
Created attachment 594797 [details] [diff] [review]
Patch v1
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-07 01:39:14 PST
(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 2012-02-07 01:41:37 PST
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) 2012-02-08 22:14:31 PST
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 [:bsmedberg] 2012-02-09 04:11:33 PST
Why is that necessary, can't those be static strings?
Comment 9 Justin Lebar (not reading bugmail) 2012-02-09 11:57:35 PST
(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) 2012-02-09 13:23:12 PST
Let's just make nsDebugImpl::SetMultiprocessMode main-thread only.  That'll simply things.
Comment 11 Justin Lebar (not reading bugmail) 2012-04-09 13:14:08 PDT
Created attachment 613366 [details] [diff] [review]
Patch v2
Comment 12 Justin Lebar (not reading bugmail) 2012-04-09 13:17:52 PDT
Created attachment 613369 [details] [diff] [review]
Patch v2.1

Remove unnecessary include.
Comment 13 Benjamin Smedberg [:bsmedberg] 2012-04-10 11:11:40 PDT
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) 2012-04-11 09:17:14 PDT
https://hg.mozilla.org/mozilla-central/rev/91bbeb1c5187

Note You need to log in before you can comment on or make changes to this bug.