The default bug view has changed. See this FAQ.

make xpcshell run on android

RESOLVED FIXED in mozilla8

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: blassey, Assigned: blassey)

Tracking

Trunk
mozilla8
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Created attachment 536652 [details] [diff] [review]
patch

There are two discrete things in this patch. The first are changes to xpcshell and the XRE api to make xpcshell aware of omnijars. The second is a script to copy stuff to device and run xpcshell. I'll probably separate the two out when its time for review.
Created attachment 536717 [details] [diff] [review]
patch for xpcshell support for omnijar
Assignee: nobody → blassey.bugs
Attachment #536652 - Attachment is obsolete: true
Attachment #536717 - Flags: review?(benjamin)
Created attachment 536718 [details] [diff] [review]
patch to add script to run on android
Attachment #536718 - Flags: review?(ctalbert)
Created attachment 536761 [details] [diff] [review]
patch for xpcshell support for omnijar

this patch fixes the usage of NS_ERROR
with these patches, this command line:
python ../build/mobile/run-xpcshell.py ~/src/mozilla-central/objdir-droid-dbg /data/app/org.mozilla.fennec_unofficial-1.apk  -s -e "\"dump('hello\n')\""

produces this output (with a debug build):
adbd is already running as root
"cd /data/local/fennec/dist/lib/ && LD_LIBRARY_PATH=/data/local/fennec/dist/lib/ GRE_HOME=/data/data/org.mozilla.fennec_unofficial  ./xpcshell --greomni /data/app/org.mozilla.fennec_unofficial-1.apk -e "dump('hello\n')""
WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/blassey/src/mozilla-central/xpcom/base/nsTraceRefcntImpl.cpp, line 172
got jar reader: 0x40215800
WARNING: No default pref files found.: file /home/blassey/src/mozilla-central/modules/libpref/src/Preferences.cpp, line 789
hello
WARNING: nsExceptionService ignoring thread destruction after shutdown: file /home/blassey/src/mozilla-central/xpcom/base/nsExceptionService.cpp, line 197
nsStringStats
 => mAllocCount:           1408
 => mReallocCount:            7
 => mFreeCount:            1408
 => mShareCount:           6937
 => mAdoptCount:             53
 => mAdoptFreeCount:         53
WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/blassey/src/mozilla-central/xpcom/base/nsTraceRefcntImpl.cpp, line 172
Attachment #536717 - Attachment is obsolete: true
Attachment #536717 - Flags: review?(benjamin)
Attachment #536761 - Flags: review?(benjamin)
Created attachment 536774 [details] [diff] [review]
patch to add script to run on android

this patch adds a 3rd required argument for the package name
Attachment #536718 - Attachment is obsolete: true
Attachment #536718 - Flags: review?(ctalbert)
Attachment #536774 - Flags: review?(ctalbert)
OS: Linux → Android
Hardware: x86_64 → All
Created attachment 536785 [details] [diff] [review]
patch to add script to run on android

the objdir argument was being ignored, fixed that
Attachment #536774 - Attachment is obsolete: true
Attachment #536774 - Flags: review?(ctalbert)
Attachment #536785 - Flags: review?(ctalbert)
The script is missing a mkdir for /data/local/fennec/dist/lib.
Comment on attachment 536761 [details] [diff] [review]
patch for xpcshell support for omnijar

Why doesn't this end up initializing omnijar twice? I don't understand exactly why xpcshell can't find omnijar on android by default, but I really don't understand how this is safe: mozilla::Omnijar::Init is not safe for dual init, as far as I can tell.

Also I really prefer XRE_InitOmnijar to take localfiles, not char*.
Attachment #536761 - Flags: review?(benjamin) → review-

Comment 8

6 years ago
Comment on attachment 536785 [details] [diff] [review]
patch to add script to run on android

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

Sorry it took so long to get to the review.  What you've got here is a great start to getting xpcshell running on the device.  I think that there is a bit of stuff in the patch that looks like its catered to your system.  Once that's cleaned up I think this will work as a general way to launch xpcshell on device.

I have two general questions:
* Did you try this on a tegra?  I got "CANNOT LINK EXECUTABLE" when I tried it there.  But I've had trouble running straight C++ code on tegras before, so it could work fine on a real phone.
* A bunch of xpcshell test files print stuff to the console for debugging and status.  Were you planning to extend this to handle that ability or to allow dumping the stdout/stderr of xpcshell onto a file on the phone which could be pulled back to the local dir from the calling computer? (could be a follow-on bug).

::: build/mobile/run-xpcshell.py
@@ +9,5 @@
> +
> +def setup():
> +    # the main code goes here
> +    objdir=sys.argv[1]
> +    dirList=os.listdir(objdir + "/dist/fennec")

