921041 - Replace the thread local storage impl of GetJNIForThread with JVM::GetEnv()

Status: RESOLVED WONTFIX

(Core Graveyard :: Widget: Android, defect)

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Don't use TLS, just keep things attached.

According to the Android docs at least JNI 1.6 is supported everywhere. Attaching as daemon should make sure we don't do any funny things on shutdown.

So the question is: won't this potentially be very wasteful if something makes throw-away threads that run JNI code?

(Spoiler: I know of at least one place in the code that does exactly that)

Comment 2

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

Comment on attachment 811214 [details] [diff] [review]
Patch 1. Don't use TLS, just keep things attached.

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

looks good to me, but want Kats to take a look too (he touched this code last I think)

wrt short lived threads like WebRTC, I think having something like the AutoJNIFrame in WebRTC that handles getting the JNIEnv and attaching on its own would be good.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

>wrt short lived threads like WebRTC, I think having something like the AutoJNIFrame in WebRTC that handles getting the JNIEnv and attaching on its own would be good.

That requires auditing all callpaths to make sure none of them are calling GetJNIForThread, though.

Maybe we should just nuke the entire thing from orbit, it's the only way to be sure.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 811214 [details] [diff] [review]
Patch 1. Don't use TLS, just keep things attached.

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

Looks ok to me as well. What is the motivation behind this change though? Performance?

::: widget/android/AndroidBridge.cpp
@@ +1600,5 @@
>                  __android_log_print(ANDROID_LOG_INFO, "GetJNIForThread",  "Could not attach");
>                  return NULL;
>              }
> +        } else if (status != JNI_OK) {
> +            __android_log_print(ANDROID_LOG_INFO, "GetJNIForThread",  "Could not get JNI Env");

It would be good to throw the error code into the message here.

Comment 5

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Looks ok to me as well. What is the motivation behind this change though?
> Performance?
Our shared WebRTC code assumes it can attach and detach a thread repeatedly. Without commenting as to if this is a good idea or not, our TLS code doesn't play well with that usage pattern, possibly increases shutdown time for no reason and essentially replicates functionality that the JVM already provides.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Note that the only thing this fix does is simplify this code a bit. Anything that ever uses GetJNIForThread will still be attached permanently.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Attachment: Gecko Event Sync lockup stack

It's crashing in mochitests during shutdown.
https://tbpl.mozilla.org/?tree=Try&rev=fae1194d528e

I tried to reproduce, got "Gecko event sync taking too long" spam. From looking in the debugger the attached stack seems to be the only thing relevant.

This is a bit scary because this patch is supposed to be mostly a no-op.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Current suspicion is that something is calling GetJNIForThread on the main thread (after it was already attached), and used to get the DetachThread in the destructor called as a side effect, but is no longer now.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

jchen points out that the compositor thread is a Thread that's attached to the JVM but that we actively destruct:
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=DeleteCompositorThread#l90

In the old code, the DetachCurrentThread call is made because the TLS destructor gets called on Thread destruction. In the new code, the thread actually gets killed without the JVM having an opportunity to react so this is why AttachCurrentThreadAsDaemon doesn't help us at all.

http://androidxref.com/4.3_r2.1/xref/dalvik/vm/Thread.cpp#1033

This warning can be seen in the mochitest logs.

Either we have to keep using the TLS trick to be able to Detach on/before thread destruction (and note the JVM does this as well to check if we do!), we have to explicitly detach threads doing JNI calls in their destructors, or we have to change the code so we don't keep threads attached forever (ala WebRTC AutoLocalJNIFrame with auto-attaching) - with nontrivial performance impact. 

Based on this, I think this is a WONTFIX. Removing TLS here is a nice cleanup but having to explicitly do an extra GetJNIForThread + DetachCurrentThread each time we kill a thread that *could have* accessed Java is even messier.