Closed Bug 1292323 Opened 3 years ago Closed 3 years ago

Add thread checking and dispatching attributes to WrapForJNI

Categories

(Core :: Widget: Android, defect, P3)

All
Android
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(9 files)

Right now, WrapForJNI has several attributes that are either unused or confusing. The goal is to clean up WrapForJNI and to add two attributes, calledFrom and dispatchTo. Specifying calledFrom allows checking for the correct calling thread. Specifying dispatchTo allows automatic dispatching of calls to appropriate threads.

For example, @WrapForJNI(calledFrom = "ui", dispatchTo = "gecko") specifies that the call must be made on the UI thread, and it should be automatically dispatched to the Gecko thread.
We need to register the Java UI thread in native code very early in the
startup process, before libxul registers its JNI entry points. So it
makes sense to register the Java UI thread in mozglue.
Attachment #8778327 - Flags: review?(snorp)
WrapForJNI has some flags that are obsolete or confusing. Clean it up so
that there are fewer but more meaningful flags. Add a "calledFrom" flag
to indicate the intended calling thread and add a "dispatchTo" flag to
indicate where a method call may be automatically dispatched.
Attachment #8778328 - Flags: review?(snorp)
Replace old flags in WrapForJNI usages with new flags. The calledFrom
and dispatchTo flags are set based on whether the method is native or
non-native, and how the method is used.
Attachment #8778330 - Flags: review?(snorp)
Update the code generator and related classes in annotation processor to
use the new WrapForJNI flags. Also add some more sanity checking to make
sure the flags are used correctly.
Attachment #8778331 - Flags: review?(snorp)
Implement checking the calling thread of a JNI call based on the
calledFrom attribute set in WrapForJNI. Also implement automatic call
dispatching based on the dispatchTo attribute set in WrapForJNI. This
eliminates the use of UsesNativeCallProxy and UsesGeckoThreadProxy.
Attachment #8778333 - Flags: review?(snorp)
Remove uses of UsesNativeCallProxy and UsesGeckoThreadProxy, now that
they are not needed.

Remove cases where we had to invoke a call in a proxy, because the call
is now specified to be invoked directly in the WrapForJNI annotation,
without the need to go through the proxy.

For SmsManager and AlarmReceiver, we no longer need to manually dispatch
everything to the Gecko thread because that's now handled automatically.
Attachment #8778339 - Flags: review?(snorp)
Attachment #8778327 - Flags: review?(snorp) → review+
Comment on attachment 8778328 [details] [diff] [review]
Clean up WrapForJNI and add calledFrom and dispatchTo attributes (v1)

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

r- just because I know you won't want to do the enum and I want to hear why :)

::: mobile/android/base/java/org/mozilla/gecko/annotation/WrapForJNI.java
@@ +35,2 @@
>       */
> +    String exceptionMode() default "abort";

This is *much* nicer, but can we use an enum instead of a string? It would end up looking like:

@WrapForJNI(exceptionMode = WrapForJNI.ExceptionMode.Ignore)

Or similar. Not as terse, but at least you don't get hosed by misspelling something, and we don't have to do silly string comparisons.

@@ +41,2 @@
>       */
> +    String calledFrom() default "any";

Same comment here re: enum

@@ +49,2 @@
>       */
> +    String dispatchTo() default "current";

And again
Attachment #8778328 - Flags: review?(snorp) → review-
Attachment #8778330 - Flags: review?(snorp) → review+
Comment on attachment 8778328 [details] [diff] [review]
Clean up WrapForJNI and add calledFrom and dispatchTo attributes (v1)

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

Jim and I talked during our 1:1 about this and using an enum is indeed a PITA
Attachment #8778328 - Flags: review- → review+
Attachment #8778331 - Flags: review?(snorp) → review+
Comment on attachment 8778333 [details] [diff] [review]
Implement JNI thread checking and dispatching (v1)

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

::: widget/android/jni/Accessors.h
@@ +38,5 @@
>  {
>      static void GetNsresult(JNIEnv* env, nsresult* rv)
>      {
>          if (env->ExceptionCheck()) {
> +#ifdef MOZ_CHECK_JNI

Is this a new one? Do we ever define this?
Attachment #8778333 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10)
> Comment on attachment 8778333 [details] [diff] [review]
> Implement JNI thread checking and dispatching (v1)
> 
> Review of attachment 8778333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/jni/Accessors.h
> @@ +38,5 @@
> >  {
> >      static void GetNsresult(JNIEnv* env, nsresult* rv)
> >      {
> >          if (env->ExceptionCheck()) {
> > +#ifdef MOZ_CHECK_JNI
> 
> Is this a new one? Do we ever define this?

Yeah we define it in jni/Utils.h
As a follow-up, merge all the NativeStubImpl specializations in jni/Natives.h
into one NativeStub, which makes it more organized and facilitates code
sharing.
Attachment #8780183 - Flags: review?(snorp)
As a second follow-up, refactor the dispatching code to be more streamlined,
and add two optimizations to the way we dispatch a call:

* avoid a pair of unnecessary calls to add/delete the global class ref,
  when dispatching a static call to the Gecko thread without a class ref
  parameter.

* avoid an extra allocation when dispatching to a proxy function.
Attachment #8780184 - Flags: review?(snorp)
Comment on attachment 8778339 [details] [diff] [review]
Update usage of UsesNativeCallProxy (v1)

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

Really nice!
Attachment #8778339 - Flags: review?(snorp) → review+
Attachment #8780183 - Flags: review?(snorp) → review+
Attachment #8780184 - Flags: review?(snorp) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9acf7d7e334
Move Java UI thread registration to mozglue; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/864ef38bcac5
Clean up WrapForJNI and add calledFrom and dispatchTo attributes; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/39207912bcd0
Update WrapForJNI usages; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/573afd555aa1
Update annotationProcessor to reflect WrapForJNI changes; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/95f4b1a92cdb
Update auto-generated bindings; r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e07a094e8c2
Implement JNI thread checking and dispatching; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/15a4865df1dc
Update usage of UsesNativeCallProxy; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f12c8a5ea71
Merge NativeStubImpl into NativeStub; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d41a61a39b3
Optimize native call dispatching; r=snorp
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d707c2b52b5
Follow-up to force clobber due to changes in generated code; r=me
You need to log in before you can comment on or make changes to this bug.