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

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Geolocation, defect)

Description

[Aaron Train [:aaronmt]]

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

Comment 1

[:garvan]

Attachment: part1: remove contextless access to NetworkUtils

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

Comment 2

[:garvan]

Attachment: part 2:make upload service nonsticky

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.

Comment 3

[Richard Newman [: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.

Comment 4

[:garvan]

Attachment: part1-remove-contextless-access-to-netutils.diff

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

Comment 5

[:garvan]

try green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=abae043f5cc5

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc24891d96e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e74bebb3250d

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/cc24891d96e0
https://hg.mozilla.org/mozilla-central/rev/e74bebb3250d

Comment 8

[:garvan]

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

Comment 9

[:garvan]

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

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Garvan, 38 is marked as affected and Aaron mentioned it in comment #0, any reason why you don't request uplift to 38?

Comment 11

[:garvan]

Comment on attachment 8593526 [details] [diff] [review]
part 2:make upload service nonsticky

see comment 9

Comment 12

[:garvan]

Comment on attachment 8594904 [details] [diff] [review]
part1-remove-contextless-access-to-netutils.diff

see comment 9

Comment 13

[Sylvestre Ledru [:Sylvestre]]

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)

Comment 14

[:garvan]

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.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

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

Comment 16

[:garvan]

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?

Comment 17

[Sylvestre Ledru [:Sylvestre]]

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!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/250baad4c5d2
https://hg.mozilla.org/releases/mozilla-aurora/rev/f457073129c4

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/1ec2ee773b51
https://hg.mozilla.org/releases/mozilla-release/rev/645fc5aa6a49

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1ec2ee773b51
https://hg.mozilla.org/releases/mozilla-beta/rev/645fc5aa6a49