Last Comment Bug 745250 - java.lang.NullPointerException: at android.view.GestureDetector.onTouchEvent(GestureDetector.java)
: java.lang.NullPointerException: at android.view.GestureDetector.onTouchEvent(...
Status: VERIFIED FIXED
[native-crash][gfx]
: crash, testcase, topcrash
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 14 Branch
: ARM Android
: -- critical (vote)
: Firefox 16
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
: 756769 (view as bug list)
Depends on: 753845
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-13 10:37 PDT by Scoobidiver (away)
Modified: 2016-07-29 14:24 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
verified
-
+


Attachments
testcase (773 bytes, text/html)
2012-05-10 01:24 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
(1/2) Assume mWaitForTouchListeners is always true (15.37 KB, patch)
2012-05-10 10:37 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review-
Details | Diff | Splinter Review
(2/2) Robustify (1.33 KB, patch)
2012-05-10 10:38 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review-
Details | Diff | Splinter Review
testcase2 (910 bytes, text/html)
2012-05-20 11:40 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Patch (5.85 KB, patch)
2012-05-30 12:45 PDT, Kartikaya Gupta (email:kats@mozilla.com)
wjohnston2000: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2012-04-13 10:37:25 PDT
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
Comment 1 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-04-13 13:25:41 PDT
So far only one URL listed:

http://arstechnica.com/open-source/news/2012/04/mozilla-may-make-flash-click-to-play-by-default-in-future-firefox.ars
Comment 2 Scoobidiver (away) 2012-04-13 13:34:02 PDT
(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.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-16 14:00:38 PDT
Not enough crashes to justify blocking. We can adjust if stats go up.
Comment 4 Scoobidiver (away) 2012-04-17 00:01:42 PDT
It was hit by 4 users.
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-17 05:46:07 PDT
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?
Comment 6 Scoobidiver (away) 2012-04-17 06:26:57 PDT
(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.
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-17 08:48:31 PDT
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.
Comment 8 Aaron Train [:aaronmt] 2012-04-29 09:52:13 PDT
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 9 Aaron Train [:aaronmt] 2012-04-29 09:52:56 PDT
comment #8 was on my Android 4.0.4 (Galaxy Nexus)
Comment 10 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-30 08:17:35 PDT
Most likely we are passing a null MotionEvent to GestureDetector.onTouchEvent. I'm not really sure why that would happen though.
Comment 11 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-10 01:24:27 PDT
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
Comment 12 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-10 09:41:38 PDT
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.
Comment 13 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-10 10:37:27 PDT
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.
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-10 10:38:30 PDT
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.
Comment 15 Wesley Johnston (:wesj) 2012-05-15 10:51:58 PDT
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 16 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-16 08:30:31 PDT
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.
Comment 17 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-20 11:40:15 PDT
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.
Comment 18 Scoobidiver (away) 2012-05-20 11:42:52 PDT
It's #18 top crasher in 14.0b2.
Comment 19 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-22 14:23:31 PDT
*** Bug 756769 has been marked as a duplicate of this bug. ***
Comment 20 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-30 12:45:48 PDT
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.
Comment 21 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-31 13:15:06 PDT
removing top crash as this is no longer a top crash.
Comment 22 Wesley Johnston (:wesj) 2012-06-06 10:47:21 PDT
Comment on attachment 628427 [details] [diff] [review]
Patch

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

I'm good with this.
Comment 23 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-06 10:51:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa56496dcf3
Comment 24 Ed Morley [:emorley] 2012-06-07 05:52:17 PDT
https://hg.mozilla.org/mozilla-central/rev/eaa56496dcf3
Comment 25 Scoobidiver (away) 2012-06-10 11:30:09 PDT
It's #18 top crasher in 14.0b6.
Comment 27 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-06-15 14:47:50 PDT
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.
Comment 28 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-15 15:13:51 PDT
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.
Comment 29 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-06-15 16:34:06 PDT
I just filed bug 765407 for this new crash.
Comment 30 Scoobidiver (away) 2012-06-16 05:24:26 PDT
It's #12 top crasher in 14.0b7.
Comment 31 Kevin Brosnan [:kbrosnan] 2012-06-17 22:19:19 PDT
Returing this to a minus. Nominated the bug Martijn filed.
Comment 32 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-18 06:54:35 PDT
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.
Comment 33 Cristian Nicolae (:xti) 2012-06-19 07:34:25 PDT
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
Comment 34 Scoobidiver (away) 2012-06-22 08:56:39 PDT
Aurora is still affected after the fix of bug 765407 landed: bp-ae0bfc1c-b75f-49c4-bdd4-59fb92120621.
Comment 35 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-22 10:32:13 PDT
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 36 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-22 10:34:05 PDT
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.
Comment 37 Scoobidiver (away) 2012-06-22 10:38:56 PDT
(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.
Comment 38 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-22 10:47:32 PDT
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 39 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-22 15:20:24 PDT
Comment on attachment 628427 [details] [diff] [review]
Patch

mobile only and low risk - go ahead and land to tip of beta.
Comment 40 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-22 15:21:12 PDT
adding qawanted to check that this does what's intended once it lands and before we go to beta next Tuesday if possible.

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