918309 - Import and use public domain JNI.jsm implementation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

It's more complete than modules/JNI.jsm, and it's also what our JNI-based add-ons already use. I think Wes was in favor of the switch, so let's do it!

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This is a direct port of the JNI.jsm file from cscott's github repo. Is this the public one you're talking about richard?

I intentionally avoided making ANY changes to it here (beyond a link to the original at the top). I'm still waffling a bit in my head about whether we should fork it ourselves, or use the github page (which hasn't been updated in awhile).

Comment 2

[Richard Newman [:rnewman]]

Yeah, that's the one. I checked the forks (mine, moz-services, etc.) and none of them have touched that file in `master`, so cscott's is still the latest.

We should probably clean it up (`let`, better OO, etc.) and send a pull req.

Comment 3

[Wesley Johnston (:wesj)]

Talked to mfinkle about this for awhile this afternoon. I managed to crash us a lot while playing with this for a bit, which makes me even more nervous for add-on authors. Its encouranging them to tap into API's that are not-even-close-to-frozen, and they'll crash as soon as they get a single signature wrong, often without any good error messages to help them diagnose it.

I think maybe the safer thing is to leave this out. Experienced/expert addon authors that want a really nice JNI.jsm api can include the file themselves and go crazy with excitement. Or maybe in some cases they'd be better writing some java in a dex file and using that API.

Comment 4

[Richard Newman [:rnewman]]

Counter-argument: we already have a bunch of add-ons that ship the full-fledged JNI.jsm (primarily for testing), and the barrier to shipping it is very low. So hoping to prevent add-ons from being crashy by not shipping it doesn't seem like a big deterrent to me.

On the other hand, we do care very much about memory consumption, and centralizing users of this library seems like a win there.

Furthermore, presumably these crashes will be eminently detectible during the add-on development and review process, so the risk of real-world crashes seems slim... and not shipping it doesn't do anything to prevent those crashes.

I appreciate the (very valid) concern about non-frozen APIs, though the release trains should at least mitigate that somewhat.

What am I missing? It seems that either we should whole-hog reject add-ons that use JNI (and thus remove even our own stub from the JS module system), or we should strongly consider shipping a better version, documenting the dangers, and putting a big red flag at the top of the docs so that developers don't cargo-cult without guidance and a supported path.

Does the .dex approach offer some kind of crash-avoidance? Is it as usable from JS as JNI.jsm?

Comment 5

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

Using JS/JNI internally has the benefit of not needing to support multiple versions of Firefox. An add-on needs to worry about shift-sands of UI and functions. JS/JNI crashes are quite poorly logcat'd and make it hard to debug, and that's if you even see the crash while you are testing on the one phone/tablet many add-on developers probably have access to.

The .dex approach is not entirely better in that you can't access parts of the Firefox UI and code from the .dex code. You can access Android system calls. Passing the results to JS means using the JSON messaging system. Crashes from this .dex code might show up nicer in logcat too since it's straight Java code, but I have no proof of that.

Comment 6

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

We might find this more useful if we start moving more of our code away from JSON messaging. I probably fall into the "danger is already all around us" camp and think we should have some form of JNI library. If this is a good one, we should do any tweaks we want for code cleanup/conventions and move ahead.

Does this library give us the ability to pass a JS function across JNI and have Java "call it"? I have my doubts but I am curious.

Comment 7

[Wesley Johnston (:wesj)]

A little trick to generate method signatures:

"javap -s -p GeckoApp" in objdir/mobile/android/base/gecko-browser-classes

I'm coming around to shipping the fancy version of this if we want. If someone wants to just dump it in and update our use of JNI.jsm, I'd be fine with us forking from that point.

Comment 8

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

Comment on attachment 808247 [details] [diff] [review]
Patch v1

Deferring review until we know if we really want this.

Comment 9

[Wesley Johnston (:wesj)]

Eric seemed interested in this. I don't anticipate this will be a huge task. We'll just have to dump this in and update all our current consumers to use it. Initially, I'd like to treat this as third party code and avoid making changes at all to it (beyond maybe an exports() line). We can update it to our style/fix other things in separate bugs if we decide too. I don't think the original is heavily maintained, so I don't see a strong reason to avoid changing it ourselves (unless we wanted to push our fixes upstream...).

