Last Comment Bug 774209 - java.lang.NullPointerException: at android.widget.PopupWindow$PopupViewContainer.dispatchKeyEvent(PopupWindow.java) on Gingerbread
: java.lang.NullPointerException: at android.widget.PopupWindow$PopupViewContai...
Status: RESOLVED FIXED
[native-crash][startupcrash]
: crash, regression
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 15 Branch
: ARM Android
: -- critical (vote)
: Firefox 18
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on:
Blocks: 773085
  Show dependency treegraph
 
Reported: 2012-07-16 02:16 PDT by Scoobidiver (away)
Modified: 2012-09-08 09:16 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
affected
+
fixed
fixed


Attachments
workaround patch (2.81 KB, patch)
2012-09-05 14:23 PDT, :Margaret Leibovic
sriram.mozilla: review+
eitan: feedback+
mzehe: feedback+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2012-07-16 02:16:59 PDT
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
Comment 1 Scoobidiver (away) 2012-07-27 11:23:21 PDT
It's #35 top crasher in 15.0b1. The trunk is also affected.
Comment 2 Chris Peterson [:cpeterson] 2012-08-10 09:48:26 PDT
Sriram, does this crash look like fallout from your fix for bug 762727?
Comment 3 Scoobidiver (away) 2012-08-28 14:49:40 PDT
It's #2 top crasher in the first hours of 15.0.
Comment 4 Chris Peterson [:cpeterson] 2012-08-28 15:19:47 PDT
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 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-29 16:03:42 PDT
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 Sriram Ramasubramanian [:sriram] 2012-08-30 12:44:41 PDT
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 Chris Peterson [:cpeterson] 2012-08-30 13:33:16 PDT
(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 Chris Peterson [:cpeterson] 2012-08-30 13:33:24 PDT
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...
Comment 9 Alex Keybl [:akeybl] 2012-09-04 08:16:40 PDT
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?
Comment 10 Chris Peterson [:cpeterson] 2012-09-04 10:42:23 PDT
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.
Comment 11 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-04 11:44:50 PDT
Assigning to Margaret then for further investigation since we don't want to have a bug tracked for FF16 unassigned.
Comment 12 :Margaret Leibovic 2012-09-04 14:49:36 PDT
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 Chris Peterson [:cpeterson] 2012-09-04 15:55:48 PDT
(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.
Comment 14 Scoobidiver (away) 2012-09-05 00:33:59 PDT
(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
Comment 15 :Margaret Leibovic 2012-09-05 13:10:35 PDT
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?
Comment 16 Eitan Isaacson [:eeejay] 2012-09-05 13:12:51 PDT
(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.
Comment 17 :Margaret Leibovic 2012-09-05 13:36:22 PDT
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.
Comment 18 :Margaret Leibovic 2012-09-05 14:23:48 PDT
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.
Comment 19 Eitan Isaacson [:eeejay] 2012-09-05 14:37:55 PDT
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...
Comment 20 :Margaret Leibovic 2012-09-06 10:27:47 PDT
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.
Comment 21 Sriram Ramasubramanian [:sriram] 2012-09-06 10:29:20 PDT
Comment on attachment 658625 [details] [diff] [review]
workaround patch

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

The logic looks good to me.
Comment 22 Marco Zehe (:MarcoZ) 2012-09-07 07:21:33 PDT
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.
Comment 24 :Margaret Leibovic 2012-09-07 22:36:49 PDT
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
Comment 25 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-07 23:05:47 PDT
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
Comment 26 :Margaret Leibovic 2012-09-08 06:18:11 PDT
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.
Comment 27 Scoobidiver (away) 2012-09-08 06:47:58 PDT
It's #19 top crasher in 15.0 and #82 in 16.0b1.
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-09-08 08:33:42 PDT
https://hg.mozilla.org/mozilla-central/rev/ba75a6fd8ef5
Comment 29 Scoobidiver (away) 2012-09-08 09:16:57 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.