839836 - add an alternative to FindClass usable from other threads without MOZILLA_INTERNAL_API (e.g. WebRTC code)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

For reasons that are not entirely clear, using FindClass from Java code invoked via the JNI on other threads doesn't work reliably.  There are some things on the net that claim to explain what's going on here, but they don't seem to really get at the heart of the issue.  <https://bugzilla.mozilla.org/show_bug.cgi?id=835973#c5> has a bit more info here.

So, an alternative approach is to add a method to AndroidJNIWrappers that calls over to the main thread to find the class and returns a globalized jclass reference.  Patch forthcoming.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

That should read "using FindClass from C++ code"

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: WIP GetGlobalClassRef, v1 (landed on alder)

This seems to work pretty well.  It will need some cleanup before it can be uplifted to the trunk, however, most of which is documented in the patch.  

One thing that's not, however, is that it introduces a dependency on media/mtransport into widget/android so that it can use runnable_utils.h to alleviate the pain that is NS_Runnable.

I'll argue that the Right Thing to do would be to move runnable_utils into toolkit or xpcom or xpcom_glue or some other place broadly usable by all Gecko code, because there's nothing mtransport-specific about it.  That said, I don't have a sense of if/how/when that might actually happen, so it may be necessary to do the NS_Runnable glop by hand before uplift.  Unless introducing this dependency is actually a reasonable thing to.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Landed on alder:

https://hg.mozilla.org/projects/alder/rev/5c62e9de6fa0

Next steps:

* rebase to trunk
* cleanup
* request review

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Off-main-thread FindClass

1) Changed the error handling to return NULL on errors, conform the JNI.
2) Removed the dependency on media/mtransport.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 726225 [details] [diff] [review]
Patch 1. Off-main-thread FindClass

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

Overall this looks great.  Thanks for the cleanup, particularly getting rid of the transport dependency. The stuff I've pointed out is pretty much just nits.  There is one non-nit, however: this wants some unit-testing, particularly since Mozilla threading code changes from time to time, and threading interaction changes might well break some JNI-related assumptions.

