661282 - make xpcshell run on android

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: 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.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch for xpcshell support for omnijar

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to add script to run on android

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: 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

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to add script to run on android

this patch adds a 3rd required argument for the package name

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to add script to run on android

the objdir argument was being ignored, fixed that

Comment 6

[Geoff Brown [:gbrown]]

The script is missing a mkdir for /data/local/fennec/dist/lib.

Comment 7

[Benjamin Smedberg]

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*.

Comment 8

[cmtalbert]

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).

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

(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

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch for xpcshell support for omnijar

Comment 11

[Benjamin Smedberg]

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.

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/10625264d566

Comment 13

[Geoff Brown [:gbrown]]

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.)

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: 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

Comment 15

[Geoff Brown [:gbrown]]

Comment on attachment 548872 [details] [diff] [review]
follow up patch

This fixes the problem I reported - works fine for me.

Comment 16

[Geoff Brown [:gbrown]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d9456378c12d (follow-up patch)

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

backed out the push because of orange http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d9456378c12d

Comment 18

[Ed Morley [:emorley]]

Follow-up patch:
http://hg.mozilla.org/integration/mozilla-inbound/rev/da3b0e3fadc7

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/da3b0e3fadc7