Last Comment Bug 586010 - dump() output from JavaScript is not visible in Android device log
: dump() output from JavaScript is not visible in Android device log
Status: RESOLVED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All Android
: -- enhancement (vote)
: Firefox 11
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 706130 709468
  Show dependency treegraph
 
Reported: 2010-08-10 11:08 PDT by Alex Pakhotin (:alexp)
Modified: 2013-10-09 07:12 PDT (History)
10 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix (1.18 KB, patch)
2010-08-10 11:16 PDT, Alex Pakhotin (:alexp)
blassey.bugs: review-
Details | Diff | Review
dump-dom (1.26 KB, patch)
2010-08-13 23:18 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Review
dump-components (1.83 KB, patch)
2010-08-13 23:19 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Review
[WIP] dump-xpcshell (1.34 KB, patch)
2010-08-13 23:23 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Review
dump-messagemanager (1.19 KB, patch)
2010-11-22 06:55 PST, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Review
Combined patch (4.79 KB, patch)
2011-08-02 10:33 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Review
Bootstrap code that allow to dump to logcat (2.60 KB, text/javascript)
2011-09-28 10:23 PDT, Alexandre Poirot [:ochameau]
no flags Details
A small addon that ship this bootstrap. (1.74 KB, application/x-xpinstall)
2011-09-28 10:26 PDT, Alexandre Poirot [:ochameau]
no flags Details
Patch v2 (4.56 KB, patch)
2011-11-09 13:29 PST, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Review

Description Alex Pakhotin (:alexp) 2010-08-10 11:08:03 PDT
stdout on Android is not redirected to the device log, so we need to use __android_log_print to write the output there.
Comment 1 Alex Pakhotin (:alexp) 2010-08-10 11:16:14 PDT
Created attachment 464487 [details] [diff] [review]
Fix

Added __android_log_print call to nsGlobalWindow::Dump().
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2010-08-12 15:11:19 PDT
Comment on attachment 464487 [details] [diff] [review]
Fix

As mentioned on irc, if gDumpFile is not null, we should use that. So this could be:

ifdef ANDROID
  if (!gDumpFile) {
    __android_log_print(ANDROID_LOG_INFO, "Gecko" , cstr);
    return NS_OK;
  }
#endif

This should probably be reviewed by a dom peer though
Comment 3 Alex Pakhotin (:alexp) 2010-08-12 23:44:16 PDT
There are two other Dump() functions in components and xpcshell:
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#188
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/shell/xpcshell.cpp#435

I've found at least the first one useful for components debugging.
Do you think we should add similar code to these?
Comment 4 Alex Pakhotin (:alexp) 2010-08-13 23:18:33 PDT
Created attachment 465955 [details] [diff] [review]
dump-dom

DOM dump() fix - updated as per review.
Comment 5 Alex Pakhotin (:alexp) 2010-08-13 23:19:38 PDT
Created attachment 465956 [details] [diff] [review]
dump-components

Components dump() fix.
Comment 6 Alex Pakhotin (:alexp) 2010-08-13 23:23:22 PDT
Created attachment 465957 [details] [diff] [review]
[WIP] dump-xpcshell

A similar fix for the dump() function used in xpcshell, but a better fix in this application would be a complete redirection of gOutFile to Android log.
This patch is optional, and could be used for testing purposes.
Comment 7 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-11-22 06:55:48 PST
Created attachment 492312 [details] [diff] [review]
dump-messagemanager
Comment 8 Alex Pakhotin (:alexp) 2011-08-02 10:33:24 PDT
Created attachment 550120 [details] [diff] [review]
Combined patch

The patch, which combines all the changes, in case someone still needs this.
Comment 9 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-09-28 09:16:00 PDT
I think this patch should honor the android way of redirecting stdout/stderr to logcat:

$ adb shell stop
$ adb shell setprop log.redirect-stdio true
$ adb shell start

If this preference is set a -Xlog-stdio is passed to the dalvik vm and output redirecting is activated. Actually this redirecting does not work for Android because there is a hack in the dalvik vm that does the redirection by hand: https://github.com/android/platform_dalvik/blob/master/vm/StdioConverter.c

So if we can call the dvmStdioConverterStartup() symbol, that live in libdv, if the log.redirect-stdio properties is set we will have the redirection for free.
Alexandre Poirot has build a little js-ctypes proto for jetpack that does that (except it does not check if the property is checked or not) and it seems to work well.
Someone could even build an addon for fennec with it in the meantime and it will allow us to use dump() without having to modify native code!
Thanks js-ctypes!
Comment 10 Alexandre Poirot [:ochameau] 2011-09-28 10:23:35 PDT
Created attachment 563102 [details]
Bootstrap code that allow to dump to logcat