Is this indeed the proper directory?  I believe that this works for you, but I don't have a dist/fennec in my fennec objdir.  I'm rebuilding now (my build was old) so perhaps that has changed.  Are you doing a make package in addition to the normal build?

@@ +12,5 @@
> +    objdir=sys.argv[1]
> +    dirList=os.listdir(objdir + "/dist/fennec")
> +    subprocess.check_call(["adb", "shell", "mkdir", "/data/local/fennec/"])
> +    subprocess.check_call(["adb", "shell", "mkdir", "/data/local/fennec/dist"])
> +    subprocess.check_call(["adb", "shell", "mkdir", "/data/local/fennec/dist/lib/defaults/"])

I think you have to make /data/local/fennec/dist/lib before you attempt to make lib/defaults.

@@ +18,5 @@
> +    for fname in dirList:
> +        flen = len(fname)
> +        if fname[flen - 3:]  == ".so":
> +            print fname
> +            copy(objdir + "/dist/fennec/", "/data/local/fennec/dist/lib/", fname );

For the local system's path, I'd use os.path.join(<path>, <path>), in case someone tries this on windows.

@@ +24,5 @@
> +    copy(objdir + "/dist/bin/", "/data/local/fennec/dist/lib/", "xpcshell")
> +
> +def main():
> +    if len(sys.argv) < 2:
> +        print "usage: python xpcshell <path to objdir> <path to apk on device> <package name> [-g] [-s] [xpcshell arguments]"

nit: I don't think the xpcshell arguments are optional.  Things fail if you don't have at least 4 arguments.  Also, I can't figure what you'd do running xpcshell without arguments.

@@ +39,5 @@
> +    subprocess.check_call(["adb", "forward", "tcp:12345", "tcp:12345"])
> +    cmd = "cd /data/local/fennec/dist/lib/ && LD_LIBRARY_PATH=/data/local/fennec/dist/lib/ GRE_HOME=/data/data/"
> +    cmd += sys.argv[3]
> +    if use_gdbserver:
> +        cmd += " /data/local/gdbserver localhost:12345"

nit: Making an assumption about where gdbserver is installed, I'd mention that up in the help (Though I'm not sure it *can* be installed anywhere else on the phone...if that's the case, ignore this).
Attachment #536785 - Flags: review?(ctalbert) → review-
(In reply to comment #7)
> Comment on attachment 536761 [details] [diff] [review] [review]
> patch for xpcshell support for omnijar
> 
> Why doesn't this end up initializing omnijar twice? 
This check prevents omnijar from being initialized twice:
http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#460

> I don't understand
> exactly why xpcshell can't find omnijar on android by default, 
Why would it? This is akin to providing a library loading path and GRE_HOME

> but I really
> don't understand how this is safe: mozilla::Omnijar::Init is not safe for
> dual init, as far as I can tell.
Its not, but it doesn't happen

> 
> Also I really prefer XRE_InitOmnijar to take localfiles, not char*.
No problem
Created attachment 542066 [details] [diff] [review]
patch for xpcshell support for omnijar
Attachment #536761 - Attachment is obsolete: true
Attachment #542066 - Flags: review?(benjamin)

Updated

6 years ago
Blocks: 668349
Comment on attachment 542066 [details] [diff] [review]
patch for xpcshell support for omnijar

This should really be a private API, but I guess you can stick in the XRE_ namespace for now.
Attachment #542066 - Flags: review?(benjamin) → review+
http://hg.mozilla.org/mozilla-central/rev/10625264d566
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
A probably unintended feature of the patch that landed is that now both --greomni and --appomni must be specified. If only --greomni is specified, I get a segmentation violation. (The original patch set appomni==greomni if only --greomni was specified.)
Created attachment 548872 [details] [diff] [review]
follow up patch

Geoff points out that the behavior of this changed between the first and second patch. This brings back the original handling of not specifying --appomni
Attachment #548872 - Flags: review?(benjamin)
Attachment #548872 - Flags: feedback?(gbrown)
Comment on attachment 548872 [details] [diff] [review]
follow up patch

This fixes the problem I reported - works fine for me.
Attachment #548872 - Flags: feedback?(gbrown) → feedback+
Attachment #548872 - Flags: review?(benjamin) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/d9456378c12d (follow-up patch)
Whiteboard: [inbound]
backed out the push because of orange http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d9456378c12d
Whiteboard: [inbound]

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: check in needed for "follow up patch"; see also bug 668351
Follow-up patch:
http://hg.mozilla.org/integration/mozilla-inbound/rev/da3b0e3fadc7
Keywords: checkin-needed
Whiteboard: check in needed for "follow up patch"; see also bug 668351 → [follow up patch on inbound]
http://hg.mozilla.org/mozilla-central/rev/da3b0e3fadc7
Whiteboard: [follow up patch on inbound]
You need to log in before you can comment on or make changes to this bug.