Last Comment Bug 661282 - make xpcshell run on android
: make xpcshell run on android
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: mozilla8
Assigned To: Brad Lassey [:blassey] (use needinfo?)
:
Mentors:
Depends on:
Blocks: 668349
  Show dependency treegraph
 
Reported: 2011-06-01 10:13 PDT by Brad Lassey [:blassey] (use needinfo?)
Modified: 2011-08-22 04:46 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.04 KB, patch)
2011-06-01 10:13 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for xpcshell support for omnijar (3.18 KB, patch)
2011-06-01 13:27 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch to add script to run on android (2.45 KB, patch)
2011-06-01 13:28 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for xpcshell support for omnijar (3.04 KB, patch)
2011-06-01 15:37 PDT, Brad Lassey [:blassey] (use needinfo?)
benjamin: review-
Details | Diff | Splinter Review
patch to add script to run on android (2.46 KB, patch)
2011-06-01 16:00 PDT, Brad Lassey [:blassey] (use needinfo?)
no flags Details | Diff | Splinter Review
patch to add script to run on android (2.42 KB, patch)
2011-06-01 16:35 PDT, Brad Lassey [:blassey] (use needinfo?)
cmtalbert: review-
Details | Diff | Splinter Review
patch for xpcshell support for omnijar (2.68 KB, patch)
2011-06-26 19:11 PDT, Brad Lassey [:blassey] (use needinfo?)
benjamin: review+
Details | Diff | Splinter Review
follow up patch (743 bytes, patch)
2011-07-27 12:36 PDT, Brad Lassey [:blassey] (use needinfo?)
benjamin: review+
gbrown: feedback+
Details | Diff | Splinter Review

Description Brad Lassey [:blassey] (use needinfo?) 2011-06-01 10:13:20 PDT
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.
Comment 1 Brad Lassey [:blassey] (use needinfo?) 2011-06-01 13:27:54 PDT
Created attachment 536717 [details] [diff] [review]
patch for xpcshell support for omnijar
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2011-06-01 13:28:59 PDT
Created attachment 536718 [details] [diff] [review]
patch to add script to run on android
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2011-06-01 15:37:57 PDT
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
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2011-06-01 16:00:39 PDT
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
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2011-06-01 16:35:25 PDT
Created attachment 536785 [details] [diff] [review]
patch to add script to run on android

the objdir argument was being ignored, fixed that
Comment 6 Geoff Brown [:gbrown] 2011-06-02 15:15:09 PDT
The script is missing a mkdir for /data/local/fennec/dist/lib.
Comment 7 Benjamin Smedberg [:bsmedberg] 2011-06-13 09:54:26 PDT
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 2011-06-16 17:17:17 PDT
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?) 2011-06-26 17:50:26 PDT
(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?) 2011-06-26 19:11:46 PDT
Created attachment 542066 [details] [diff] [review]
patch for xpcshell support for omnijar
Comment 11 Benjamin Smedberg [:bsmedberg] 2011-07-20 12:56:31 PDT
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 13 Geoff Brown [:gbrown] 2011-07-26 07:37:05 PDT
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?) 2011-07-27 12:36:26 PDT
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
Comment 15 Geoff Brown [:gbrown] 2011-07-27 13:23:00 PDT
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] 2011-08-10 14:12:03 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/d9456378c12d (follow-up patch)
Comment 17 Brad Lassey [:blassey] (use needinfo?) 2011-08-10 15:19:38 PDT
backed out the push because of orange http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d9456378c12d
Comment 18 Ed Morley [:emorley] 2011-08-22 01:10:19 PDT
Follow-up patch:
http://hg.mozilla.org/integration/mozilla-inbound/rev/da3b0e3fadc7
Comment 19 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-22 04:46:36 PDT
http://hg.mozilla.org/mozilla-central/rev/da3b0e3fadc7

Note You need to log in before you can comment on or make changes to this bug.