745250 - java.lang.NullPointerException: at android.view.GestureDetector.onTouchEvent(GestureDetector.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

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)]

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)]

(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?)]

Not enough crashes to justify blocking. We can adjust if stats go up.

Comment 4

[Scoobidiver (away)]

It was hit by 4 users.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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)]

(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:[e-mail])]

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]]

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]]

comment #8 was on my Android 4.0.4 (Galaxy Nexus)

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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 (dead)]

Attachment: 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:[e-mail])]

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:[e-mail])]

Attachment: (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:[e-mail])]

Attachment: (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)]

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:[e-mail])]

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 (dead)]

Attachment: 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)]

It's #18 top crasher in 14.0b2.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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)]

removing top crash as this is no longer a top crash.

Comment 22

[Wesley Johnston (:wesj)]

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:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa56496dcf3

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eaa56496dcf3

Comment 25

[Scoobidiver (away)]

It's #18 top crasher in 14.0b6.

Comment 26

[Scoobidiver (away)]

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

Comment 27

[Martijn Wargers (dead)]

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:[e-mail])]

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 (dead)]

I just filed bug 765407 for this new crash.

Comment 30

[Scoobidiver (away)]

It's #12 top crasher in 14.0b7.

Comment 31

[Kevin Brosnan [Ex-Mozilla]]

Returing this to a minus. Nominated the bug Martijn filed.

Comment 32

[Kartikaya Gupta (email:[e-mail])]

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)]

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)]

Aurora is still affected after the fix of bug 765407 landed: bp-ae0bfc1c-b75f-49c4-bdd4-59fb92120621.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

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:[e-mail])]

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)]

(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:[e-mail])]

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]

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]

adding qawanted to check that this does what's intended once it lands and before we go to beta next Tuesday if possible.

Comment 41

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7315c99d681b
https://hg.mozilla.org/releases/mozilla-beta/rev/38146fa2a6f2