java.lang.NullPointerException: at android.widget.PopupWindow$PopupViewContainer.dispatchKeyEvent(PopupWindow.java) on Gingerbread

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
General
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Scoobidiver (away), Assigned: Margaret)

Tracking

({crash, regression})

15 Branch
Firefox 18
ARM
Android
crash, regression
Points:
---

Firefox Tracking Flags

(firefox15+ affected, firefox16+ fixed, firefox17 fixed)

Details

(Whiteboard: [native-crash][startupcrash], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
It first appeared in 15.0a2/20120714. The Aurora regression range is:
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=7140e39a6427&tochange=50963e16d1dc
It might be a regression from bug 764405 or bug 769001.

java.lang.NullPointerException
	at android.widget.PopupWindow$PopupViewContainer.dispatchKeyEvent(PopupWindow.java:1428)
	at android.view.ViewRoot.deliverKeyEventToViewHierarchy(ViewRoot.java:2560)
	at android.view.ViewRoot.deliverKeyEvent(ViewRoot.java:2525)
	at android.view.ViewRoot.handleMessage(ViewRoot.java:1873)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:130)
	at android.app.ActivityThread.main(ActivityThread.java:3683)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:507)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:839)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:597)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+at+android.widget.PopupWindow%24PopupViewContainer.dispatchKeyEvent%28PopupWindow.java%29
(Reporter)

Comment 1

5 years ago
It's #35 top crasher in 15.0b1. The trunk is also affected.
Summary: java.lang.NullPointerException: at android.widget.PopupWindow$PopupViewContainer.dispatchKeyEvent(PopupWindow.java) → java.lang.NullPointerException: at android.widget.PopupWindow$PopupViewContainer.dispatchKeyEvent(PopupWindow.java) on Gingerbread
Sriram, does this crash look like fallout from your fix for bug 762727?
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
(Reporter)

Comment 3

5 years ago
It's #2 top crasher in the first hours of 15.0.
tracking-firefox15: --- → ?
Whiteboard: [native-crash] → [native-crash][startupcrash]
Curiously, of the ~480 PopupWindow crashes from the last four weeks, ~475 are on Gingerbread and ~5 on Froyo. I don't see any ICS or JB crashes.
(Reporter)

Updated

5 years ago
Keywords: topcrash
Will track for 15 and watch for any user comments or increased volume, though at this time it doesn't appear to be a chemspill-worthy issue.
tracking-firefox15: ? → +
Is there any STR to reproduce this? I tried installing Firefox (release) on two gingerbread phones and neither of them crashed.

Hypothesis: Probably APK could get corrupted while downloading -- resulting in CPP crash.

Argument: If it's a startup crash on Gingerbread, then it should be a startup crash on all Gingerbread phones.

Pointers: mozglue/android/APKOpen.cpp:299 ??
          memory/mozalloc/mozalloc_abort.cpp:89 ??
