Closed Bug 1130052 Opened 5 years ago Closed 5 years ago

Stumbler crash NPE


(Android Background Services Graveyard :: Geolocation, defect)

Firefox 35
Not set


(firefox35 wontfix, firefox36 fixed, firefox37 fixed, firefox38 fixed)

Firefox 38
Tracking Status
firefox35 --- wontfix
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed


(Reporter: garvan, Assigned: garvan)




(1 file, 1 obsolete file)

see also bug

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

at org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver$UploadAlarmService.onHandleIntent(
at android.os.Handler.dispatchMessage(
at android.os.Looper.loop(
Flags: needinfo?(snorp)
(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:
I am going to go with returning from onHandleIntent(Intent) if the intent is null:

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.
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.
Attachment #8559980 - Flags: review?(vng)
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.
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.
Immediately returning on a null intent makes a lot more sense to me.
Attachment #8559980 - Flags: review?(vng) → review+
Victor can you do a look at upload() also for anything that is checkable. I don't see anything that is missed.
Flags: needinfo?(snorp)
carry over r+ from vng.
fixed whitespace.
Attachment #8559980 - Attachment is obsolete: true
Attachment #8559999 - Flags: review+
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 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.
Attachment #8559999 - Flags: approval-mozilla-release?
Attachment #8559999 - Flags: approval-mozilla-beta?
Attachment #8559999 - Flags: approval-mozilla-aurora?
Try build green:

(some tests are still running ATM, those aren't relevant)
Keywords: checkin-needed
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.
Flags: needinfo?(gkeeley)
We can't repro this. We have test cases for this we added:

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.
Flags: needinfo?(gkeeley)
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Comment on attachment 8559999 [details] [diff] [review]
if null intent, return from onHandleIntent

Low risk, taking it in 36.
Attachment #8559999 - Flags: approval-mozilla-beta?
Attachment #8559999 - Flags: approval-mozilla-beta+
Attachment #8559999 - Flags: approval-mozilla-aurora?
Attachment #8559999 - Flags: approval-mozilla-aurora+
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.
Attachment #8559999 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.