Closed Bug 918309 Opened 8 years ago Closed 7 years ago

Import and use public domain JNI.jsm implementation

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: rnewman, Assigned: myk)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 6 obsolete files)

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!
Attached patch Patch v1 (obsolete) — Splinter Review
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).
Attachment #808247 - Flags: review?(mark.finkle)
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.
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.
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?
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.
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.
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 on attachment 808247 [details] [diff] [review]
Patch v1

Deferring review until we know if we really want this.
Attachment #808247 - Flags: review?(mark.finkle)
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).
Assignee: nobody → 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.
Assignee: eedens → nobody
(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.
Attached patch patch v2: updated to tip (obsolete) — Splinter Review
Here's an updated patch and a try run:

https://tbpl.mozilla.org/?tree=Try&rev=58c68de92de3
(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
Attached patch patch v3: fix leak (obsolete) — Splinter Review
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
Attachment #808247 - Attachment is obsolete: true
Attachment #8460959 - Attachment is obsolete: true
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.
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.
Assignee: nobody → myk
Attachment #8464653 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch patch v5: wordsmith commentary (obsolete) — Splinter Review
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!
Attachment #8468394 - Attachment is obsolete: true
Attachment #8468624 - Flags: review?(wjohnston)
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 :)
Attachment #8468624 - Flags: review?(wjohnston) → review-
(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. :-)
Attachment #8468624 - Attachment is obsolete: true
Attachment #8469848 - Flags: review?(wjohnston)
(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.
Attachment #8469848 - Attachment is obsolete: true
Attachment #8469848 - Flags: review?(wjohnston)
Attachment #8470064 - Flags: review?(wjohnston)
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).
Attachment #8470064 - Flags: review?(wjohnston) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/668e6ee2859a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
It'd be useful to document this for Fennec core and addon hackers, although the API is likely to change over time.
Keywords: dev-doc-needed
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.