Closed
Bug 774209
Opened 13 years ago
Closed 13 years ago
java.lang.NullPointerException: at android.widget.PopupWindow$PopupViewContainer.dispatchKeyEvent(PopupWindow.java) on Gingerbread
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox15+ affected, firefox16+ fixed, firefox17 fixed)
RESOLVED
FIXED
Firefox 18
People
(Reporter: scoobidiver, Assigned: Margaret)
References
Details
(Keywords: crash, regression, Whiteboard: [native-crash][startupcrash])
Crash Data
Attachments
(1 file)
2.81 KB,
patch
|
sriram
:
review+
eeejay
:
feedback+
MarcoZ
:
feedback+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•13 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
Comment 2•13 years ago
|
||
Sriram, does this crash look like fallout from your fix for bug 762727?
Reporter | ||
Comment 3•13 years ago
|
||
It's #2 top crasher in the first hours of 15.0.
tracking-firefox15:
--- → ?
Whiteboard: [native-crash] → [native-crash][startupcrash]
Comment 4•13 years ago
|
||
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.
Comment 5•13 years ago
|
||
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.
Comment 6•13 years ago
|
||
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 ??
Comment 7•13 years ago
|
||
(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.
Comment 8•13 years ago
|
||
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•13 years ago
|
tracking-firefox16:
--- → +
Comment 9•13 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
Comment 10•13 years ago
|
||
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
Comment 11•13 years ago
|
||
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•13 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?
Comment 13•13 years ago
|
||
(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•13 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•13 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
Comment 16•13 years ago
|
||
(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•13 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•13 years ago
|
||
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 19•13 years ago
|
||
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•13 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 21•13 years ago
|
||
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 22•13 years ago
|
||
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•13 years ago
|
||
Target Milestone: --- → Firefox 18
Assignee | ||
Comment 24•13 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 25•13 years ago
|
||
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•13 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•13 years ago
|
||
It's #19 top crasher in 15.0 and #82 in 16.0b1.
Keywords: topcrash
Comment 28•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 29•13 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.
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•