Closed Bug 1110420 Opened 5 years ago Closed 5 years ago

getInUS should return false for custom builds (i.e. distribution.ini)

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.3 - 12 Jan
Tracking Status
firefox34 --- wontfix
firefox35 + verified
firefox36 + verified
firefox37 + verified

People

(Reporter: mconnor, Assigned: Gavin)

References

Details

Attachments

(1 file)

If there's a setting in a distribution.ini file, we should just use that.  I assume this is the cleanest way to do this.
OS: Windows 8.1 → All
Hardware: x86_64 → All
Flags: qe-verify+
Flags: firefox-backlog+
Points: --- → 2
Attached patch patchSplinter Review
Kind of forgot that we need this in 35... Mark/dolske, can one of you review this ASAP?
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #8541703 - Flags: review?(mhammond)
Attachment #8541703 - Flags: review?(dolske)
Attachment #8541703 - Flags: review?(mhammond)
Attachment #8541703 - Flags: review?(dolske)
Attachment #8541703 - Flags: review+
Comment on attachment 8541703 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TBPL]:
[Risks and why]: 
[String/UUID change made/needed]:

Approval Request Comment
[Feature/regressing bug #]: Firefox 34 search work
[User impact if declined]: bug 1110209 in Firefox 35
[Describe test coverage new/current, TBPL]: none yet
[Risks and why]: low risk check
[String/UUID change made/needed]: none
Attachment #8541703 - Flags: approval-mozilla-beta?
Attachment #8541703 - Flags: approval-mozilla-aurora?
Comment on attachment 8541703 [details] [diff] [review]
patch

Self-approving, I guess - discussed this with Lukas via email.
Attachment #8541703 - Flags: approval-mozilla-beta?
Attachment #8541703 - Flags: approval-mozilla-beta+
Attachment #8541703 - Flags: approval-mozilla-aurora?
Attachment #8541703 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/163080209ca9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Iteration: --- → 37.3
QA Contact: petruta.rasa
Are there some steps and/or results I could use in order to verify this? Thanks!
(In reply to Petruta Rasa [QA] [:petruta] from comment #8)
> Are there some steps and/or results I could use in order to verify this?

I can't think of a reasonable test scenario - Gavin, do you have ideas, or can we just qe-verify- this?
Flags: needinfo?(gavin.sharp)
It's not a complete test, but you could set distribution.id to an arbitrary value in about:config in a 33 build, and then update to a 35 (or later) build and verify that with a US timezone, the default engine remains Google (and not Yahoo).
Flags: needinfo?(gavin.sharp)
Using normal en-US 33.0 beta builds and adding distribution.id pref in about:config (clean profiles, US timezone), the updated 35.0 builds had the Yahoo set as default search provider. In those cases the preference browser.search.isUS was set to True after update.
Only once the default search engine was Google and the pref browser.search.isUS False - in this case I used a dirty profile and distribution.id=" " (a null string).

On the other hand, Partner Repacks builds are only updated to 34.0 on both "beta" and "release" channels. For the moment I can't use them to check this bug for Firefox 35.
(In reply to Petruta Rasa [QA] [:petruta] from comment #11)
> Using normal en-US 33.0 beta builds and adding distribution.id pref in
> about:config (clean profiles, US timezone), the updated 35.0 builds had the
> Yahoo set as default search provider. In those cases the preference
> browser.search.isUS was set to True after update.

Hmm, I can't reproduce this. I get Yahoo after update with those steps (and no browser.search.isUS). What are you setting the distribution.id pref to?
Flags: needinfo?(petruta.rasa)
I got Yahoo too and not Google as supposed to (comment 10).
I tried with different values for distribution.id - 3, xyz, " ".
Flags: needinfo?(petruta.rasa)
Just to clarify, you are adding distribution.id as a string pref, right? It must be a string and it must not be empty (e.g. "foo" would work but "" would not).

When you are testing this, does distribution.id appear as set in about:config after updating to Firefox 35?

After updating to 35, can you enable devtools.chrome.enabled, open the Browser Console, and then execute "Services.prefs.getCharPref("distribution.id");" and see what the result is?
Flags: needinfo?(petruta.rasa)
Retested using different update channels: beta, beta-cdntest, release.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #14)
> Just to clarify, you are adding distribution.id as a string pref, right? It must be a string and it must not be   > empty (e.g. "foo" would work but "" would not).
Yes.

> When you are testing this, does distribution.id appear as set in about:config after updating to Firefox 35?
Yes, the pref and the value are the same after update.
 
> After updating to 35, can you enable devtools.chrome.enabled, open the Browser Console, and then execute
> "Services.prefs.getCharPref("distribution.id");" and see what the result is?
The result is the string value that I set in about:config for distribution.id ("foo").

The browser.search.isUS was set to true only on "beta" channel, on the other two it was false.

The default search engine remained Google in all channels. Only on beta channel it switched to Yahoo after updating from 33 beta 6 to 35 beta 3. Performing manual update again, the build updated to Firefox 35 release and the default search engine was switched back to Google.
On "release" and "beta-cdntest" the builds were updated directly to Firefox 36 beta 1.
Flags: needinfo?(petruta.rasa)
Gavin, what do you suggest regarding the above comments?
Flags: needinfo?(gavin.sharp)
It sounds to me like perhaps your beta test is being confused because it us updating to a beta build from before this patch landed? Can you maybe provide the about:config "built from" changesets for all of the builds that you are updating to?
Flags: needinfo?(gavin.sharp)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #17)
> provide the about:config "built from" changesets

(noting this here only because I managed to confuse Mark: I meant about:buildconfig "built from" changesets)
(In reply to Petruta Rasa [QA] [:petruta] from comment #18)
> Built from https://hg.mozilla.org/releases/mozilla-beta/rev/1b26127c3323

This build contains the fix for this bug. So just to confirm, you are upgrading from a new profile in a build older than 34.0.5 directly to this build, and after the update browser.search.isUS is true? That should really be impossible :/
Flags: needinfo?(petruta.rasa)
Could this be explained by the geoip request taking longer than 2s to complete?  IIUC, revision 1b26127c3323 doesn't have the improved timeout handling we recently landed, so a timeout will force a fallback to the timezone check.  The next startup might see geoip succeed and thus override the previous value found via the timezone.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #20)
> This build contains the fix for this bug. So just to confirm, you are
> upgrading from a new profile in a build older than 34.0.5 directly to this
> build, and after the update browser.search.isUS is true? That should really
> be impossible :/

As mentioned in comment 15, 33 beta 6 build wasn't updated directly to 34.0.5 - it updated to 35 beta 3 first. I opened that build again now and the pref is false, as Mark suggests above.

I installed 33 beta 6 again and set the distribution.id to "foo" - this time it updated directly to 36 beta 2 having the default search engine Google. The pref browser.search.isUS is false now.
Flags: needinfo?(petruta.rasa)
Sorry - I think we crossed wires here.

I had assumed we were talking only about verification in Firefox 35, where GeoIP shouldn't be causing any issues (because it didn't land there).

Comment 18 links to a 36 beta build, and then a 35 release build. If you are updating to a 36 beta build before updating to a 35 release build, then it is expected that the verification will go wrong, because the GeoIP code in Firefox 36 will throw things off.

It's probably best to avoid using actual updates to verify this bug, given the strange update behavior. The best way to verify this for Firefox 35 is:

1) set your system clock to be in a North American time zone 
2) install a Firefox 33 build, set the distribution.id pref to "foo". Default engine should be Google.
3) with the same profile, launch a Firefox 35 build
4) Expected:
  - browser.search.isUS is not set
  - Google is still the default engine

