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

VERIFIED FIXED in Firefox 14

Status

()

Firefox for Android
General
--
critical
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: Scoobidiver (away), Assigned: kats)

Tracking

({crash, testcase, topcrash})

14 Branch
Firefox 16
ARM
Android
crash, testcase, topcrash
Points:
---

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed, firefox16 verified, blocking-fennec1.0 -, fennec+)

Details

(Whiteboard: [native-crash][gfx], crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
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
(Reporter)

Comment 2

5 years ago
(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.
Whiteboard: [native-crash] → [native-crash][gfx]
blocking-fennec1.0: --- → ?
Not enough crashes to justify blocking. We can adjust if stats go up.
blocking-fennec1.0: ? → -
(Reporter)

Comment 4

5 years ago
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?
(Reporter)

Comment 6

5 years ago
(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.
Assignee: nobody → bugmail.mozilla
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
(Reporter)

Updated

5 years ago
blocking-fennec1.0: - → ?
Keywords: testcase
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.
Depends on: 753845
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.
Attachment #622780 - Flags: review?(wjohnston)
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.
Attachment #622781 - Flags: review?(wjohnston)
blocking-fennec1.0: ? → soft
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.
Attachment #622780 - Flags: review?(wjohnston) → review-
Attachment #622781 - Flags: review?(wjohnston) → review-
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.
(Reporter)

Comment 18

5 years ago
It's #18 top crasher in 14.0b2.
No longer depends on: 753845
Keywords: topcrash
(Reporter)

Updated

5 years ago
Depends on: 753845
Duplicate of this bug: 756769
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.
Attachment #622780 - Attachment is obsolete: true
Attachment #622781 - Attachment is obsolete: true
Attachment #628427 - Flags: review?(wjohnston)
tracking-fennec: --- → +
blocking-fennec1.0: soft → -
removing top crash as this is no longer a top crash.
Keywords: topcrash
Comment on attachment 628427 [details] [diff] [review]
Patch

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

I'm good with this.
Attachment #628427 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa56496dcf3
Target Milestone: --- → Firefox 16
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/eaa56496dcf3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 25

5 years ago
It's #18 top crasher in 14.0b6.
(Reporter)

Comment 26

5 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(Reporter)

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
I just filed bug 765407 for this new crash.
(Reporter)

Comment 30

5 years ago
It's #12 top crasher in 14.0b7.
(Reporter)

Updated

5 years ago
blocking-fennec1.0: - → ?
Keywords: topcrash
Returing this to a minus. Nominated the bug Martijn filed.
blocking-fennec1.0: ? → -
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
Status: RESOLVED → VERIFIED
status-firefox16: fixed → verified
(Reporter)

Comment 34

5 years ago
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
Attachment #628427 - Flags: approval-mozilla-aurora?
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.
Attachment #628427 - Flags: approval-mozilla-beta?
(Reporter)

Comment 37

5 years ago
(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.
Attachment #628427 - Flags: approval-mozilla-beta?
Attachment #628427 - Flags: approval-mozilla-beta+
Attachment #628427 - Flags: approval-mozilla-aurora?
Attachment #628427 - Flags: approval-mozilla-aurora+
adding qawanted to check that this does what's intended once it lands and before we go to beta next Tuesday if possible.
Keywords: qawanted
https://hg.mozilla.org/releases/mozilla-aurora/rev/7315c99d681b
https://hg.mozilla.org/releases/mozilla-beta/rev/38146fa2a6f2
status-firefox14: affected → fixed
status-firefox15: affected → fixed

Updated

5 years ago
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.