1095914 - Port critical fixes from mozstumber github to Fennec stumbler

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Geolocation, defect)

Description

[:garvan]

Each one of these addresses a crash in the stumbler, discovered through Mozilla Stumbler and fixed there.
The last item on the list is to take the CellScanner file verbatim from mozstumbler github rather than port individual fixes, as that file has been refactored for thread-safety.

sha: 36dfba8
https://github.com/mozilla/MozStumbler/pull/1192
ACRA Report: NPE WifiScanner.java:98

sha: 161c693
https://github.com/mozilla/MozStumbler/pull/1194
1185 NPE Reporter.startup (no telephony manager)

sha: 25dba57
https://github.com/mozilla/MozStumbler/pull/1195
1187 - NPE - Appears to be from no app context

sha: 35a0654
https://github.com/mozilla/MozStumbler/pull/1206
1170 ACRA report: provider doesn't exist: gps

Update CellScanner.java (and replace CellScannerNoWCDMA.java with CellScannerImplementation.java).
Rather than do single commits, just replace the 2 files entirely. Here are the important changes in those files:
* 39bbbaa
The cell scanning code was split to remove wcdma scanning on Fennec due to an older API level on Fennec. This is no longer the case. CellScannerNoWCDMA.java renamed to CellScannerImplementation.java.
* ebb23b5
Remove broadcastsync on the timer thread, have the timer thread message back to the owning class through a handler (guaranteed thread-safe mechanism to notify the owning class that work is done)
* c1f5743 
Null-check the TelephonyManager availability

Comment 1

[:garvan]

Attachment: 36dfba8: NPE WifiScanner

Comment 2

[:garvan]

Attachment: 161c693: NPE no telephony manager in Reporter startup

Comment 3

[:garvan]

Attachment: 25dba57: NPE no app context

Comment 4

[:garvan]

Attachment: 35a0654: NPE no GPS provider

Comment 6

[:garvan]

Comment on attachment 8519436 [details] [diff] [review]
Copy latest CellScanner code from mozstumbler github

I see some cleanup items before review.

Comment 8

[:garvan]

Attachment: Copy latest CellScanner code from mozstumbler github

(I keep making changes in github, and then updating this patch, will stop now.)

Important changes:
1) Null-check the TelephonyManager availability

2) The cell scanning code was split to remove wcdma scanning on Fennec due to an older API level on Fennec. This is no longer the case. CellScannerNoWCDMA.java renamed to CellScannerImplementation.java.

3) Remove broadcastsync on the timer thread, have the timer thread message back to the owning class through a handler (guaranteed thread-safe mechanism to notify the owning class that work is done).

NOTE: If you want to apply locally, this patch applies after bug 1036514. Will ensure this is committed after that bug.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8520700 [details] [diff] [review]
Copy latest CellScanner code from mozstumbler github

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

Have a rubber stamp.  You and vng can do the real review here.

Comment 10

[:garvan]

Attachment: Copy latest CellScanner code from mozstumbler github

Thanks Nick.
Changed 2 minor lines to sync better with github. Carrying over r+.

Victor, marked you for f?, as this code is being ported from github you are already familiar with it, but please eyeball the patch in case you think:
- there is more I should pull in from github
- there is cleaner way to be landing these patches from github to here

ASIDE: Victor and Nick, we should discuss future plans for landing code from github in Portland.

Comment 11

[:garvan]

Try
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8d5ca93ca444

Comment 12

[:garvan]

I forgot to mention an easy to grok github milestone for these changes: they are part of the Nov 4th Mozilla Stumbler 1.3 release on the play store.

Comment 13

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

Comment on attachment 8522433 [details] [diff] [review]
Copy latest CellScanner code from mozstumbler github

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

Reporter::receivedGpsMessages is slightly different between v1.3.0 and Fennec.  Those don't look consequential.  The NPE fixes for this bug look fine though.

Comment 14

[:garvan]

Try is green, link in comment 11

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/344c9c48fb33
https://hg.mozilla.org/integration/fx-team/rev/1ef454e43aa2
https://hg.mozilla.org/integration/fx-team/rev/76675cb87138
https://hg.mozilla.org/integration/fx-team/rev/fb5f63e54648
https://hg.mozilla.org/integration/fx-team/rev/f23b9848820f

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/344c9c48fb33
https://hg.mozilla.org/mozilla-central/rev/1ef454e43aa2
https://hg.mozilla.org/mozilla-central/rev/76675cb87138
https://hg.mozilla.org/mozilla-central/rev/fb5f63e54648
https://hg.mozilla.org/mozilla-central/rev/f23b9848820f

Comment 17

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

Comment on attachment 8519428 [details] [diff] [review]
161c693: NPE no telephony manager in Reporter startup

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

r+

Comment 18

[:garvan]

Comment on attachment 8519426 [details] [diff] [review]
36dfba8:  NPE WifiScanner

Approval Request Comment
[Feature/regressing bug #]: Stumbler for Fennec
[User impact if declined]: Crash if wifi manager service is unavailable. Unknown reproducibility, reported as a crash in Mozilla Stumbler in automated crash report.
[Describe test coverage new/current, TBPL]: None
[Risks and why]: The crash report was a custom ROM, so I assume some custom ROMs have extra settings to disable WIFI at the OS level.  
[String/UUID change made/needed]: None.

Comment 19

[:garvan]

Comment on attachment 8519428 [details] [diff] [review]
161c693: NPE no telephony manager in Reporter startup

Approval Request Comment
[Feature/regressing bug #]: Stumbler for Fennec
[User impact if declined]: Major. Crash on devices with no phone. 
[Describe test coverage new/current, TBPL]: None.
[Risks and why]: No code risks, just a missing null pointer check.
[String/UUID change made/needed]: None

Comment 20

[:garvan]

Comment on attachment 8519429 [details] [diff] [review]
25dba57: NPE no app context

Approval Request Comment
[Feature/regressing bug #]: Stumbler for Fennec
[User impact if declined]: Possible crash. This was a non-reproducible crash reported by automated crash reports for Mozilla Stumbler.
[Describe test coverage new/current, TBPL]: None.
[Risks and why]: No risk, just adds a null pointer check.
[String/UUID change made/needed]: None.

Comment 21

[:garvan]

Comment on attachment 8519430 [details] [diff] [review]
35a0654: NPE no GPS provider

Approval Request Comment
[Feature/regressing bug #]: Stumbler for Fennec
[User impact if declined]: Bad. Devices with no GPS will crash.
[Describe test coverage new/current, TBPL]: None.
[Risks and why]: Code just adds missing check for GPS availability. No risk.
[String/UUID change made/needed]: None.

Comment 22

[:garvan]

Comment on attachment 8522433 [details] [diff] [review]
Copy latest CellScanner code from mozstumbler github

Approval Request Comment
[Feature/regressing bug #]: Stumbler for Fennec
[User impact if declined]: 1) Crash if no phone on the device. 2) For devices with a phone, it won't scan CDMA towers, so we are missing coverage.
[Describe test coverage new/current, TBPL]: None
[Risks and why]: Low risk, this code is what was released to the public in Mozilla Stumbler, which has a few thousand users, and no reports of problems in this code.
[String/UUID change made/needed]: None.

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8519429 [details] [diff] [review]
25dba57: NPE no app context

a = me on all of these, no need for multiple approvals if the work is on big piece that all needs uplift.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/75e35dfbebcb
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd7fbe828b10
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3ae3d724956
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c8a21d22dc3
https://hg.mozilla.org/releases/mozilla-aurora/rev/2f5d3b2f5f64