Closed Bug 963242 Opened 6 years ago Closed 6 years ago

crash in @0x0 | JavaThreadDetachFunc

Categories

(Firefox for Android :: General, defect, critical)

29 Branch
All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox28 --- unaffected
firefox29 + fixed

People

(Reporter: kbrosnan, Assigned: snorp)

References

()

Details

(4 keywords)

Crash Data

Attachments

(2 files)

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
Reproducible on my HTC One (Android 4.4) after tapping the Flash 'Downloads' button in the above URL.
Assignee: nobody → snorp
I can't explain why the env or vm would be null here, but we should at least check for it.
Attachment #8365056 - Flags: review?(bugmail.mozilla)
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...
Attachment #8365056 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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...
Whiteboard: [leave open]
Were going to track this since this a top crasher on nightly.
I think it's a regression from bug 959237.
Blocks: 959237
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.
Attachment #8366728 - Flags: review?(bugmail.mozilla)
(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 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
Attachment #8366728 - Flags: review?(bugmail.mozilla) → review+
Marking this fixed now, since Jim's change is more convincing.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
tracking-fennec: ? → ---
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.