243870 - Suppress stderr output in release builds for components that cannot be loaded.

Status: NEW

(Core :: XPCOM, defect)

Description

[Darin Fisher]

Suppress stderr output in release builds for components that cannot be loaded.

Given that there are now some optional components that depend on system
libraries that may or may not be present on the end-users machine that we should
disable the fprintf to stderr that exists in the native component loader.  NSPR
logging should be sufficient to help debug problems in release builds.

http://lxr.mozilla.org/mozilla/source/xpcom/components/nsNativeComponentLoader.cpp#531

I think this error reporting just makes our release products look less professional.

Comment 1

[Darin Fisher]

See for example th first post to mozillazine.org regarding 1.7rc2:

http://mozillazine.org/talkback.html?article=4752#17

It's clear that folks easily misunderstand the significance of this error message.

Comment 2

[timeless]

erm, we intentionally kept it as standard out logging. why are end users looking
at a console to begin with?

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Perhaps because our Linux tarballs don't install themselves into the menu, for
starters.

(With Darin on this.)

Comment 4

[Akkana Peck]

I'm with Darin, too.  Spewing on stdout/stderr makes no sense in a release build
and makes us look like we don't care about end users.  If it's important enough
to bother the user, pop up a dialog.

Re "why are users looking at a console?": if we expect them not to be looking,
then what good is showing a message there?

Expert users can turn on logging if need be (though I wish we made it easier for
them, perhaps something like a --debug flag that turned on nspr logging, but
that's another issue.)

Comment 5

[Darin Fisher]

akk: yeah, i agree... a debug option like that would be great!

Comment 6

[Asa Dotzler [:asa]]

who can help here? time is getting short for 1.7.

Comment 7

[Mike Kaply [:mkaply]]

We should just remove this printf for the branch.

It's logged if PR_LOGGING is on and this appears to be an "always compiled" log
item.

Comment 8

[chris hofmann]

think seawood has helped clean up things like this in the past.  if we are going
to do this for 1.7 we need it quickly and darin is out-of-town, then swamped
with a few other 1.7 blockers.   need some help mas rapido..

Comment 9

[Akkana Peck]

Attachment: Put #ifdef DEBUG around the spew

Maybe I'm missing something ... is there something needed beyond the trivial
action of putting #ifdef DEBUG around the offending printf, now that we all
agree it should go?

There's already a PR_LOG right after it, so nothing should need to be added
there.

Here's a patch to do that.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 149422 [details] [diff] [review]
Put #ifdef DEBUG around the spew

sr=shaver.

Comment 11

[cls]

Comment on attachment 149422 [details] [diff] [review]
Put #ifdef DEBUG around the spew

r=cls

FWIW, a handful of those "don't print to console in opt builds" bugs are still
open, including the one for xpcom (bug 78667).

Comment 12

[chris hofmann]

Comment on attachment 149422 [details] [diff] [review]
Put #ifdef DEBUG around the spew

a=chofmann for 1.7  -- thanks for the quick turn around

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 149422 [details] [diff] [review]
Put #ifdef DEBUG around the spew

a=asa (on behalf of drivers) for checkin to 1.7

Comment 14

[Akkana Peck]

Fix checked in to trunk and branch.

Comment 15

[Darin Fisher]

FORCE_PR_LOG is not defined anywhere in xpcom/components, so i suspect that the
PR_LOG statements are not enabled in release builds.  that means that in release
builds there is maybe no way to determine that this error is occuring (well, on
linux you could check the /proc/<pid>/maps file)... perhaps we want to define
FORCE_PR_LOG in a MOZ_LOGGING section?

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

reopening to get comment 15 addressed

Comment 17

[cls]

Attachment: Use MOZ_LOGGING (checked in on trunk)

A FORCE_PR_LOG define was added in bug 316416.  It needs to be wrapped with MOZ_LOGGING.

Comment 18

[Benjamin Smedberg]

cls, what does this last patch actually do? I'm confused.

Comment 19

[cls]

If you only define FORCE_PR_LOG, then logging is enabled in all builds.  The MOZ_LOGGING ifdef allows you to turn off NSPR logging support in builds by using --disable-logging (also used by the embedding profiles).

Comment 20

[cls]

Comment on attachment 218243 [details] [diff] [review]
Use MOZ_LOGGING (checked in on trunk)

The FORCE_PR_LOG was only added on the trunk.  Do we want the full ifdef on the 1.7 & 1.8 branches as well?

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

that'd be really nice imo (what about MOZILLA_1_8_BRANCH?)

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 218243 [details] [diff] [review]
Use MOZ_LOGGING (checked in on trunk)

This patch wouldn't apply to the branch, which doesn't have the FORCE_PR_LOG in the first place. Need some sort of rationale for how this fits the build criteria for the stability/security branches before we could approve it.

Comment 23

[Nochum Sossonko [:Natch]]

Can this be closed?