::: widget/android/AndroidJNIWrapper.cpp
@@ +27,5 @@
> +  public:
> +    GetGlobalClassRefRunnable(const char *className, jclass *foundClass) :
> +        mClassName(className), mResult(foundClass) {}
> +    NS_IMETHOD Run() {
> +        *mResult = __jsjni_GetGlobalClassRef(mClassName);

Is there any chance this wants to be NS_IMETHODIMP?  I don't actually know, because I've almost always seen the declaration and the implementation separated, and thus NS_IMETHOD for the decl and NS_IMETHODIMP for the implementation.

@@ +51,5 @@
> +  jclass
> +  __jsjni_GetGlobalClassRef(const char *className) {
> +
> +    jclass foundClass = jsjni_FindClass(className);
> +    if (!foundClass) return NULL;

Putting the return on the same line as the condition makes debugging harder, because it's no longer possible to set a breakpoint that only triggers when the call fails.

@@ +56,5 @@
> +
> +    // root class globally
> +    JNIEnv *env = mozilla::AndroidBridge::GetJNIEnv();
> +    jclass globalRef = static_cast<jclass>(env->NewGlobalRef(foundClass));
> +    if (!globalRef) return NULL;

Same issue as previous comment.

@@ +61,5 @@
> +
> +    // return the newly create global reference
> +    return globalRef;
> +  }
> +

Since this doesn't require external/default visibility, I wonder if there's any reason to give it extern "C" linkage.  On the other hand, assuming it works now, it's probably not worth spending more time on than just asking somebody who knows more about this stuff than I do (eg bsmedberg).  This wouldn't effect getting an r+ on the bug.

::: widget/android/AndroidJNIWrapper.h
@@ +15,5 @@
> +// invoked via the JNI that doesn't have MOZILLA_INTERNAL_API defined.
> +// The caller is responsible for ensuring that the class is not leaked by
> +// calling DeleteGlobalRef at an appropriate time.
> +extern "C" jclass jsjni_GetGlobalClassRef(const char *className);
> +

This probably wants to be doxygen style commentary so that it gets automagically parsed by (eg) dxr.mozilla.org.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

>Is there any chance this wants to be NS_IMETHODIMP?

No. NS_IMETHOD is basically "virtual" (with extra magic on Windows) and NS_IMETHODIMP is "". I checked on #developers and bz confirmed that NS_IMETHOD = virtual NS_IMETHODIMP. So the code is correct. Note that the code structure was lifted from the mtransport/runnable magic it replaces.

>Putting the return on the same line as the condition makes debugging harder, 
>because it's no longer possible to set a breakpoint that only triggers when the 
>call fails.

I was keeping the code style consistent with the rest of the file, but I can fix this as I do agree here.

>Since this doesn't require external/default visibility, I wonder if there's any 
>reason to give it extern "C" linkage.

The vote is 3-1 on this one in favor of extern being required to prevent name mangling from mismatching the functions, so let's keep it as-is :-)

>There is one non-nit, however: this wants some unit-testing, particularly since 
>Mozilla threading code changes from time to time, and threading interaction 
>changes might well break some JNI-related assumptions.

Given how this code is called, and the need for the Java stack to be up, this will almost certainly require a mochitest, using JS to activate the C++ code which then calls the JNI functions, almost doing the same thing (or entirely the same) as the existing WebRTC tests that we still have got to run. Or did I miss something there and is there an easier way to test this codepath?

Would you consider r+-ing this if we create a separate bug for tests for this (which might be entirely covered by existing-but-disabled tests!)? Note that FindClass already does a hard check right now if it's being called on the right thread.

I would prefer to land the code that enables video now so people can start testing on m-c, as we already understand that none of the WebRTC tests is working and that's the next thing to do after that, unless we want to be bisecting regressions every week.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #6)

> I was keeping the code style consistent with the rest of the file, but I can
> fix this as I do agree here.

At some level, I'm not a peer for these files, so you're still going to either need one of the peers to delegate their review to me, or ask them to look at it.  But I support breaking the local style here. 

> The vote is 3-1 on this one in favor of extern being required to prevent
> name mangling from mismatching the functions, so let's keep it as-is :-)

Sold.

> >There is one non-nit, however: this wants some unit-testing, particularly since 
> >Mozilla threading code changes from time to time, and threading interaction 
> >changes might well break some JNI-related assumptions.
> 
> Given how this code is called, and the need for the Java stack to be up,
> this will almost certainly require a mochitest, using JS to activate the C++
> code which then calls the JNI functions, almost doing the same thing (or
> entirely the same) as the existing WebRTC tests that we still have got to
> run. Or did I miss something there and is there an easier way to test this
> codepath?

Is there some reason a regular old C++ unit test wouldn't work?  

<https://developer.mozilla.org/en-US/docs/Compiled-code_automated_tests>

Of course, the automation doesn't yet run those tests.  :-(

> Would you consider r+-ing this if we create a separate bug for tests for
> this (which might be entirely covered by existing-but-disabled tests!)? Note
> that FindClass already does a hard check right now if it's being called on
> the right thread.
> 
> I would prefer to land the code that enables video now so people can start
> testing on m-c, as we already understand that none of the WebRTC tests is
> working and that's the next thing to do after that, unless we want to be
> bisecting regressions every week.

Yeah, I could do that.

So r=dmose, conditional on the filing that bug.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

>Is there some reason a regular old C++ unit test wouldn't work? 

Those don't load the Java environment that the JNI FindClass has to run in, do they?

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

It wouldn't be exactly the same environment, but it might be close enough.

Alternately, there are a bunch of Java tests in mobile/android/base/tests.  It might not be too hard to write a Java file that used JNI to invoke the C++ file in a similar environment.

But I now understand better your point that mochitests might really be the shortest path here. Though you might be able to get away with an xpcshell test.  Either way, I presume js-ctypes would be the way to call this stuff from JS?

Comment 10

[Gian-Carlo Pascutto [:gcp]]

>Alternately, there are a bunch of Java tests in mobile/android/base/tests.  It 
>might not be too hard to write a Java file that used JNI to invoke the C++ file in 
>a similar environment.

Provided we add a bunch of extra this-specific-test-only JNI calls and all the associated scaffolding, yes. You're proposing a bunch of extra JNI calls, so we can run a test going Java->(new JNI layer)->C++->Java. This seems very roundabout compared to the alternatives.

>But I now understand better your point that mochitests might really be the 
>shortest path here. Though you might be able to get away with an xpcshell test.

Yes, if those run with the full Android environment, instead of just Gecko. (I don't know if that is the case).

>Either way, I presume js-ctypes would be the way to call this stuff from JS?

I'd fire up a WebRTC camera test to be honest

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. v2 Off-main-thread FindClass

Moving review to blassey who's a widget/android peer, addressed review comments.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #10)
> >Alternately, there are a bunch of Java tests in mobile/android/base/tests.  It 
> >might not be too hard to write a Java file that used JNI to invoke the C++ file in 
> >a similar environment.
> 
> Provided we add a bunch of extra this-specific-test-only JNI calls and all
> the associated scaffolding, yes. You're proposing a bunch of extra JNI
> calls, so we can run a test going Java->(new JNI layer)->C++->Java. This
> seems very roundabout compared to the alternatives.
> 
> >But I now understand better your point that mochitests might really be the 
> >shortest path here. Though you might be able to get away with an xpcshell test.

xpcshell tests don't have a java environment to call into. Mochitests would probably be the best mechanism, but I agree with gcp here that the WebRTC tests themselves give us enough coverage.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 729060 [details] [diff] [review]
Patch 1. v2 Off-main-thread FindClass

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

Two things to note here. First, if this is used improperly it could leak global refs and exceed our limit. Second, due to the sync dispatch, there it could deadlock (again, if used improperly).

Hopefully the callers won't use it improperly. But if we start to see problems, we can solve the first problem with a ref counted wrapper for the global reference. The second may be more tricky.

::: widget/android/AndroidJNIWrapper.cpp
@@ +51,5 @@
> +  jclass
> +  __jsjni_GetGlobalClassRef(const char *className) {
> +
> +    jclass foundClass = jsjni_FindClass(className);
> +    if (!foundClass) return NULL;

I would rather you use 'env->FindClass(className);' directly (after retrieving the env 2 lines below obviously)

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Brad Lassey [:blassey] from comment #12)
> 
> xpcshell tests don't have a java environment to call into. Mochitests would
> probably be the best mechanism, but I agree with gcp here that the WebRTC
> tests themselves give us enough coverage.

That does seem like the path of less resistance, however I believe that in order for it to be true, one or both of two things need to happen:

1a) We need to convince ourselves that the C++ WebRTC unit tests actually cause the Java code to execute.

1b) The work gbrown did a while back to enable running C++ unit test via make needs to be glued into the build automation.  I know there are open bugs for this, but I can't seem to find them.  CCing Geoff, as I bet he can.

2) The WebRTC mochitests that whimboo and/or jsmith wrote need to be tweaked so that they actually run on Android, rather than being skipped.  I suspect this is as simple as tweaking a flag in the JS and making them somehow key off --enable-webrtc instead of having an artifical "desktop" JS variable.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Dan Mosedale (:dmose) from comment #14)
> 2) The WebRTC mochitests that whimboo and/or jsmith wrote need to be tweaked
> so that they actually run on Android, rather than being skipped.  I suspect
> this is as simple as tweaking a flag in the JS and making them somehow key
> off --enable-webrtc instead of having an artifical "desktop" JS variable.