I don't think we have any consumers of this code in add-ons, so I'm keep to just not worry about them (but we can make sure the dev docs people know this changed drastically).

Comment 10

[Eric Edens [:eedens]]

I did a build with this new  JNI.jsm; the local Robocop tests ran fine, but on the try server they failed. So it seems the task might be larger than we originally thought. I'll take another swing later, but for now I'll release it back to "nobody" in case someone wants to pick it up.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Eric Edens [:eedens] from comment #10)
> I did a build with this new  JNI.jsm; the local Robocop tests ran fine, but
> on the try server they failed. So it seems the task might be larger than we
> originally thought. I'll take another swing later, but for now I'll release
> it back to "nobody" in case someone wants to pick it up.

Try build URL, please.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: updated to tip

Here's an updated patch and a try run:

https://tbpl.mozilla.org/?tree=Try&rev=58c68de92de3

Comment 13

[Myk Melez [:myk] [@mykmelez]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #12)
> Here's an updated patch and a try run:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=58c68de92de3

Some tests succeeded, both robocop and mochitest, while others failed in various places, all because of a crash in libdvm.  I've triggered another try run with this morning's tip to see if anything has changed before I start digging deeper:

https://tbpl.mozilla.org/?tree=Try&rev=ac22c330d846

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: fix leak

This fixes what I think is a global reference leak, although unfortunately it doesn't seem to affect crashiness on tryserver:

https://tbpl.mozilla.org/?tree=Try&rev=5dcea41e7aa2

Comment 15

[Myk Melez [:myk] [@mykmelez]]

I can reproduce the crash locally by running robocop tests, and enabling CheckJNI causes these messages to show up in the log:

  2509               dalvikvm  E  JNI ERROR (app bug): attempt to use stale global reference 0x1f40057e
  2509               dalvikvm  W  JNI WARNING: jclass is an invalid global reference (0x1f40057e) (GetStaticMethodID)
  2509               dalvikvm  W  in Lorg/mozilla/gecko/mozglue/GeckoLoader;.nativeRun:(Ljava/lang/String;)V (GetStaticMethodID)


Instrumenting JNI.jsm shows that it loads and then unloads that global reference before the crash, and that the reference is to Lorg/mozilla/gecko/Tabs;:

  2509                  Gecko  I  JNILoadClass jcls: ctypes.voidptr_t(ctypes.UInt64("0x1f40057e")) Lorg/mozilla/gecko/Tabs;
  2509                  Gecko  I  JNILoadClass jcls: ctypes.voidptr_t(ctypes.UInt64("0x1d300596")) Ljava/lang/Object;
  2509                  Gecko  I  JNILoadClass jcls: ctypes.voidptr_t(ctypes.UInt64("0x1d30059a")) I
  2509                  Gecko  I  JNIUnloadClasses jcls: ctypes.voidptr_t(ctypes.UInt64("0x1d300596")) Ljava/lang/Object;
  2509                  Gecko  I  JNIUnloadClasses jcls: ctypes.voidptr_t(ctypes.UInt64("0x1f40057e")) Lorg/mozilla/gecko/Tabs;
  2509                  Gecko  I  JNIUnloadClasses jcls: ctypes.voidptr_t(ctypes.UInt64("0x1d30059a")) I


But my JNI fu is not yet sufficient to explain what this means.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: purge global refs to avoid reusing them and to GC referents

Ok, I think I figured it out.

JNI.LoadClass calls NewGlobalRef to create a global ref, after which it adds it to the registry.  Then, on the other end, JNI.UnloadClasses calls DeleteGlobalRef on all refs in the registry, but it never deletes them from the registry itself.  So the next time a consumer calls LoadClass on a previously-loaded class, the module reuses the class's existing, stale global ref from the registry.

Here's a fix: in UnloadClasses, purge the registry (and the 'classes' object, which also holds such refs) so the next consumer doesn't reuse the stale refs, and the referents (i.e. the reference objects themselves) can be garbage-collected.

(Perhaps ensureLoaded should also check that an existing ref is still valid, unsure.  Presumably such bugs are few and far between, although they sure are painful.)

With this fix, I no longer crash locally.  I'd send this to Try, but it's currently closed due to bug 1040308, so I'll try again in the morning.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

https://tbpl.mozilla.org/?tree=Try&rev=6bf1eb3a2e0e
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6bf1eb3a2e0e

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: wordsmith commentary

The try run looks good!

There's so much more I want to do here, both stylistic changes and substantive ones (and, of the latter, to both the API and the implementation).  But, as folks discussed earlier in the bug, it seems better to land this, now that it's working, and then iterate on it in the tree (as opposed to iterating here, in this bug, via successive patches).

This version is equivalent to the version I pushed to try, except that I wordsmithed my commentary to (hopefully) make it clearer, since I wrote that stuff at 4am-ish in the morning!

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 8468624 [details] [diff] [review]
patch v5: wordsmith commentary

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

Theres one other calls into JNI.jsm that you'll need to update:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/PaymentsUI.js#145

The other uses were for homescreen shortcuts for apps and seem to all be gone :)

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: also update PaymentsUI.js

