Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 11

Status

Fennec Graveyard
General
--
enhancement
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: alexp, Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 11
All
Android
Dependency tree / graph

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

7 years ago
stdout on Android is not redirected to the device log, so we need to use __android_log_print to write the output there.
(Reporter)

Comment 1

7 years ago
Created attachment 464487 [details] [diff] [review]
Fix

Added __android_log_print call to nsGlobalWindow::Dump().
Attachment #464487 - Flags: review?(blassey.bugs)
(Reporter)

Updated

7 years ago
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-
(Reporter)

Comment 3

7 years ago
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?
(Reporter)

Comment 4

7 years ago
Created attachment 465955 [details] [diff] [review]
dump-dom

DOM dump() fix - updated as per review.
Attachment #464487 - Attachment is obsolete: true
(Reporter)

Comment 5

7 years ago
Created attachment 465956 [details] [diff] [review]
dump-components

Components dump() fix.
(Reporter)

Comment 6

7 years ago
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.
Created attachment 492312 [details] [diff] [review]
dump-messagemanager
Attachment #492312 - Attachment is patch: true
Attachment #492312 - Attachment mime type: text/x-c++src → text/plain
(Reporter)

Comment 8

6 years ago
Created attachment 550120 [details] [diff] [review]
Combined patch

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!
Created attachment 563102 [details]
Bootstrap code that allow to dump to logcat

Here is the jsctypes hack.
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.
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
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?
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 15

6 years ago
> 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.

Comment 17

6 years ago
Why are some of those printing just to the log on android while others are going to both the log and stdout/stderr?
(Reporter)

Comment 18

6 years ago
(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

6 years ago
Justin, could you make those consistent please?
(Assignee)

Comment 20

6 years ago
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.
Attachment #573303 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #550120 - Attachment is obsolete: true
Attachment #550120 - Flags: review?(bzbarsky)

Comment 21

6 years ago
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+
(Assignee)

Comment 22

6 years ago
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!)
status-firefox11: --- → fixed
(Reporter)

Comment 23

6 years ago
(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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
(Assignee)

Updated

6 years ago
Assignee: alexp → justin.lebar+bug

Updated

6 years ago
Blocks: 706130
Blocks: 709468
You need to log in before you can comment on or make changes to this bug.