959237 - Define GetJNIEnv and GetJNIForThread as infallible

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

GetJNIEnv and GetJNIForThread are fundamental operations that have no reason to fail. If they do fail, Fennec likely won't run correctly, and we probably have a bug (e.g. using JNI before initialization or after termination). Defining them as infallible means callers don't have to check for null being returned, and real bugs mentioned before get more visibility when we do crash because of unexpected failures.

There are some special cases with tests where we don't have a Java frontend (xpcshell tests, C++ tests, e10s tests), and these special cases can be whitelisted.

Comment 1

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

Attachment: Define GetVM, GetJNIEnv, and GetJNIForThread as infallible (v1)

Currently when either of these methods fail, we log something and rely on the calling code to null check. Since these failures are serious and likely unrecoverable, it's better to define these methods as infallible and just crash if they do fail (won't happen if things are working correctly).

Comment 2

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

Attachment: Add AndroidBridge::HasEnv checks (v1)

For certain tests, the AndroidBridge is not initialized, but some GeckoAppShell functions are still called indirectly. For now, this patch adds checks to skip these calls if there's no AndroidBridge.

Comment 3

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

Comment on attachment 8359311 [details] [diff] [review]
Define GetVM, GetJNIEnv, and GetJNIForThread as infallible (v1)

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

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1572,3 @@
>        env->IsSameObject(view, (jobject)sFullScreenInstance->mInstance->GetJavaSurface())) {
>      sFullScreenInstance->ExitFullScreen();
>    } 

fix this whitespace

::: gfx/thebes/nsSurfaceTexture.cpp
@@ +29,2 @@
>  
>    

fix this white space

Comment 4

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

Comment on attachment 8359312 [details] [diff] [review]
Add AndroidBridge::HasEnv checks (v1)

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

::: ipc/glue/MessagePump.cpp
@@ +105,5 @@
>      // This processes messages in the Android Looper. Note that we only
>      // get here if the normal Gecko event loop has been awoken above.
>      // Bug 750713
> +    if (MOZ_LIKELY(AndroidBridge::HasEnv())) {
> +        did_work |= GeckoAppShell::PumpMessageLoop();

put the HasEnv() check in PumpMessageLoop()

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1c7e26a97d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f69ca51a4e8


(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> Comment on attachment 8359312 [details] [diff] [review]
> Add AndroidBridge::HasEnv checks (v1)
> 
> Review of attachment 8359312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/glue/MessagePump.cpp
> @@ +105,5 @@
> >      // This processes messages in the Android Looper. Note that we only
> >      // get here if the normal Gecko event loop has been awoken above.
> >      // Bug 750713
> > +    if (MOZ_LIKELY(AndroidBridge::HasEnv())) {
> > +        did_work |= GeckoAppShell::PumpMessageLoop();
> 
> put the HasEnv() check in PumpMessageLoop()

PumpMessageLoop() is autogenerated so the check had to be outside of it.

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ae1c7e26a97d
https://hg.mozilla.org/mozilla-central/rev/3f69ca51a4e8