Closed Bug 1095914 Opened 10 years ago Closed 10 years ago

Port critical fixes from mozstumber github to Fennec stumbler

Categories

(Android Background Services Graveyard :: Geolocation, defect)

Firefox 35
defect
Not set
normal

Tracking

(firefox35 fixed, firefox36 fixed, fennec35+)

RESOLVED FIXED
Firefox 36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
fennec 35+ ---

People

(Reporter: garvan, Assigned: garvan)

Details

Attachments

(5 files, 3 obsolete files)

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
Attachment #8519426 - Flags: review?(vng)
Attachment #8519428 - Flags: review?(vng)
Attachment #8519429 - Flags: review?(vng)
Attachment #8519430 - Flags: review?(vng)
Comment on attachment 8519436 [details] [diff] [review]
Copy latest CellScanner code from mozstumbler github

I see some cleanup items before review.
Attachment #8519436 - Flags: review?(nalexander)
Attachment #8519426 - Flags: review?(vng)
Attachment #8519426 - Flags: review+
(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.
Attachment #8520372 - Attachment is obsolete: true
Attachment #8520372 - Flags: review?(nalexander)
Attachment #8520700 - Flags: review?(nalexander)
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.
Attachment #8520700 - Flags: review?(nalexander) → review+
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.
Attachment #8520700 - Attachment is obsolete: true
Attachment #8522433 - Flags: review+
Attachment #8522433 - Flags: feedback?(vng)
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 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.
Attachment #8522433 - Flags: feedback?(vng) → feedback+
Try is green, link in comment 11
Keywords: checkin-needed
Comment on attachment 8519428 [details] [diff] [review]
161c693: NPE no telephony manager in Reporter startup

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

r+
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.
Attachment #8519426 - Flags: approval-mozilla-aurora?
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
Attachment #8519428 - Flags: approval-mozilla-aurora?
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.
Attachment #8519429 - Flags: approval-mozilla-aurora?
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.
Attachment #8519430 - Flags: approval-mozilla-aurora?
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.
Attachment #8522433 - Flags: approval-mozilla-aurora?
Attachment #8519428 - Flags: review?(vng) → review+
Attachment #8519429 - Flags: review?(vng) → review+
Attachment #8519430 - Flags: review?(vng) → review+
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.
Attachment #8519429 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8519426 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8519428 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8519430 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8522433 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: --- → ?
tracking-fennec: ? → 35+
You need to log in before you can comment on or make changes to this bug.