1060292 - PR_Assert() and PR_Abort() output is not visible by default on Android

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

Both PR_Assert() and PR_Abort() invoke PR_LogPrint() unconditionally when they're called. PR_Assert() additionally explicitly writes to stderr so that its output is visible even if logging is entirely disabled (i.e. NSPR_LOG_MODULES is not set and thus logFile is null).

Since both function interrupt execution it's important that their output is readily available otherwise the application will die without an apparent reason (see bug 881389 comment 41).

We should explicitly write to the logcat directly on Android/FxOS in order for the calls' output to be always visible.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Make PR_Assert() and PR_Abort() output visible in the logcat on Android

This patch makes PR_Abort() print unconditionally to the logcat when invoked and makes PR_Assert() similarly use __android_log_assert() to unconditionally output to the logcat when invoked. This mirrors the behavior on other platforms that unconditionally print to stderr and then invoke an assertion-specific function that traps before aborting so that a debugger has a chance to catch it before the program closes.

Comment 2

[Wan-Teh Chang]

Comment on attachment 8481195 [details] [diff] [review]
[PATCH] Make PR_Assert() and PR_Abort() output visible in the logcat on Android

Review of attachment 8481195 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Thanks for the patch.

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/acccc321337d

::: pr/src/io/prlog.c
@@ +546,5 @@
>  PR_IMPLEMENT(void) PR_Assert(const char *s, const char *file, PRIntn ln)
>  {
>      PR_LogPrint("Assertion failure: %s, at %s:%d\n", s, file, ln);
>      fprintf(stderr, "Assertion failure: %s, at %s:%d\n", s, file, ln);
>      fflush(stderr);

Do you think we should skip the fprintf call on Android?

Comment 3

[Dave Hylands [:dhylands]]

(In reply to Wan-Teh Chang from comment #2)
> ::: pr/src/io/prlog.c
> @@ +546,5 @@
> >  PR_IMPLEMENT(void) PR_Assert(const char *s, const char *file, PRIntn ln)
> >  {
> >      PR_LogPrint("Assertion failure: %s, at %s:%d\n", s, file, ln);
> >      fprintf(stderr, "Assertion failure: %s, at %s:%d\n", s, file, ln);
> >      fflush(stderr);
> 
> Do you think we should skip the fprintf call on Android?

Normally, stderr is redirected to /dev/null on Android and B2G.

It is possible, under B2G, to launch B2G from an adb shell and in that case, stderr will be visible. So I think that there is possibly some merit in keeping and I can't see any disadvantage in keeping it.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Dave Hylands [:dhylands][on PTO - back Sep 11] from comment #3)
> Normally, stderr is redirected to /dev/null on Android and B2G.
> 
> It is possible, under B2G, to launch B2G from an adb shell and in that case,
> stderr will be visible. So I think that there is possibly some merit in
> keeping and I can't see any disadvantage in keeping it.

+1, agreed.