963242 - crash in @0x0 | JavaThreadDetachFunc

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

This bug was filed from the Socorro interface and is 
report bp-5f871cc4-9664-4b88-ae1c-b574d2140123.
=============================================================

Crash when enabling Flash.

STR: load http://www.homestarrunner.com/main15.html tap on characters button

Comment 1

[Aaron Train [:aaronmt]]

Reproducible on my HTC One (Android 4.4) after tapping the Flash 'Downloads' button in the above URL.

Comment 2

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

Attachment: Add null checks in AndroidBridge::JavaThreadDetachFunc()

I can't explain why the env or vm would be null here, but we should at least check for it.

Comment 3

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

Comment on attachment 8365056 [details] [diff] [review]
Add null checks in AndroidBridge::JavaThreadDetachFunc()

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

I feel like this might just be a symptom of some other underlying issue. According to the docs I found the destructor should never get called with a null argument...

Comment 4

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Comment on attachment 8365056 [details] [diff] [review]
> Add null checks in AndroidBridge::JavaThreadDetachFunc()
> 
> Review of attachment 8365056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel like this might just be a symptom of some other underlying issue.
> According to the docs I found the destructor should never get called with a
> null argument...

Yeah, maybe it's the VM that's null? Dunno. Definitely something fishy going on...

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/14a1340ab2ff

Comment 6

[Benjamin Kerensa [:bkerensa]]

Were going to track this since this a top crasher on nightly.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14a1340ab2ff

Comment 8

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

I think it's a regression from bug 959237.

Comment 9

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

Attachment: Only save JNI env in TLS when first attaching (v1)

This should do the trick. I think the crash is happening because we were saving all JNIEnv pointers in TLS, even if the JNIEnv did not come from AttachCurrentThread. When we end up calling DetachCurrentThread on these pointers in JavaThreadDetachFunc, Bad Things happen.

Comment 10

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

(In reply to Jim Chen [:jchen :nchen] from comment #9)
> Created attachment 8366728 [details] [diff] [review]
> Only save JNI env in TLS when first attaching (v1)
> 
> This should do the trick. I think the crash is happening because we were
> saving all JNIEnv pointers in TLS, even if the JNIEnv did not come from
> AttachCurrentThread. When we end up calling DetachCurrentThread on these
> pointers in JavaThreadDetachFunc, Bad Things happen.

Makes sense to me.

Comment 11

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

Comment on attachment 8366728 [details] [diff] [review]
Only save JNI env in TLS when first attaching (v1)

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

Sounds reasonable

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/45db690af8c6

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/45db690af8c6

Comment 14

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

Marking this fixed now, since Jim's change is more convincing.