Here is the jsctypes hack.
Comment 11 Alexandre Poirot [:ochameau] 2011-09-28 10:26:31 PDT
Created attachment 563103 [details]
A small addon that ship this bootstrap.

And a small addon that ship this bootstrap file and allow to see dump() message in logcat.
Comment 12 Justin Lebar (not reading bugmail) 2011-11-08 07:48:39 PST
We need to resolve this bug, please.  It makes debugging JS code on b2g basically impossible.

I'd prefer not to have to figure out how to bootstrap an addon into my b2g image.  I'd rather we made dump() work properly without an addon.
Comment 13 Justin Lebar (not reading bugmail) 2011-11-08 07:55:47 PST
Comment on attachment 550120 [details] [diff] [review]
Combined patch

Vivien, I don't really understand the Android Right Way argument.  But this patch looks like something we can have Right Now, which works with very little magic.

What's the downside to taking this patch now?
Comment 14 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-11-08 08:11:45 PST
(In reply to Justin Lebar [:jlebar] from comment #13)
> Comment on attachment 550120 [details] [diff] [review] [diff] [details] [review]
> Combined patch
> 
> What's the downside to taking this patch now?

It's means we're not going to honor the system property log.redirect-stdio that is false by default to not pollute the log.
Normally users that want stdout into their logcat file have the choice to redirect the output of logcat to a file (and in this case stdout will be redirect to it) or to |$ adb shell setprop log.redirect-stdio true| and the output will be in logcat.

Honestly I don't think we should block on that but I would like the platform to honor this pref and the js-ctypes code can do it very easily.
Comment 15 Justin Lebar (not reading bugmail) 2011-11-08 08:14:06 PST
> It's means we're not going to honor the system property log.redirect-stdio that
> is false by default to not pollute the log.

But we're not redirecting any and all stdout to the log; we're just redirecting dump() calls to the log.  In consistency typical of our code, dump is (apparently) sent to stderr() sometimes!
Comment 16 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-11-08 08:28:16 PST
(In reply to Justin Lebar [:jlebar] from comment #15)
> > It's means we're not going to honor the system property log.redirect-stdio that
> > is false by default to not pollute the log.
> 
> But we're not redirecting any and all stdout to the log; we're just
> redirecting dump() calls to the log.  In consistency typical of our code,
> dump is (apparently) sent to stderr() sometimes!

I'm fine with the current patch as a workaround but it's look like a hack to me.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-08 19:14:35 PST
Why are some of those printing just to the log on android while others are going to both the log and stdout/stderr?
Comment 18 Alex Pakhotin (:alexp) 2011-11-08 19:58:38 PST
(In reply to Boris Zbarsky (:bz) from comment #17)
> Why are some of those printing just to the log on android while others are
> going to both the log and stdout/stderr?

Not intentionally - those pieces were just implemented in different time for internal purposes, so might be a bit inconsistent.

There was a discussion about these changes in general, and I think some alternative solution was suggested and implemented, though I still use this approach for my debugging, so don't really have enough information about the alternatives.
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-08 20:08:49 PST
Justin, could you make those consistent please?
Comment 20 Justin Lebar (not reading bugmail) 2011-11-09 13:29:44 PST
Created attachment 573303 [details] [diff] [review]
Patch v2

Sure.  I made them consistently send to both the device log and wherever else, since we might be logging to a file.
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-09 13:57:27 PST
Comment on attachment 573303 [details] [diff] [review]
Patch v2

r=me though I wonder whether a helper somewhere that takes a char* and fd and does the right thing is worthwhile.
Comment 22 Justin Lebar (not reading bugmail) 2011-11-09 16:53:33 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/525329d5642a

(Alex, the patch should have credited you as the author; hg changed it on me and I didn't check before I pushed.  I'm sorry!)
Comment 23 Alex Pakhotin (:alexp) 2011-11-09 17:34:50 PST
(In reply to Justin Lebar [:jlebar] from comment #22)

> (Alex, the patch should have credited you as the author; hg changed it on me
> and I didn't check before I pushed.  I'm sorry!)

So, if there is anything wrong with this, you will be blamed, not me. ;)
Comment 24 Marco Bonardo [::mak] 2011-11-10 03:27:58 PST
https://hg.mozilla.org/mozilla-central/rev/525329d5642a

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