Closed Bug 1414965 Opened 2 years ago Closed 2 years ago

Reset search geo preferences due to bug 1413652

Categories

(Firefox :: Search, defect)

57 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 blocking verified
firefox58 --- verified

People

(Reporter: mkaply, Assigned: mkaply)

Details

Attachments

(2 files)

Because of bug 1413652, we know people have incorrect geo values.

We're going to do a reset of these prefs with Firefox 57.
Comment on attachment 8925678 [details]
Bug 1414965 - Reset geo preferences for Firefox 57.

https://reviewboard.mozilla.org/r/196808/#review202036

r=me, let's get this into nightlies soon.

::: browser/components/nsBrowserGlue.js:2168
(Diff revision 3)
>              lwthemePrefs.setStringPref("usedThemes", JSON.stringify(usedThemes));
>            }
>          } catch (e) { /* Don't panic if this pref isn't what we expect it to be. */ }
>        }
>      }
> +    if (currentUIVersion < 58) {

Please add an empty line before this block to match the style of the rest of the method.
Attachment #8925678 - Flags: review?(florian) → review+
[Tracking Requested - why for this release]: Needed for updating geos.
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/d44fe4ae02fe
Reset geo preferences for Firefox 57. r=florian
I am told this is a change that *must* happen in 57, tracked as blocking.
> Backed out for unexpected connection in talos test ts_paint_heavy.

The log you posted seems to be about a crash which is this:

https://bugzilla.mozilla.org/show_bug.cgi?id=1409685

or did I miss something? Where in the log does it talk about a connection?

ccing florian to see if he has any ideas when he gets in tomorrow.
Flags: needinfo?(mozilla) → needinfo?(florian)
Ok, this is a crash in [@ mozilla::net::nsSocketTransport::InitiateSocket] which is something we see as signature if there is an unexpected connection in test automation and when the process gets terminated. Here the text about the unexpected connection is missing.
The crash indicates that we are attempting to do an external network request, which isn't fine for tests. This request is most likely the geoip lookup.

When the geoip code was introduced (bug 1109120), these 2 prefs were introduced in various pieces of test code to avoid the network request:
user_pref("browser.search.isUS", true);
user_pref("browser.search.countryCode", "US");

In the current talos code, this is done at http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/testing/talos/talos/config.py#136

The code we are landing in this bug clears these values in the migration code. For most tests this is fine because the migration code doesn't run for new profiles (profiles that don't have a user value for the browser.migration.version pref). ts_paint_heavy specifically uses an existing profile so it goes through the migration code; we'll need to find another way to disable the request for this test.
Comment on attachment 8925903 [details] [diff] [review]
Disable geoip lookups on Talos

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

I verified locally this fixes the crash, also I have seen it on try
Attachment #8925903 - Flags: review?(jmaher) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f758a76a87
Reset geo preferences for Firefox 57. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfe61dcf41a
disable geoip lookups for talos tests, r=jmaher.
Hi Mike, please nominate for uplift to beta57.
Flags: needinfo?(mozilla)
Comment on attachment 8925678 [details]
Bug 1414965 - Reset geo preferences for Firefox 57.

Approval Request Comment
[Feature/Bug causing the regression]: Reset geo due to bug 141495
[User impact if declined]: Bad geolocation
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: yes.
[Needs manual test from QE? If yes, steps to reproduce]: Doc has been provided to QE.
[List of other uplifts needed for the feature/fix]: Talos fix in subsequent patch
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Resets prefs to what they would have been in a new profile.
[String changes made/needed]:
Flags: needinfo?(mozilla)
Attachment #8925678 - Flags: approval-mozilla-beta?
(In reply to Mike Kaply [:mkaply] from comment #17)

> Approval Request Comment

> [List of other uplifts needed for the feature/fix]: Talos fix in subsequent
> patch

We actually don't need the talos patch on 57, it's fixing a talos test that has been introduced in 58 (in bug 1407398).
https://hg.mozilla.org/mozilla-central/rev/b7f758a76a87
https://hg.mozilla.org/mozilla-central/rev/bbfe61dcf41a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8925678 [details]
Bug 1414965 - Reset geo preferences for Firefox 57.

Must fix, Beta57+
Attachment #8925678 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
We verified the fix pushed for this issue on 58.0a1 (2017-11-08), using Windows 10 (64-bit), macOS 10.13 and Ubuntu 16.04 (64-bit) -- everything works as expected. 

Detailed test results are available here: https://goo.gl/kX1cAi.

Note: we plan to verify this on 57.0 as well, as soon as we have builds available.
This fix has been verified on 57.0 [1] as well, using Windows 10 (64-bit), macOS 10.13 and Ubuntu 16.04 (64-bit) -- everything works as expected. 

Detailed test results are available here: https://goo.gl/kX1cAi.


[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-release&revision=ea14b85abeac942084437602be8486b410e4bfbd&filter-searchStr=tc(n)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.