1168407 - Make JavaAddonManager interface bi-directional

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

For a long time, Fennec has supported loading arbitrary Java code (in the form of DEX files): Bug 794479.  There are additional tickets (such as Bug 799631) to flesh out an interface for Java Addons.  This ticket tracks implementing one such interface: making the JavaAddonManager interface bi-directional.

That means that a Java Addon will be able to both receive and respond to messages from Gecko, but also send a message to Gecko (and get a response).

One possible use case: an addon wishes to integrate with an existing APK on device (such as the barcode scanning software ZXing).  The Gecko -> Java interface is used to respond to user requests to scan a barcode.  The Java -> Gecko interface might be used to notify Fennec when ZXing settings are modified.

Comment 1

[Nick Alexander :nalexander [he/him]]

This will need developer docs on MDN, anchored at https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API.  If time allows, documenting Messaging.jsm would be extremely valuable as well.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1168407 - Pre: Move roboextender under mobile/android/tests. r?gbrown

Bug 1168407 - Implement a bidirectional Java addon interface. r=kats,rnewman

There are several parts to this ticket:

1) Produce javaaddons-1.0.jar, a standalone JAR defining a (versioned)
Java interface suitable for consumption by third-party Java addon
implementations.

2) Support the new V1 interface in the JavaAddonManager.

3) Add JUnit 3 tests of the class-finding functionality.  This
includes building a test DEX file that implements the V1 interface
(consuming the built JAR).  This is both a functional test and living
documentation of how to produce such an addon.

4) Add Robocop JavascriptTests testing the JavaScript message passing
interface to and from Java.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #2)
> Created attachment 8614305 [details]
> MozReview Request: Bug 1168407 - Implement a bidirectional Java addon
> interface. r=kats,rnewman
> 
> Bug 1168407 - Implement a bidirectional Java addon interface. r=kats,rnewman
> 
> There are several parts to this ticket:
> 
> 1) Produce javaaddons-1.0.jar, a standalone JAR defining a (versioned)
> Java interface suitable for consumption by third-party Java addon
> implementations.
> 
> 2) Support the new V1 interface in the JavaAddonManager.
> 
> 3) Add JUnit 3 tests of the class-finding functionality.  This
> includes building a test DEX file that implements the V1 interface
> (consuming the built JAR).  This is both a functional test and living
> documentation of how to produce such an addon.
> 
> 4) Add Robocop JavascriptTests testing the JavaScript message passing
> interface to and from Java.

rnewman: preliminary pass, when you get a moment.  You too, mfinkle.  As discussed with rnewman, this sandboxes each Java addon behind a unique GUID.  The motivation is to make all Gecko interactions explicit, so that reviewers can see the flow between Gecko and the Java addon.  (Previous approaches let Java addons sip the Gecko event stream.  They still can, they just need to be explicit in the JavaScript that does it.)

Known issues:

* Commenting is minimal.  So is MDN-style whole feature documentation.  It'll come.

* The JUnit 3 test is out of date.  There is some automated testing in place; it seems to work.

* It's hard to access classes.dex in an addon right now.  I'll add URI resolution and GeckoJarReader support into the Java side next, but I want to get a demo out before adding this polish.

* I'm not providing a Context to the addon constructor yet.  We could also expose this via the event dispatcher.  In some way, the Java code will want one.

* I've largely ignored thread considerations.  I'm not really certain what the right model is; I don't think it wouldn't be bad to assert that Java code invocations always happen on the main thread, and to act accordingly, because lots of Android code needs to happen on the main thread.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8614305 [details]
MozReview Request: Bug 1168407 - Pre: Move roboextender under mobile/android/tests. r?gbrown

https://reviewboard.mozilla.org/r/9879/#review8775

::: mobile/android/base/EventDispatcher.java:164
(Diff revision 1)
> +                // There were native listeners, and they're gone.  Dispatch an error rather than

Get a rubberstamp from nchen.

