Closed Bug 724072 Opened 8 years ago Closed 8 years ago

If using multiple processes, display pid along with NS_WARNINGs

Categories

(Core :: General, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(1 file, 2 obsolete files)

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?
They could, but since they don't use XPCOM I don't think they actually do.
What you propose sounds hard.  jduell made us process-specific NSPR log files, which should satisfy your use case here.
> 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.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #594797 - Flags: feedback?(jones.chris.g)
(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 on attachment 594797 [details] [diff] [review]
Patch v1

Looks good.  Use GetCurrentProcId() from base/process_util.h for the tagging.
Attachment #594797 - Flags: feedback?(jones.chris.g) → feedback+
Depends on: 725519
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.
Why is that necessary, can't those be static strings?
(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.
Let's just make nsDebugImpl::SetMultiprocessMode main-thread only.  That'll simply things.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #613366 - Flags: review?(benjamin)
Attachment #594797 - Attachment is obsolete: true
Assignee: nobody → justin.lebar+bug
Attached patch Patch v2.1Splinter Review
Remove unnecessary include.
Attachment #613366 - Attachment is obsolete: true
Attachment #613366 - Flags: review?(benjamin)
Attachment #613369 - Flags: review?(benjamin)
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.
Attachment #613369 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/91bbeb1c5187
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.