(In reply to Wesley Johnston (:wesj) from comment #19)
> Theres one other calls into JNI.jsm that you'll need to update:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> PaymentsUI.js#145

Aha!  Ok, updated.  I read through docs and old bugs but didn't find any concrete info on how to test this.  Perhaps you can do so or point me at a test app?


> The other uses were for homescreen shortcuts for apps and seem to all be
> gone :)

Indeed!  But with this new implementation, we should have both the ability and the temptation to add more. :-)

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v7: test PaymentsUI JNI usage

(In reply to Myk Melez [:myk] [@mykmelez] from comment #20)
> I read through docs and old bugs but didn't find any
> concrete info on how to test this.  Perhaps you can do so or point me at a
> test app?

Hmm, we can at least test that JNI can be used to access the same GeckoNetworkManager.getMNC/getMCC methods that PaymentsUI accesses.  Here's a version of the patch that adds that to the testJNI robocop test.

Comment 22

[Wesley Johnston (:wesj)]

Comment on attachment 8470064 [details] [diff] [review]
patch v7: test PaymentsUI JNI usage

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

Its not easy to test. You should be able to install the dev marketplace addon:
https://addons.mozilla.org/en-US/developers/addon/dev-marketplace

then go to marketplace.allizom.com and look for a :paid addon to install. Its been awhile, so I'm not sure how well it works :) When writing I used the stuff at https://bugzilla.mozilla.org/show_bug.cgi?id=813756#c7 instead (i.e. I set the prefs manually, and then used the github test site).

Comment 23

[Myk Melez [:myk] [@mykmelez]]

(In reply to Wesley Johnston (:wesj) from comment #22)
> Its not easy to test. You should be able to install the dev marketplace
> addon:
> https://addons.mozilla.org/en-US/developers/addon/dev-marketplace
> 
> then go to marketplace.allizom.com and look for a :paid addon to install.
> Its been awhile, so I'm not sure how well it works :) When writing I used
> the stuff at https://bugzilla.mozilla.org/show_bug.cgi?id=813756#c7 instead
> (i.e. I set the prefs manually, and then used the github test site).

It worked!  I actually tried to do this last night on the production Marketplace site, but there aren't any :paid apps that are compatible with :mobile (and the only :free-inapp :mobile app, Ginger, wanted me to try it for 30 days before paying).

After installing the "Dev marketplace" addon, however, I was able to complete a payment on marketplace.allizom.org, and now I'm the proud owner of a license for "Test App (chipmunk8913)," which cost me $0.99 when I selected "make a real payment" on the "default network."

(I also tried to "simulate a payment", but that timed out.)

The logs also suggest that my mcc/mnc values are being correctly provided to the website, which is logging them:

  1776           GeckoConsole  E  [cli] mcc: 310
  1776           GeckoConsole  E  [cli] mnc: 260

While on my other test device, which doesn't have a SIM card in it, they are reported as unavailable:

  8643           GeckoConsole  E  [cli] mnc/mcc not available

So this works as expected.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/fx-team/rev/668e6ee2859a

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/668e6ee2859a

Comment 26

[Myk Melez [:myk] [@mykmelez]]

It'd be useful to document this for Fennec core and addon hackers, although the API is likely to change over time.