1130052 - Stumbler crash NPE UploadAlarmReceiver.java:56

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Geolocation, defect)

Description

[:garvan]

see also bug

I see an instance of this crash present on Beta (36). From the Play Store console:

java.lang.NullPointerException
at org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver$UploadAlarmService.onHandleIntent(UploadAlarmReceiver.java:56)
at android.app.IntentService$ServiceHandler.handleMessage(IntentService.java:65)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loop(Looper.java:212)
at android.os.HandlerThread.run(HandlerThread.java:61)

Comment 1

[:garvan]

(hit enter on the autocomplete, hence the empty request. go bugzilla go.)

Is there any info on the crash frequency, number of times this has happened?

To make things difficult, the crash report is not at a line that is NPE-crashable:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/UploadAlarmReceiver.java#56

Comment 2

[:garvan]

Victor:
I am going to go with returning from onHandleIntent(Intent) if the intent is null:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/UploadAlarmReceiver.java#52

The normal case is that this service has been started from stumbler service to perform uploads. The null intent indicates the service has been started by other means. In the prev. patch I thought I would handle that case for completenes, but in actuality, we don't care about this case. The upload alarm will get retriggered by the stumbler service at some future point.

Comment 3

[:garvan]

Attachment: If null intent, return from onHandleIntent

Currently, the is the best defence I can think of against this crash, to only handle the service starting with a non-null intent. There is no downside to this -other than I can't conclusively say this will fix the crash.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

It hasn't happened a lot, only 6 times this week on Beta. I see the same stack on release -- 20 reports this week.

I agree that the stack seems weird. I wonder if the NPE came from within upload(). I see there we use NetworkUtils.getInstance() unconditionally.

Comment 5

[:garvan]

Victor: gah. Wrong editor, will fix gecko whitespacing on that patch.

James: thanks for feedback, will look at that and see what other guards can go in.

Comment 6

[Victor Ng [:vng] [:crankycoder]]

Immediately returning on a null intent makes a lot more sense to me.

Comment 7

[:garvan]

Victor can you do a look at upload() also for anything that is checkable. I don't see anything that is missed.

Comment 8

[:garvan]

Attachment: if null intent, return from onHandleIntent

carry over r+ from vng.
fixed whitespace.

Comment 9

[Victor Ng [:vng] [:crankycoder]]

The upload() looks ok to me.  The only snags would've been the DatastorageManager and NetworkInfo instances not being initialized, but those all happen when the StumblerService has init() called.

Comment 10

[:garvan]

Comment on attachment 8559999 [details] [diff] [review]
if null intent, return from onHandleIntent

Approval Request Comment
[Feature/regressing bug #]: Bug 1106584, incomplete NPE fix attempt 
[User impact if declined]: If user opted-in to stumbling, this crash can happen. The opt-in level is estimated ~5000 users, and the crash is low incidence (26 crashes - although likely under under-reported because not all crashes are submitted to the Play Store). 
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: No-risk patch, just an immediate null pointer check and return.
[String/UUID change made/needed]: None.

Comment 11

[:garvan]

Try build green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=914510550e89

(some tests are still running ATM, those aren't relevant)

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

The fix looks simple enough but we have had regressions from NPE fixes before. Let's get this on m-c ASAP. Once it's there, do you have a way to verify the fix?

If this has been verified on m-c by Monday, when we gtb with Beta 8, we can consider it for inclusion in 36. If not, it will likely be in 37. From the user impact described in comment 10, I don't think this is a driver for release. It may be a suitable ride along fix but I don't expect another 35 point release at this point.

Comment 13

[:garvan]

We can't repro this. We have test cases for this we added: https://github.com/mozilla/MozStumbler/pull/1462

Unfortunately, the crash report isn't even lining up with a crashable line. In summary, we have no developer repro, and can't confirm the exact point of the crash. A frustrating case. This is our best-guess fix, and if it doesn't work, that is good in sense because we know to look elsewhere.

I agree NPE checks can cause regressions because it can cause unexpected behaviour in the callers, but this check is at the entry point to the Android service.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/3da0ac9c71d5

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3da0ac9c71d5

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8559999 [details] [diff] [review]
if null intent, return from onHandleIntent

Low risk, taking it in 36.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7b7838347d18

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/130e59d97b15

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8559999 [details] [diff] [review]
if null intent, return from onHandleIntent

Clearing the uplift flags as we are working on the 36 release now.