Closed Bug 1155237 Opened 4 years ago Closed 4 years ago

Crash @ java.lang.NullPointerException: Attempt to invoke virtual method 'boolean org.mozilla.mozstumbler.service.utils.NetworkUtils.isWifiAvailable()' on a null object reference

Categories

(Android Background Services Graveyard :: Geolocation, defect)

Firefox 38
ARM
Android
defect
Not set

Tracking

(firefox38 fixed, firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed, fennec39+)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed
fennec 39+ ---

People

(Reporter: aaronmt, Assigned: garvan)

Details

(Keywords: crash)

Attachments

(2 files, 1 obsolete file)

java.lang.NullPointerException: Attempt to invoke virtual method 'boolean org.mozilla.mozstumbler.service.utils.NetworkUtils.isWifiAvailable()' on a null object reference
at org.mozilla.mozstumbler.service.uploadthread.UploadAlarmReceiver$UploadAlarmService.onHandleIntent(UploadAlarmReceiver.java:58)
at android.app.IntentService$ServiceHandler.handleMessage(IntentService.java:65)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loop(Looper.java:145)
at android.os.HandlerThread.run(HandlerThread.java:61)

This is on the Play Store on 38 with a handful of reports
tracking-fennec: ? → 39+
The static access to NetworkUtils class is triggering the NPE. It was there for (not particularly good) reasons outside of Fennec usage. And I am removing this usage from fennec and Mozilla Stumbler
Attachment #8593525 - Flags: review?(rnewman)
The upload alarm is scheduled by the stumbler module sometime after fennec startup. It doesn't need to be sticky, it will get rescheduled in future next time fennec is run.
Attachment #8593526 - Flags: review?(rnewman)
Comment on attachment 8593525 [details] [diff] [review]
part1: remove contextless access to NetworkUtils

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

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/uploadthread/AsyncUploader.java
@@ +41,2 @@
>          public final boolean mUseWifiOnly;
> +        public UploadSettings(boolean shouldIgnoreWifiStatus, 

Nit: trailing whitespace.

@@ +148,5 @@
>          long uploadedObservations = 0;
>          long uploadedCells = 0;
>          long uploadedWifis = 0;
>  
> +        if (!mSettings.mShouldIgnoreWifiStatus && mSettings.mUseWifiOnly && mSettings.mIsWifiAvailable) {

This is no longer really 'Settings', is it?

Why not just pass the NetworkUtils instance to AsyncUploader as an argument?

UploadSettings stays the same, and you actually get *more accurate* details about whether wifi is available, because you can call isWifiAvailable when you need it, not during construction.
Attachment #8593525 - Flags: review?(rnewman)
Attachment #8593526 - Flags: review?(rnewman) → review+
Status: NEW → ASSIGNED
Component: General → Geolocation
Product: Firefox for Android → Android Background Services
> > +        if (!mSettings.mShouldIgnoreWifiStatus && mSettings.mUseWifiOnly && mSettings.mIsWifiAvailable) {
> 
> This is no longer really 'Settings', is it?

It is if you like badly named variables :). I'll fix that, changed to AsyncUploadArgs, since it is just a collection of arguments to the AsyncUploader task.

> Why not just pass the NetworkUtils instance to AsyncUploader as an argument?

Ok, so you _actually_ read these PRs :). 
Agreed, the AsyncUploadArgs now has mNetworkUtils as a member.

These changes are minor (removed extra ws also), so carrying over r+
Attachment #8593525 - Attachment is obsolete: true
Attachment #8594904 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/cc24891d96e0
https://hg.mozilla.org/mozilla-central/rev/e74bebb3250d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8594904 [details] [diff] [review]
part1-remove-contextless-access-to-netutils.diff

Approval Request Comment
[Feature/regressing bug #]: Part of geolocation stumbling in Fennec
[User impact if declined]: Continued intermittent crashing if Fennec stumbling enabled
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: Minor changes, low risk
[String/UUID change made/needed]: None
Attachment #8594904 - Flags: approval-mozilla-aurora?
Comment on attachment 8593526 [details] [diff] [review]
part 2:make upload service nonsticky

Approval Request Comment
[Feature/regressing bug #]: Part of geolocation stumbling in Fennec
[User impact if declined]: Continued intermittent crashing if Fennec stumbling enabled
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: Minor changes, low risk
[String/UUID change made/needed]: None
Attachment #8593526 - Flags: approval-mozilla-aurora?
Garvan, 38 is marked as affected and Aaron mentioned it in comment #0, any reason why you don't request uplift to 38?
Flags: needinfo?(gkeeley)
Comment on attachment 8593526 [details] [diff] [review]
part 2:make upload service nonsticky

see comment 9
Flags: needinfo?(gkeeley)
Attachment #8593526 - Flags: approval-mozilla-beta?
Comment on attachment 8594904 [details] [diff] [review]
part1-remove-contextless-access-to-netutils.diff

see comment 9
Attachment #8594904 - Flags: approval-mozilla-beta?
Comment on attachment 8593526 [details] [diff] [review]
part 2:make upload service nonsticky

[Triage Comment]

Thanks. Taking it for 38 beta 7
(we already merged, so, it is going to land in m-r)
Attachment #8593526 - Flags: approval-mozilla-release+
Attachment #8593526 - Flags: approval-mozilla-beta?
Attachment #8593526 - Flags: approval-mozilla-aurora?
Attachment #8593526 - Flags: approval-mozilla-aurora+
part2 got ok'd, can you mark part1 also? Thanks.

This is my first uplift since I got L1 access, and it has go straight to m-r, just my freaking luck. To confirm/double-check/triple-check, there is nothing special/different that comes to mind here in the landing process vs. landing like one normally would to m-c.
Flags: needinfo?(sledru)
Comment on attachment 8594904 [details] [diff] [review]
part1-remove-contextless-access-to-netutils.diff

Don't worry, it goes into m-r but we will build a few more beta.
Sheriffs are going to take care of the uplift for you.

To sum up: no need to worry ;)
Flags: needinfo?(sledru)
Attachment #8594904 - Flags: approval-mozilla-release+
Attachment #8594904 - Flags: approval-mozilla-beta?
Attachment #8594904 - Flags: approval-mozilla-aurora?
Attachment #8594904 - Flags: approval-mozilla-aurora+
Hold the phone -to quadruple-check- I don't land this myself to m-a or to m-r, the sherrifs do that for me?
You could but you should leave the pro doing that for you (most of the devs are not doing it).
So, you don't have to do anything more!
You need to log in before you can comment on or make changes to this bug.