FWIW, changing a mochitest in the dom/media/tests/mochitest to run on all platforms requires setting a runTest second parameter to false. In the case of what we're probably getting at - Desktop + Android, we'll need to add a small patch that allows configuring runTest to run on Desktop & Android, but not B2G.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Jason Smith [:jsmith] from comment #15)
> 
> FWIW, changing a mochitest in the dom/media/tests/mochitest to run on all
> platforms requires setting a runTest second parameter to false. In the case
> of what we're probably getting at - Desktop + Android, we'll need to add a
> small patch that allows configuring runTest to run on Desktop & Android, but
> not B2G.

I guess it depends whether the intent is to land on Android preffed off first.  If so, that's the case where keying off of the enabled pref may be the right choice. Of course it also depends on the the logistics of the B2G landing.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Dan Mosedale (:dmose) from comment #16)
> I guess it depends whether the intent is to land on Android preffed off
> first.  If so, that's the case where keying off of the enabled pref may be
> the right choice. Of course it also depends on the the logistics of the B2G
> landing.

For the initial WebRTC landing we also run the tests while the preference was turned off. I assume you want to run the tests once the code has been landed and we support WebRTC on Android? In that case your approach will not work. As Jason mentioned before we will have to tweak the parameter to runTest() and it's internal logic to determine the platform runnable state.

Comment 18

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd1785e07a0

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5dd1785e07a0