Closed Bug 661282 Opened 9 years ago Closed 8 years ago

make xpcshell run on android

Categories

(Core :: XPConnect, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(3 files, 5 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → blassey.bugs
Attachment #536652 - Attachment is obsolete: true
Attachment #536717 - Flags: review?(benjamin)
Attachment #536718 - Flags: review?(ctalbert)
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)
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
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 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
Attachment #536761 - Attachment is obsolete: true
Attachment #542066 - Flags: review?(benjamin)
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
Closed: 8 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.)
Attached patch follow up patchSplinter Review
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+
backed out the push because of orange http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d9456378c12d
Whiteboard: [inbound]
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]
You need to log in before you can comment on or make changes to this bug.