1292323 - Add thread checking and dispatching attributes to WrapForJNI

Status: RESOLVED FIXED

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

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Move Java UI thread registration to mozglue (v1)

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.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Clean up WrapForJNI and add calledFrom and dispatchTo attributes (v1)

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.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Update WrapForJNI usages (v1)

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.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Update annotationProcessor to reflect WrapForJNI changes (v1)

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.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Update auto-generated bindings (v1)

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Implement JNI thread checking and dispatching (v1)

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.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Update usage of UsesNativeCallProxy (v1)

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.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

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?

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to James Willcox (:snorp) ([e-mail]) 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

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Merge NativeStubImpl into NativeStub (v1)

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.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Optimize native call dispatching (v1)

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.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8778339 [details] [diff] [review]
Update usage of UsesNativeCallProxy (v1)

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

Really nice!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d707c2b52b5
Follow-up to force clobber due to changes in generated code; r=me

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d9acf7d7e334
https://hg.mozilla.org/mozilla-central/rev/864ef38bcac5
https://hg.mozilla.org/mozilla-central/rev/39207912bcd0
https://hg.mozilla.org/mozilla-central/rev/573afd555aa1
https://hg.mozilla.org/mozilla-central/rev/95f4b1a92cdb
https://hg.mozilla.org/mozilla-central/rev/3e07a094e8c2
https://hg.mozilla.org/mozilla-central/rev/15a4865df1dc
https://hg.mozilla.org/mozilla-central/rev/4f12c8a5ea71
https://hg.mozilla.org/mozilla-central/rev/1d41a61a39b3

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2d707c2b52b5