Closed Bug 770915 Opened 8 years ago Closed 6 years ago

Sandbox's dump() doesn't appear in adb logcat

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: ochameau, Assigned: ochameau)

Details

Attachments

(1 file)

Bug 586010 allowed to get various stdio function to work properly and appear in adb logcat. But we missed Sandbox's dump() method.

So that, if you do:
  Components.utils.Sandbox("http://foo.com").dump("bar");
Nothing is going to appear in logcat.

Having said that, as suggested in bug 586010 comment 14, instead of adding some specific calls to  __android_log_print/__android_log_write in any possible function playing with stdout/stderr, it would be better to call dalvik's method `dvmStdioConverterStartup`:
https://github.com/android/platform_dalvik/blob/master/vm/StdioConverter.cpp#L50

It will be even better to call this method, only when "log.redirect-stdio" property is set to true. As described here:
http://developer.android.com/tools/debugging/debugging-log.html#viewingStd

It would make us behave like a regular Android application and avoid patching all possible function playing with stdout/stderr! It might even improve performances as we will avoid calling there __android_log* methods.
(In reply to Alexandre Poirot (:ochameau) from comment #0)
> It will be even better to call this method, only when "log.redirect-stdio"
> property is set to true. As described here:
> http://developer.android.com/tools/debugging/debugging-log.html#viewingStd
> 
> It would make us behave like a regular Android application and avoid
> patching all possible function playing with stdout/stderr! It might even
> improve performances as we will avoid calling there __android_log* methods.

Setting log.redirect-stdio to true should already redirect stdout/stderr to logcat.
As for performance, the android_log* functions are still called with log.redirect-stdio=true, just on a separate thread.
(In reply to Mike Hommey [:glandium] from comment #1)
> Setting log.redirect-stdio to true should already redirect stdout/stderr to
> logcat.

Setting it to true or false doesn't seem to change anything to Firefox behavior.
Is Components.utils.Sandbox("http://foo.com").dump("bar\n"); appearing in logcat when set to true? (I tried without success)
(In reply to Alexandre Poirot (:ochameau) from comment #3)
> (In reply to Mike Hommey [:glandium] from comment #1)
> > Setting log.redirect-stdio to true should already redirect stdout/stderr to
> > logcat.
> 
> Setting it to true or false doesn't seem to change anything to Firefox
> behavior.

Did you restart android?
Component: General → XPConnect
Product: Fennec Graveyard → Core
Assignee: nobody → poirot.alex
Comment on attachment 828897 [details] [diff] [review]
Call the magic android method to make it appear

That will allow all sdk module (like all devtools one) to have a working dump on b2g \o/
Attachment #828897 - Flags: review?(gkrizsanits)
Comment on attachment 828897 [details] [diff] [review]
Call the magic android method to make it appear

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

I see nothing wrong with this patch, but I have no idea if it's good or bad to use the same tag ("GeckoDump") as the nsGlobalWindow version of dump. So if you can double check that with some b2g folks, I'm fine with this.
Attachment #828897 - Flags: review?(gkrizsanits) → review+
Comment on attachment 828897 [details] [diff] [review]
Call the magic android method to make it appear

I'm just replicating what is being done for window.dump()

But you are right, I have no clue why {xpcom,jsm}.dump() uses regular Gecko:
  http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSComponentLoader.cpp#109
whereas window.dump() uses GeckoDump:
  http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#5655

I can easily switch to just Gecko...
Attachment #828897 - Flags: feedback?(anygregor)
Comment on attachment 828897 [details] [diff] [review]
Call the magic android method to make it appear

Looks fine to me but mwu knows the ifdef configurations much better.
Attachment #828897 - Flags: feedback?(anygregor) → feedback?(mwu)
Comment on attachment 828897 [details] [diff] [review]
Call the magic android method to make it appear

Seems ok.
Attachment #828897 - Flags: feedback?(mwu) → feedback+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae2fbe7075ce
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.