To verify in Firefox 36 or later requires an addition to step 2 (set the browser.search.geoip.url pref to be 'data:application/json,{"country_code": "US"}'), and probably requires fixing bug 1123974 first, so we can hold off on that.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #23)
> Sorry - I think we crossed wires here.
> 
> I had assumed we were talking only about verification in Firefox 35, where
> GeoIP shouldn't be causing any issues (because it didn't land there).

It does update to Firefox 36 now since it's the latest beta version :)

> Comment 18 links to a 36 beta build, and then a 35 release build. If you are
> updating to a 36 beta build before updating to a 35 release build, then it
> is expected that the verification will go wrong, because the GeoIP code in
> Firefox 36 will throw things off.
> 
> It's probably best to avoid using actual updates to verify this bug, given
> the strange update behavior. The best way to verify this for Firefox 35 is:
> 
> 1) set your system clock to be in a North American time zone 
> 2) install a Firefox 33 build, set the distribution.id pref to "foo".
> Default engine should be Google.
> 3) with the same profile, launch a Firefox 35 build
> 4) Expected:
>   - browser.search.isUS is not set
>   - Google is still the default engine
 
This works as expected under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5. Marking as verified for Firefox 35.

> To verify in Firefox 36 or later requires an addition to step 2 (set the
> browser.search.geoip.url pref to be 'data:application/json,{"country_code":
> "US"}'), and probably requires fixing bug 1123974 first, so we can hold off
> on that.
Verified as fixed using steps from comment 23 on Firefox 36 RC and Aurora 37.0a2 under Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OSX 10.9.5.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.