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)
Tracking
(firefox35 fixed, firefox36 fixed, fennec35+)
RESOLVED
FIXED
Firefox 36
People
(Reporter: garvan, Assigned: garvan)
Details
Attachments
(5 files, 3 obsolete files)
2.54 KB,
patch
|
vng
:
review+
vng
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
vng
:
review+
vng
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.32 KB,
patch
|
vng
:
review+
vng
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.25 KB,
patch
|
vng
:
review+
vng
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
40.30 KB,
patch
|
garvan
:
review+
vng
:
feedback+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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 hidden (spam) |
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)
Updated•10 years ago
|
Attachment #8519426 -
Flags: review?(vng)
Attachment #8519426 -
Flags: review+
Updated•10 years ago
|
Attachment #8519428 -
Flags: review+
Updated•10 years ago
|
Attachment #8519429 -
Flags: review+
Updated•10 years ago
|
Attachment #8519430 -
Flags: review+
Comment hidden (spam) |
(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 9•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
Try https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8d5ca93ca444
Assignee | ||
Comment 12•10 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 13•10 years ago
|
||
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+
Comment 15•10 years ago
|
||
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]
Comment 16•10 years ago
|
||
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
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Comment 17•10 years ago
|
||
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•10 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•10 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•10 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•10 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•10 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?
Updated•10 years ago
|
Attachment #8519428 -
Flags: review?(vng) → review+
Updated•10 years ago
|
Attachment #8519429 -
Flags: review?(vng) → review+
Updated•10 years ago
|
Attachment #8519430 -
Flags: review?(vng) → review+
Comment 23•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8519426 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8519428 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8519430 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Attachment #8522433 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•9 years ago
|
||
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
Updated•9 years ago
|
tracking-fennec: ? → 35+
You need to log in
before you can comment on or make changes to this bug.
Description
•