::: mobile/android/base/JavaAddonManagerV1.java:6
(Diff revision 1)
> +package org.mozilla.gecko;

I wish this and JAM.java were in a subpackage/subdir…

::: mobile/android/base/JavaAddonManagerV1.java:40
(Diff revision 1)
> +    private final Map<String, JavaAddonEventDispatcherV1Impl> mGuidToDispatcherMap = new HashMap<>();

s/Guid/GUID/g

::: mobile/android/base/JavaAddonManagerV1.java:55
(Diff revision 1)
> +            // we've already done this registration. don't do it again

Nit: capitals and punctuation.

::: mobile/android/base/JavaAddonManagerV1.java:54
(Diff revision 1)
> +        if (mApplicationContext != null) {

You haven't defined a threading policy for `init`, so you can get double inits (or worse).

Either impose a thread policy for init, or you need to make mApplicationContext an AtomicReference.

::: mobile/android/base/JavaAddonManagerV1.java:71
(Diff revision 1)
> +                .class);

Remove the unnecessary linebreak.

::: mobile/android/base/JavaAddonManagerV1.java:82
(Diff revision 1)
> +            if (event.equals("JavaAddonManagerV1:Load")) {

Extract constants, use string switch.

::: mobile/android/base/JavaAddonManagerV1.java:111
(Diff revision 1)
> +     * Curiously, the dispatcher does not hold a direct reference to its instance.  It will

It would help to clarify here: "add-on instance".

::: mobile/android/base/JavaAddonManagerV1.java:125
(Diff revision 1)
> +            this.dexFileName = dexFileName;

This is never used.

::: mobile/android/base/JavaAddonManagerV1.java:116
(Diff revision 1)
> +    private class JavaAddonEventDispatcherV1Impl implements JavaAddonEventDispatcherV1 {

There's nothing about this that is specific to JavaAddonEventListenerV1, really. You could make this

```
private class PrefixedEventDispatcher<T: NativeEventListener> ...
```

right?

::: mobile/android/base/JavaAddonManagerV1.java:120
(Diff revision 1)
> +        // Both protected by synchronized(this) blocks.

Both?

::: mobile/android/base/JavaAddonManagerV1.java:135
(Diff revision 1)
> +            final NativeEventListener listenerWrapper = new NativeEventListener() {

I'd lift this out and make it static, with explicit constructor parameters. You're probably capturing a big closure here.

::: mobile/android/base/JavaAddonManagerV1.java:192
(Diff revision 1)
> +                it.remove();

Why bother? You're unregistering everything in a synchronized block. Just call .clear once!

::: mobile/android/base/JavaAddonManagerV1.java:202
(Diff revision 1)
> +                    if (callback == null) {

Do this check *outside* your anonymous class!

::: mobile/android/config/proguard/proguard.cfg:214
(Diff revision 1)
> +-keep class org.mozilla.javaddons.* {

Typo: javaddons -> javaaddons.

::: mobile/android/modules/JavaAddonManager.jsm:12
(Diff revision 1)
> +Cu.import("resource://gre/modules/Messaging.jsm"); /*global Messaging */

Kill /*these useless comments*/

::: mobile/android/modules/JavaAddonManager.jsm:44
(Diff revision 1)
> +};

No semicolon after top-level functions.

::: mobile/android/modules/JavaAddonManager.jsm:66
(Diff revision 1)
> +  _prefix: function(aMessage) {

What is this, 1990?  :P

  _prefix: function (message) {

or be modern:

  _prefix(message) {

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions

::: mobile/android/modules/JavaAddonManager.jsm:41
(Diff revision 1)
> +  this._guid = options.guid;

Check options.guid for sanity.

::: mobile/android/tests/javaaddons/AndroidManifest.xml.in:8
(Diff revision 1)
> +    <uses-sdk android:minSdkVersion="8"

9?

::: mobile/android/tests/browser/junit3/src/TestJavaAddonManager.java:23
(Diff revision 1)
> +//    public void testJavaAddonManager() throws Exception {

Do you plan to uncomment these?

Comment 5

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/9879/#review9431

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8614305 [details]
MozReview Request: Bug 1168407 - Pre: Move roboextender under mobile/android/tests. r?gbrown

Bug 1168407 - Implement a bidirectional Java addon interface. r=kats,rnewman

There are several parts to this ticket:

1) Produce javaaddons-1.0.jar, a standalone JAR defining a (versioned)
Java interface suitable for consumption by third-party Java addon
implementations.

2) Support the new V1 interface in the JavaAddonManager.

3) Add JUnit 3 tests of the class-finding functionality.  This
includes building a test DEX file that implements the V1 interface
(consuming the built JAR).  This is both a functional test and living
documentation of how to produce such an addon.

4) Add Robocop JavascriptTests testing the JavaScript message passing
interface to and from Java.

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1168407 - Implement a bidirectional Java addon interface. f=jchen,r=rnewman r?mfinkle

Bug 1168407 - Review comment: add org.mozilla.gecko.javaaddons.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1168407 - Review comment: Guid -> GUID.

Bug 1168407 - Review comment: Guid -> GUID.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1168407 - Review comments.

Bug 1168407 - Review comments.

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1168407 - Review comment: Remove outdated test.

Bug 1168407 - Review comment: Remove outdated test.

I would like to have JUnit 3 tests but it's not easy to load a
classes.dex file in JUnit 3 tests.  I guess I could put a classes.dex
in robocop.apk/assets.  Future work.

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1168407 - Move everything into JavaAddonInterfaceV1. r=rnewman

Bug 1168407 - Move everything into JavaAddonInterfaceV1. r=rnewman

This patch is motivated by mfinkle's desire to not have version
information appear in consuming code.  This gets most of the way
there: consumers can import a single JavaAddonInterfaceV1 interface,
with internal sub-interfaces that are not versioned.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1168407 - Pre: Add GeckoJarReader.extractStream. r=rnewman

Bug 1168407 - Pre: Add GeckoJarReader.extractStream. r=rnewman

The use case is to ship a classes.dex file in a Gecko add-on.  This
makes it easy to extract such a file from an add-on to a temporary
location.  (Sadly, the Android Dex loading classes expect files, not
streams.)

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1168407 - Load classes.dex files from more general Gecko URIs. r=rnewman

Bug 1168407 - Load classes.dex files from more general Gecko URIs. r=rnewman

This makes it easy to ship a classes.dex file from within a Gecko
add-on.  We use this mechanism, including a classes.dex in the
Roboextender add-on, to test this code path.

Comment 14

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/9877/#review9631

::: mobile/android/base/EventDispatcher.java:161
(Diff revision 2)
>              if (listeners.size() == 0) {

isEmpty()

::: mobile/android/base/javaaddons/JavaAddonManager.java:85
(Diff revision 2)
> +            if (event.equals("Dex:Load")) {

String switch(), please.

::: mobile/android/base/javaaddons/JavaAddonManagerV1.java:39
(Diff revision 2)
> +    private final org.mozilla.gecko.EventDispatcher mDispatcher;

Move mDispatcher so it doesn't look like it's affected by the "Protected by..." comment.

::: mobile/android/base/BrowserApp.java:45
(Diff revision 2)
> +import org.mozilla.gecko.javaaddons.JavaAddonManager;

This doesn't seem to be used.

::: mobile/android/base/util/GeckoJarReader.java:116
(Diff revision 2)
> +        OutputStream output = new FileOutputStream(file);

You should do this *after* you've made the decision to read (i.e., line 140), and you should close it in a try..finally.

::: mobile/android/base/util/GeckoJarReader.java:144
(Diff revision 2)
> +                output.write(buf, 0, len);

There has gotta be a utility in the tree that copies between two streams, right?

::: mobile/android/base/util/GeckoJarReader.java:113
(Diff revision 2)
> +    public static File extractStream(Context context, String url, File dir) throws IOException {

/**
 * Extracts the provided streaming resource from our jar into a temporary file. The caller is responsible for deleting the file after use, or copying it to a stable location.
 */

What happens if this is called with varying invalid inputs?

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8614305 [details]
MozReview Request: Bug 1168407 - Pre: Move roboextender under mobile/android/tests. r?gbrown

Bug 1168407 - Pre: Move roboextender under mobile/android/tests. r?gbrown

I want to include a file built during the Fennec build to the
roboextender extension; this is an easy way to arrange it.

I took the opportunity to make the extension file tree mirror the XPI
file tree, which always used base/.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620459 [details]
MozReview Request: Bug 1168407 - Pre: Add GeckoJarReader.extractStream. r=rnewman

Bug 1168407 - Pre: Add GeckoJarReader.extractStream. r=rnewman

The use case is to ship a classes.dex file in a Gecko add-on.  This
makes it easy to extract such a file from an add-on to a temporary
location.  (Sadly, the Android Dex loading classes expect files, not
streams.)

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620454 [details]
MozReview Request: Bug 1168407 - Implement a bidirectional Java addon interface. f=jchen,r=rnewman r?mfinkle

Bug 1168407 - Implement a bidirectional Java addon interface. f=jchen,r=rnewman r?mfinkle

There are several parts to this ticket:

1) Produce javaaddons-1.0.jar, a standalone JAR defining a (versioned)
Java interface suitable for consumption by third-party Java addon
implementations.

2) Support the new V1 interface in the JavaAddonManager.

3) Add Robocop JavascriptTests testing the JavaScript message passing
interface to and from Java.

This patch can be read as "not in tests/" and "everything in tests/".

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620454 [details]
MozReview Request: Bug 1168407 - Implement a bidirectional Java addon interface. f=jchen,r=rnewman r?mfinkle

This incorporates rnewman's review.  Thanks, Richard!

Comment 19

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/9877/#review9807

> This doesn't seem to be used.

This was because JAM moved packages.

> String switch(), please.

This existed before me.  (I did the one I added.)

> /**
>  * Extracts the provided streaming resource from our jar into a temporary file. The caller is responsible for deleting the file after use, or copying it to a stable location.
>  */
> 
> What happens if this is called with varying invalid inputs?

Funny story -- https://bugzilla.mozilla.org/show_bug.cgi?id=1174922 happens :)

> There has gotta be a utility in the tree that copies between two streams, right?

Sadly no, and I'm not blocking this Q2 goal on that :)

Comment 20

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620459 [details]
MozReview Request: Bug 1168407 - Pre: Add GeckoJarReader.extractStream. r=rnewman

rnewman reviewed this already.

Comment 21

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49dcb0f35fc

Comment 22

[Geoff Brown [:gbrown]]

Comment on attachment 8614305 [details]
MozReview Request: Bug 1168407 - Pre: Move roboextender under mobile/android/tests. r?gbrown

https://reviewboard.mozilla.org/r/9879/#review9813

Ship It!

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #19)
> https://reviewboard.mozilla.org/r/9877/#review9807
> 
> > This doesn't seem to be used.
> 
> This was because JAM moved packages.
> 
> > String switch(), please.
> 
> This existed before me.  (I did the one I added.)

We avoid using switch() because it breaks when building GeckoView apps (or did).

Comment 24

[Richard Newman [:rnewman]]

We use source level 7 now, for quite a while, and elsewhere in the codebase. If we were going to break GV consumers, we would already have done so… but I expect most of them were using SL7 long before us.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #24)
> We use source level 7 now, for quite a while, and elsewhere in the codebase.
> If we were going to break GV consumers, we would already have done so… but I
> expect most of them were using SL7 long before us.

... and my memory is clearing: We had to avoid switch() with R.id.xxx stuff, not plain-old-strings.

Comment 26

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28d53f4b1d66

Comment 27

[Nick Alexander :nalexander [he/him]]

mfinkle: latest try build looks good.  testJavaAddons is passing in http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-d9ddeee1bb90/try-android-api-11/try_panda_android_test-robocop-7-bm102-tests1-panda-build1578.txt.gz.

Comment 28

[Nick Alexander :nalexander [he/him]]

I started working on documentation for this.  It's very much Not Finished, since it doesn't describe the Java side of the interface, and it is not hyper-linked: https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/JavaAddonManager.jsm

Comment 29

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #27)
> mfinkle: latest try build looks good.  testJavaAddons is passing in
> http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/nalexander@mozilla.
> com-d9ddeee1bb90/try-android-api-11/try_panda_android_test-robocop-7-bm102-
> tests1-panda-build1578.txt.gz.

For reasons I don't understand, I have consistent failures in rc3 on Android 9.  I'll hunt it down tomorrow.

Comment 30

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74da3c2f5888

Comment 31

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #29)
> (In reply to Nick Alexander :nalexander from comment #27)
> > mfinkle: latest try build looks good.  testJavaAddons is passing in
> > http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/nalexander@mozilla.
> > com-d9ddeee1bb90/try-android-api-11/try_panda_android_test-robocop-7-bm102-
> > tests1-panda-build1578.txt.gz.
> 
> For reasons I don't understand, I have consistent failures in rc3 on Android
> 9.  I'll hunt it down tomorrow.

This turned out to be simple: older Android versions don't support loading from .DEX directly, see [1] and [2].  They can still load from .APK and .JAR files.

It would be possible to work around this by producing a .JAR file at run time, and this is even potentially attractive, since we extract .DEX files and write them to temporary locations in the patch already.  However, in the interest of time, we're going to test against .APK files and instead document the restrictions for add-on consumers to be aware of.

[1] http://androidxref.com/4.4.4_r1/xref/libcore/dalvik/src/main/java/dalvik/system/DexPathList.java
[2] http://androidxref.com/2.3.6/xref/libcore/dalvik/src/main/java/dalvik/system/DexClassLoader.java

Comment 32

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #30)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=74da3c2f5888

gps: This try build appears to be busted due to config/recurse.mk not doing what I expect.  Specifically, if you look at the two relevant build logs [1] and [2], you can see that we try to copy javaaddons-test.apk into [e-mail] before it's built, see [3].

I have added a dependency in https://hg.mozilla.org/try/file/74da3c2f5888/config/recurse.mk#l152 like

mobile/android/tests/browser/robocop/roboextender/tools: mobile/android/tests/javaaddons/tools

I have a tools:: rule in https://hg.mozilla.org/try/file/74da3c2f5888/mobile/android/tests/javaaddons/Makefile.in#l14

I have a tools:: rule in https://hg.mozilla.org/try/file/74da3c2f5888/mobile/android/tests/browser/robocop/roboextender/Makefile.in#l22

[1] http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-08350b798b7d/try-android-api-9/t[y-android-api-9-bm76-try1-build245.txt.gz

[2] http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-08350b798b7d/try-android-api-11/try-android-api-11-bm78-try1-build451.txt.gz

[3] 11:56:43     INFO -  Verification succesful
11:56:43     INFO -  gmake[5]: Leaving directory `/builds/slave/try-and-api-9-0000000000000000/build/src/obj-firefox/mobile/android/tests/browser/junit3'
11:56:43     INFO -  gmake[5]: Entering directory `/builds/slave/try-and-api-9-0000000000000000/build/src/obj-firefox/mobile/android/tests/browser/robocop/roboextender'
11:56:43     INFO -  mkdir -p ../../../../../../_tests/testing/mochitest/extensions/[e-mail]/base
11:56:43     INFO -  cp /builds/slave/try-and-api-9-0000000000000000/build/src/mobile/android/tests/browser/robocop/roboextender/base/* ../../../../../../_tests/testing/mochitest/extensions/[e-mail]/base
11:56:43     INFO -  cp ../../../../../../mobile/android/tests/javaaddons/javaaddons-test.apk ../../../../../../_tests/testing/mochitest/extensions/[e-mail]/base
11:56:43     INFO -  cp: cannot stat `../../../../../../mobile/android/tests/javaaddons/javaaddons-test.apk': No such file or directory
11:56:43     INFO -  gmake[5]: [tools] Error 1 (ignored)
11:56:43     INFO -  gmake[5]: Leaving directory `/builds/slave/try-and-api-9-0000000000000000/build/src/obj-firefox/mobile/android/tests/browser/robocop/roboextender'
11:56:43     INFO -  gmake[5]: Entering directory `/builds/slave/try-and-api-9-0000000000000000/build/src/obj-firefox/mobile/android/tests/javaaddons'
11:56:43     INFO -  javaaddons-test.jar
11:56:43     INFO -  /builds/slave/try-and-api-9-0000000000000000/build/src/android-sdk-linux/build-tools/22.0.1/aapt package -f -M /builds/slave/try-and-api-9-0000000000000000/build/src/obj-firefox/mobile/android/tests/javaaddons/AndroidManifest.xml -I /builds/slave/try-and-api-9-0000000000000000/build/src/android-sdk-linux/platforms/android-22/android.jar -S /builds/slave/try-and-api-9-0000000000000000/build/src/mobile/android/tests/javaaddons/res  \
11:56:43     INFO -  	-J . \
11:56:43     INFO -  	-F javaaddons-test.ap_
11:56:43     INFO -  /builds/slave/try-and-api-9-0000000000000000/build/src/android-sdk-linux/build-tools/22.0.1/aapt: /lib/libz.so.1: no version information available (required by /builds/slave/try-and-api-9-0000000000000000/build/src/android-sdk-linux/build-tools/22.0.1/aapt)
11:56:43     INFO -  /usr/bin/javac -target 1.7 -source 1.7 -classpath /builds/slave/try-and-api-9-0000000000000000/build/src/android-sdk-linux/platforms/android-22/android.jar -bootclasspath /builds/slave/try-and-api-9-0000000000000000/build/src/android-sdk-linux/platforms/android-22/android.jar -encoding UTF8 -g:source,lines -Werror  -Xlint:all -d javaaddons-test-classes -classpath /builds/slave/try-and-api-9-0000000000000000/build/src/obj-firefox/mobile/android/javaaddons/javaaddons-1.0.jar /builds/slave/try-and-api-9-0000000000000000/build/src/mobile/android/tests/javaaddons/src/org/mozilla/javaaddons/test/ClassWithNoRecognizedConstructors.java /builds/slave/try-and-api-9-0000000000000000/build/src/mobile/android/tests/javaaddons/src/org/mozilla/javaaddons/test/JavaAddonV0.java /builds/slave/try-and-api-9-0000000000000000/build/src/mobile/android/tests/javaaddons/src/org/mozilla/javaaddons/test/JavaAddonV1.java
11:56:44     INFO -  /usr/bin/jar cMf javaaddons-test.jar -C javaaddons-test-classes .
11:56:44     INFO -  /usr/bin/javac -target 1.7 -source 1.7 -classpath /builds/slave/try-and-api-9-0000000000000000/build/src/android-sdk-linux/platforms/android-22/android.jar -bootclasspath /builds/slave/try-and-api-9-0000000000000000/build/src/android-sdk-linux/platforms/android-22/android.jar -encoding UTF8 -g:source,lines -Werror  -d classes R.java \
11:56:44     INFO -  	-classpath javaaddons-test.jar
11:56:44     INFO -  /builds/slave/try-and-api-9-0000000000000000/build/src/android-sdk-linux/build-tools/22.0.1/dx --dex --output=classes.dex classes javaaddons-test.jar
11:56:45     INFO -  cp javaaddons-test.ap_ javaaddons-test-unsigned-unaligned.apk
11:56:45     INFO -  /usr/bin/zip -0 javaaddons-test-unsigned-unaligned.apk classes.dex
11:56:45     INFO -    adding: classes.dex (stored 0%)
11:56:45     INFO -  cp javaaddons-test-unsigned-unaligned.apk javaaddons-test-unaligned.apk
11:56:45     INFO -  /builds/slave/try-and-api-9-0000000000000000/build/src/obj-firefox/_virtualenv/bin/python /builds/slave/try-and-api-9-0000000000000000/build/src/mobile/android/debug_sign_tool.py --keytool=/usr/bin/keytool --jarsigner=/usr/bin/jarsigner  javaaddons-test-unaligned.apk
11:56:46     INFO -  /builds/slave/try-and-api-9-0000000000000000/build/src/android-sdk-linux/build-tools/22.0.1/zipalign -f -v 4 javaaddons-test-unaligned.apk javaaddons-test.apk
11:56:46     INFO -  Verifying alignment of javaaddons-test.apk (4)...
11:56:46     INFO -        50 META-INF/MANIFEST.MF (OK - compressed)
11:56:46     INFO -       326 META-INF/ANDROIDD.SF (OK - compressed)
11:56:46     INFO -       669 META-INF/ANDROIDD.RSA (OK - compressed)
11:56:46     INFO -      1787 AndroidManifest.xml (OK - compressed)
11:56:46     INFO -      2264 resources.arsc (OK)
11:56:46     INFO -      2916 classes.dex (OK)
11:56:46     INFO -  Verification succesful

Comment 33

[Gregory Szorc [:gps]]

I would grep for "tools" in <objdir>/root-deps.mk and <objdir>/root.mk and verify that it has the directories you added to recurse.mk. The "recurse_tools" rule from root-deps.mk is what get executed when you do `make tools`. So it should be easy to debug this...

Watch out for "$(MAKE) -C <dir> tools" invocations in Makefile.in files: I think there are still a few.

Comment 34

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dab695aa2b88

Comment 35

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08a1c69cbf1

Comment 36

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d3ff2d0db2

Comment 37

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #36)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d3ff2d0db2

mfinkle: this try is very green.  (I used a hammer to make the build ordering work.)

There are a bunch of tiny patches to fold into the main one but the meat is in place.  Can you review (or delegate) the main patch?

Comment 38

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8620454 [details]
MozReview Request: Bug 1168407 - Implement a bidirectional Java addon interface. f=jchen,r=rnewman r?mfinkle

https://reviewboard.mozilla.org/r/10757/#review10169

::: mobile/android/base/javaaddons/JavaAddonManagerV1.java:109
(Diff revision 2)
> +                    dispatcher.unregisterAllEventListeners();

Do we need a sendSuccess or sendError call in the MESSAGE_UNLOAD handler?

Comment 39

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8620454 [details]
MozReview Request: Bug 1168407 - Implement a bidirectional Java addon interface. f=jchen,r=rnewman r?mfinkle

https://reviewboard.mozilla.org/r/10757/#review10171

Everything looks OK. Just check on the need for the success or error callback.

Comment 40

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/c30f0b5c6e50
https://hg.mozilla.org/integration/fx-team/rev/d99a590f2524
https://hg.mozilla.org/integration/fx-team/rev/60db000f3acb

Comment 41

[Nick Alexander :nalexander [he/him]]

Bombs away.  I added the callback call as mfinkle suggested.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c30f0b5c6e50
https://hg.mozilla.org/mozilla-central/rev/d99a590f2524
https://hg.mozilla.org/mozilla-central/rev/60db000f3acb

Comment 43

[Lawrence Mandel [:lmandel] (use needinfo)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Improvement to Java use in Fennec add-ons
[Suggested wording]: JavaAddonManager interface can now send, receive and respond to messages from Gecko
[Links (documentation, blog post, etc)]:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/JavaAddonManager.jsm