If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Port critical fixes from mozstumber github to Fennec stumbler

RESOLVED FIXED in Firefox 35

Status

Android Background Services
Geolocation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: garvan, Assigned: garvan)

Tracking

Firefox 35
Firefox 36

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed, fennec35+)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
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
(Assignee)

Comment 1

3 years ago
Created attachment 8519426 [details] [diff] [review]
36dfba8:  NPE WifiScanner
Attachment #8519426 - Flags: review?(vng)
(Assignee)

Comment 2

3 years ago
Created attachment 8519428 [details] [diff] [review]
161c693: NPE no telephony manager in Reporter startup
Attachment #8519428 - Flags: review?(vng)
(Assignee)

Comment 3

3 years ago
Created attachment 8519429 [details] [diff] [review]
25dba57: NPE no app context
Attachment #8519429 - Flags: review?(vng)
(Assignee)

Comment 4

3 years ago
Created attachment 8519430 [details] [diff] [review]
35a0654: NPE no GPS provider
Attachment #8519430 - Flags: review?(vng)
Comment hidden (spam)
(Assignee)

Comment 6

3 years ago
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+
Attachment #8519428 - Flags: review+
Attachment #8519429 - Flags: review+
Attachment #8519430 - Flags: review+
Comment hidden (spam)
(Assignee)

Comment 8

3 years ago
Created attachment 8520700 [details] [diff] [review]
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.
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+
(Assignee)

Comment 10

3 years ago
Created attachment 8522433 [details] [diff] [review]
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.
Attachment #8520700 - Attachment is obsolete: true
Attachment #8522433 - Flags: review+
Attachment #8522433 - Flags: feedback?(vng)
(Assignee)

Comment 11

3 years ago
Try
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8d5ca93ca444
(Assignee)

Comment 12

3 years ago
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+
(Assignee)

Comment 14

3 years ago
Try is green, link in comment 11
Keywords: checkin-needed
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
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Comment on attachment 8519428 [details] [diff] [review]
161c693: NPE no telephony manager in Reporter startup

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

r+
(Assignee)

Comment 18

3 years ago
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?
(Assignee)

Comment 19

3 years ago
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?
(Assignee)

Comment 20

3 years ago
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?
(Assignee)

Comment 21

3 years ago
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?
(Assignee)

Comment 22

3 years ago
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+
(Assignee)

Updated

3 years ago
tracking-fennec: --- → ?
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
status-firefox35: --- → fixed
status-firefox36: --- → fixed
tracking-fennec: ? → 35+
You need to log in before you can comment on or make changes to this bug.