It first appeared in 14.0a1/20120411030716 and was hit by 3 users so far: bp-5912e95f-a2ce-4d65-8778-b6c2a2120412. java.lang.NullPointerException at android.view.GestureDetector.onTouchEvent(GestureDetector.java:457) at org.mozilla.gecko.gfx.TouchEventHandler.dispatchEvent(TouchEventHandler.java:217) at org.mozilla.gecko.gfx.TouchEventHandler.processEventBlock(TouchEventHandler.java:249) at org.mozilla.gecko.gfx.TouchEventHandler.access$200(TouchEventHandler.java:47) at org.mozilla.gecko.gfx.TouchEventHandler$ListenerTimeoutProcessor.run(TouchEventHandler.java:281) at android.os.Handler.handleCallback(Handler.java:605) at android.os.Handler.dispatchMessage(Handler.java:92) at android.os.Looper.loop(Looper.java:137) at android.app.ActivityThread.main(ActivityThread.java:4424) at java.lang.reflect.Method.invokeNative(Native Method) at java.lang.reflect.Method.invoke(Method.java:511) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:787) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:554) at dalvik.system.NativeStart.main(Native Method) More reports at: https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+at+android.view.GestureDetector.onTouchEvent%28GestureDetector.java%29
(In reply to Naoki Hirata :nhirata from comment #1) > So far only one URL listed: It probably means that plugins.click_to_play is set to true for those cases.
Not enough crashes to justify blocking. We can adjust if stats go up.
It was hit by 4 users.
Considering this NPE is happening inside android system code we probably can't fix it correctly. We could just catch/ignore it though. Is it happening only on a particular device/OS version?
(In reply to Kartikaya Gupta (:kats) from comment #5) > Is it happening only on a particular device/OS version? It seems to happen only on Android 2.3 (from 2.3.3 to 2.3.6). If it's an Android bug, it's odd it appeared first in 14.0a1/20120411. There's probably a regression somewhere.
Possibly bug 742019 then, I think that was the only change recently to anything that might result in this. Still, android code shouldn't be throwing the exception no matter what we do.
Just ran into this on Nightly and Aurora (04/29) E/GeckoAppShell( 2422): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main") E/GeckoAppShell( 2422): java.lang.NullPointerException E/GeckoAppShell( 2422): at android.view.GestureDetector.onTouchEvent(GestureDetector.java:457) E/GeckoAppShell( 2422): at org.mozilla.gecko.gfx.TouchEventHandler.dispatchEvent(TouchEventHandler.java:222) I was panning before the page was fully complete
comment #8 was on my Android 4.0.4 (Galaxy Nexus)
Most likely we are passing a null MotionEvent to GestureDetector.onTouchEvent. I'm not really sure why that would happen though.
Created attachment 622654 [details] testcase I can reproduce this crash with this testcase on the Samsung Galaxy Nexus. Steps to reproduce: - Tap on the button, which opens a new window - Tap on the window.close button - Immediately after that, start some fling actions, while the window is still open
I'm blown away that you were able to come up with a test case for this that reproduces the problem. Amazing. Turns out the following STR reproduce the crash: 1) Start on a page with a touch listener registered 2) Do some touch event 3) Switch to a page with no touch listener registered 4) Do some touch event However all of this has to happen within a 500ms window while also hitting a different race condition.
Created attachment 622780 [details] [diff] [review] (1/2) Assume mWaitForTouchListeners is always true The exact sequence of events from a code point of view that triggers this bug is as follows: 1) A block of events comes in on a page that has touch events 2) TouchEventHander schedules a mListenerTimeoutProcessor 3) The events are inserted into the mEventQueue 4) The events are sent to gecko touch listeners 5) gecko touch listeners respond, and the block of events is processed normally. After this, mProcessingBalance is -1 because there is still one mListenerTimeoutProcessor that hasn't fired yet 6) The tab closes 7) mWaitForTouchListeners gets set to false because the newly selected tab doesn't have touch events 8) A new block of events comes in 9) In TouchEventHandler, the code path that executes mProcessingBalance++ gets run because mWaitForTouchListeners and mHoldInQueue are false. The events are dispatched directly and not queued. At this point mProcessingBalance is 0. 10) The mListenerTimeoutProcessor scheduled in step 2 fires. Since mProcessingBalance is 0, it calls processEventBlock, which does a mEventQueue.poll() on an empty queue. 11) dispatchEvent(null) is called, which causes badness in the form of NPEs. There are a few problems here. The first problem I think is that in step 7, mWaitForTouchListeners gets set to false. As of bug 744518 we always have a touchstart listener registered in browser.js, so clearly mWaitForTouchListeners doesn't take into account chrome-code listeners. While this is actually not a problem right now, it might end up being a problem later. The second problem is that in step 9, the mProcessingBalance increment was meant to be an optimization for the cases where we're not waiting for touch listeners to respond. This "out-of-band" change of mProcessingBalance seems like a bad idea in retrospect because it allows edge cases like this bug to creep through. My solution that fixes both of these problems is to just take out mWaitForTouchListeners entirely. We don't need it, and if it were accurate it would always be true because of the touchstart listener in browser.js. Taking this out simplifies things and lets us remove a lot more code that is not needed anymore. That's what the attached patch does. Also note that I'm applying this patch on top of the patches from bug 749384 (pending review still) which should take care of any pan/zoom lag due to touch events. If those patches don't go in but this one does, then all pages will demonstrate the lag in stopping fling.
Created attachment 622781 [details] [diff] [review] (2/2) Robustify Just to guard against other bugs of this nature, since throwing an NPE on the UI thread crashes Fennec when we could just ignore it and carry on.
I'm... nervous about removing the mWaitForTouchListeners stuff. Its there to provide an optimization for pan/zoom gestures on pages with no touch listeners. Without it, I expect there is (at least 200ms?) lag between the gesture and the page moving? Is the alternative just to make sure we clear the event queue (and cancel any in progress touches) when we reset mWaitForTouchListeners? I'm fine with mWaitForTouchListeners not taking into account chrome listeners. Maybe we should rename it to better reflect that sometime.
Comment on attachment 622780 [details] [diff] [review] (1/2) Assume mWaitForTouchListeners is always true You're right, this does introduce a slight lag. I didn't notice it before but when doing a side-by-side test it shows up. I'll look at the patch again; when I wrote I thought about it and had decided it wouldn't introduce a lag but I was wrong.
Created attachment 625505 [details] testcase2 This is much easier to reproduce. Tap on the button, then double tap on the green box to get this crash.
It's #18 top crasher in 14.0b2.
Created attachment 628427 [details] [diff] [review] Patch Here's another attempt at fixing this. It turned out to not be so simple to just clean out all the state when we set mWaitForTouchListeners because there could be touch events and timeout processors in-flight already and then they would end up running on the wrong event blocks if I reset the state in between. The fundamental problem in the original code was that mProcessingBalance was getting modified out-of-sequence because of that hack I had in handleEvent, and this patch removes that and uses a replacement codepath that ensures all events get handled in the right order.
removing top crash as this is no longer a top crash.
Comment on attachment 628427 [details] [diff] [review] Patch Review of attachment 628427 [details] [diff] [review]: ----------------------------------------------------------------- I'm good with this.
It's #18 top crasher in 14.0b6.
It has been hit by 2 users since the patch landed: https://crash-stats.mozilla.com/report/list?version=FennecAndroid%3A16.0a1&signature=java.lang.NullPointerException%3A%20at%20android.view.GestureDetector.onTouchEvent%28GestureDetector.java%29
I was hitting this crash a couple of times while testing. Probably better to file a new bug for this. I'm not sure yet If I'll be able to get a simple testcase for this.
This crash seems to be different; best to file a new bug. This one is happening on this line in GestureDetector.java: handled |= mDoubleTapListener.onDoubleTapEvent(ev); probably because we set the double-tap listener to null in some cases. This crash seems to be a regression from bug 707571.
I just filed bug 765407 for this new crash.
It's #12 top crasher in 14.0b7.
Returing this to a minus. Nominated the bug Martijn filed.
The crashes in b7 are likely a combination of this bug and the other one, since the patches on this bug haven't been uplifted to 14 yet.
I cannot reproduce this crash on the latest Nightly, by following the steps from comment #11. Closing bug as verified fixed on: Firefox 16.0a1 (2012-06-19) Device: Galaxy Nexus OS: Android 4.0.2
Aurora is still affected after the fix of bug 765407 landed: bp-ae0bfc1c-b75f-49c4-bdd4-59fb92120621.
Comment on attachment 628427 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 742019 User impact if declined: crashes while double-tapping, in some cases Testing completed (on m-c, etc.): on m-c for a while now Risk to taking this patch (and alternatives if risky): mobile-only, low risk since it's been on m-c for a while String or UUID changes made by this patch: none
Comment on attachment 628427 [details] [diff] [review] Patch Also requesting beta approval for 14.01 since it seems to be #13 top crasher in 14.0b7, if I'm reading the crash stats right.
(In reply to Kartikaya Gupta (:kats) from comment #36) > Also requesting beta approval for 14.01 since it seems to be #13 top crasher > in 14.0b7, if I'm reading the crash stats right. We don't know which part is caused by bug 765407 and which one is caused by this bug.
True. Still, bug 765407 has been uplifted to beta and this probably should be as well, since it seems like a safe enough fix.
Comment on attachment 628427 [details] [diff] [review] Patch mobile only and low risk - go ahead and land to tip of beta.
adding qawanted to check that this does what's intended once it lands and before we go to beta next Tuesday if possible.