(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> Argument: If it's a startup crash on Gingerbread, then it should be a
> startup crash on all Gingerbread phones.
> 
> Pointers: mozglue/android/APKOpen.cpp:299 ??
>           memory/mozalloc/mozalloc_abort.cpp:89 ??

This crash is an uncaught Java exception. mozalloc_abort() is in the C stack trace because that is how Java_org_mozilla_gecko_GeckoAppShell_reportJavaCrash() triggers the Breakpad crash reporting.
If the regression range in comment 0 is correct, could this crash be is a regression from Margaret's door hanger changes? Those are the only changesets that touched PopupWindow code.

This crash looks like a race condition when a key event is delivered to a PopupWindow that has already been unloaded. Of the 1100+ crash reports from the last 4 weeks, ~1080+ crashed in less than 1000 ms. But I have no idea why a key event would be dispatched so quickly after launch...

Updated

5 years ago
tracking-firefox16: --- → +

Comment 9

5 years ago
Chris - since you're leading the investigation so far, assigning this #6 top FF15 crasher to you. Are there any speculative fixes we could consider making on Aurora/Beta ahead of the next release?
Assignee: nobody → cpeterson
Sriram or Margaret would probably be the best people to investigate. I don't know the UI code very well or what caused this regression.
Assignee: cpeterson → nobody
Assigning to Margaret then for further investigation since we don't want to have a bug tracked for FF16 unassigned.
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 12

5 years ago
I searched the internet and found that other app developers have come across this crash, but it doesn't look like anyone has really figured out the root cause, other than recognizing that there's some Android bug here. Some answers mentioned that problems can happen if you give the PopupWindow focus before showing it, or setting a background on the popup before showing it, as problems can occur if the user hits the back button before the popup is actually shown. So maybe our problem has to do with doing things to the PopupWindow before we show it? The fact that there's such a high volume of these crashes, and that some happen very shortly after first launch makes me suspect the problem is with DoorHangerPopup, not SiteIdentityPopup.

Looking at the bugs in the regression range, I'm having a tough time figuring out what exactly we could have done wrong to introduce this issue, especially since a lot of the changes should only affect tablets, and we're seeing these crashes on Gingerbread/Froyo. I thought maybe making the change to include an offset (even if it's 0) in showAsDropDown could cause a problem, but looking at the android source code, showAsDropDown(v) just calls showAsDropDow(v, 0, 0). Are we certain that this regression range is correct?
(In reply to Margaret Leibovic [:margaret] from comment #12)
> Are we certain that this regression range is correct?

The earliest crash report I can find is Aurora 15.0a2 2012-07-14 (like Scoobidriver saw in comment 1): 5e956335-4acf-4797-80f7-a004a2120715

I don't know if comment 1's changeset regression range is correct. You would need to double-check the about:buildconfig revisions for Aurora builds 7-13 and 7-14.
(Reporter)

Comment 14

5 years ago
(In reply to Margaret Leibovic [:margaret] from comment #12)
> Are we certain that this regression range is correct?
My bad. I was wrong. It is shifted by one day. The right one is:
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=88382a387a66&tochange=7140e39a6427
(Assignee)

Comment 15

5 years ago
Aha, I bet it was caused by http://hg.mozilla.org/releases/mozilla-aurora/rev/caca38771162.

Eitan, can we delay calling setFocusable(true) until after we show the popup?
Blocks: 773085
(In reply to Margaret Leibovic [:margaret] from comment #15)
> Aha, I bet it was caused by
> http://hg.mozilla.org/releases/mozilla-aurora/rev/caca38771162.
> 
> Eitan, can we delay calling setFocusable(true) until after we show the popup?

I'm pretty sure, yes.
(Assignee)

Comment 17

5 years ago
Hm, thinking about this more, it appears the problem is that popup is getting focus before it's showing, which means that the fact that we're calling setFocusable(true) at all is giving us grief. Maybe we need to do some hack where we call setFocusable(true) after we show the popup then setFocusable(false) when we hide it.
(Assignee)

Comment 18

5 years ago
Created attachment 658625 [details] [diff] [review]
workaround patch

Eitan, does this maintain the functionality you added in bug 773085?

I'm not certain that this would fix the crash, but it feels like a low-risk approach we could land to see if it works.
Attachment #658625 - Flags: feedback?(eitan)
Comment on attachment 658625 [details] [diff] [review]
workaround patch

Yeah, looks OK. I can't test it right now. If you had a device with a keyboard or bluetooth keyboard you could. It is a strange fix, but if it does the trick...
Attachment #658625 - Flags: feedback?(eitan) → feedback+
(Assignee)

Comment 20

5 years ago
Comment on attachment 658625 [details] [diff] [review]
workaround patch

(In reply to Eitan Isaacson [:eeejay] from comment #19)

> Yeah, looks OK. I can't test it right now. If you had a device with a
> keyboard or bluetooth keyboard you could. It is a strange fix, but if it
> does the trick...

I don't have a device with a keyboard or bluetooth keyboard, and I'm traveling. Maybe Marco can test to confirm this doesn't break accessibility. I know this is a strange fix, but we just need to do something to work around this Android bug on older versions of Android.

I'm asking Sriram to review, and I can try landing this to see if it actually fixes the crash. Fingers crossed.
Attachment #658625 - Flags: review?(sriram)
Attachment #658625 - Flags: feedback?(marco.zehe)
Comment on attachment 658625 [details] [diff] [review]
workaround patch

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

The logic looks good to me.
Attachment #658625 - Flags: review?(sriram) → review+
Comment on attachment 658625 [details] [diff] [review]
workaround patch

This works fine, and alerts such as the one asking whether one wants to install and open the Market Place are still accessible via the keyboard. f=me.
Attachment #658625 - Flags: feedback?(marco.zehe) → feedback+
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba75a6fd8ef5
Target Milestone: --- → Firefox 18
(Assignee)

Comment 24

5 years ago
Comment on attachment 658625 [details] [diff] [review]
workaround patch

This is a speculative crash fix, but it's low-risk enough that I think we can uplift it sooner rather than later to see if it works. I only see one crash on Nightly in the last week, so we may need to uplift to see if it actually works.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 773085
User impact if declined: crashes when doorhangers appear
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): low-risk, we verified this doesn't break the accessibility change from bug 773085
String or UUID changes made by this patch: n/a
Attachment #658625 - Flags: approval-mozilla-beta?
Attachment #658625 - Flags: approval-mozilla-aurora?
Comment on attachment 658625 [details] [diff] [review]
workaround patch

early enough that we can back this out if we have to, but it would be good to get more data via uplifting, approving
Attachment #658625 - Flags: approval-mozilla-beta?
Attachment #658625 - Flags: approval-mozilla-beta+
Attachment #658625 - Flags: approval-mozilla-aurora?
Attachment #658625 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 26

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/19e15ec05b04
https://hg.mozilla.org/releases/mozilla-beta/rev/512c15654507

I also just realized that I should add a setFocusable(true) call after we call showAtLocation in the no anchor case (the way we show these doorhangers for web apps). I'll file a follow-up for that.

I'm leaving the status flags as affected for now until we learn if this has worked.
(Reporter)

Comment 27

5 years ago
It's #19 top crasher in 15.0 and #82 in 16.0b1.
Keywords: topcrash
https://hg.mozilla.org/mozilla-central/rev/ba75a6fd8ef5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 29

5 years ago
(In reply to Margaret Leibovic [:margaret] from comment #26)
> I'm leaving the status flags as affected for now until we learn if this has
> worked.
The verified status is for that.
status-firefox16: affected → fixed
status-firefox17: affected → fixed
You need to log in before you can comment on or make changes to this bug.