Closed Bug 586010 Opened 10 years ago Closed 8 years ago

dump() output from JavaScript is not visible in Android device log

Categories

(Firefox for Android Graveyard :: General, enhancement)

All
Android
enhancement
Not set

Tracking

(firefox11 fixed)

RESOLVED FIXED
Firefox 11
Tracking Status
firefox11 --- fixed

People

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

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

stdout on Android is not redirected to the device log, so we need to use __android_log_print to write the output there.
Attached patch Fix (obsolete) — Splinter Review
Added __android_log_print call to nsGlobalWindow::Dump().
Attachment #464487 - Flags: review?(blassey.bugs)
Assignee: nobody → alexp
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
Attachment #464487 - Flags: review?(blassey.bugs) → review-
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?
Attached patch dump-dom (obsolete) — Splinter Review
DOM dump() fix - updated as per review.
Attachment #464487 - Attachment is obsolete: true
Attached patch dump-components (obsolete) — Splinter Review
Components dump() fix.
Attached patch [WIP] dump-xpcshell (obsolete) — Splinter Review
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.
Attachment #492312 - Attachment is patch: true
Attachment #492312 - Attachment mime type: text/x-c++src → text/plain
Attached patch Combined patch (obsolete) — Splinter Review
The patch, which combines all the changes, in case someone still needs this.
Attachment #465955 - Attachment is obsolete: true
Attachment #465956 - Attachment is obsolete: true
Attachment #465957 - Attachment is obsolete: true
Attachment #492312 - Attachment is obsolete: true
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!
Here is the jsctypes hack.
And a small addon that ship this bootstrap file and allow to see dump() message in logcat.
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 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?
Attachment #550120 - Flags: review?(bzbarsky)
(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.
> 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!
(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.
Why are some of those printing just to the log on android while others are going to both the log and stdout/stderr?
(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.
Justin, could you make those consistent please?
Attached patch Patch v2Splinter Review
Sure.  I made them consistently send to both the device log and wherever else, since we might be logging to a file.
Attachment #573303 - Flags: review?(bzbarsky)
Attachment #550120 - Attachment is obsolete: true
Attachment #550120 - Flags: review?(bzbarsky)
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.
Attachment #573303 - Flags: review?(bzbarsky) → review+
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!)
(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. ;)
https://hg.mozilla.org/mozilla-central/rev/525329d5642a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
Assignee: alexp → justin.lebar+bug
Blocks: 706130
You need to log in before you can comment on or make changes to this bug.