Closed
Bug 1110420
Opened 9 years ago
Closed 9 years ago
getInUS should return false for custom builds (i.e. distribution.ini)
Categories
(Firefox :: Search, defect)
Firefox
Search
Tracking
()
People
(Reporter: mconnor, Assigned: Gavin)
References
Details
Attachments
(1 file)
1.20 KB,
patch
|
markh
:
review+
Gavin
:
approval-mozilla-aurora+
Gavin
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•9 years ago
|
status-firefox34:
--- → wontfix
status-firefox35:
--- → affected
status-firefox36:
--- → affected
tracking-firefox35:
--- → +
tracking-firefox36:
--- → +
OS: Windows 8.1 → All
Hardware: x86_64 → All
Assignee | ||
Updated•9 years ago
|
Flags: qe-verify+
Flags: firefox-backlog+
Assignee | ||
Updated•9 years ago
|
Points: --- → 2
Assignee | ||
Comment 1•9 years ago
|
||
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)
Updated•9 years ago
|
status-firefox37:
--- → affected
tracking-firefox37:
--- → +
Updated•9 years ago
|
Attachment #8541703 -
Flags: review?(mhammond)
Attachment #8541703 -
Flags: review?(dolske)
Attachment #8541703 -
Flags: review+
Assignee | ||
Comment 2•9 years ago
|
||
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?
Assignee | ||
Comment 3•9 years ago
|
||
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+
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/163080209ca9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/7023ec60f4fb
Updated•9 years ago
|
Iteration: --- → 37.3
Updated•9 years ago
|
QA Contact: petruta.rasa
Comment 8•9 years ago
|
||
Are there some steps and/or results I could use in order to verify this? Thanks!
Comment 9•9 years ago
|
||
(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)
Assignee | ||
Comment 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
(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)
Comment 13•9 years ago
|
||
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)
Assignee | ||
Comment 14•9 years ago
|
||
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)
Comment 15•9 years ago
|
||
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)
Comment 16•9 years ago
|
||
Gavin, what do you suggest regarding the above comments?
Flags: needinfo?(gavin.sharp)
Assignee | ||
Comment 17•9 years ago
|
||
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)
Comment 18•9 years ago
|
||
Built from https://hg.mozilla.org/releases/mozilla-beta/rev/1b26127c3323 Built from https://hg.mozilla.org/releases/mozilla-release/rev/32e36869f84a
Assignee | ||
Comment 19•9 years ago
|
||
(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)
Assignee | ||
Comment 20•9 years ago
|
||
(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)
Comment 21•9 years ago
|
||
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.
Comment 22•9 years ago
|
||
(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)
Assignee | ||
Comment 23•9 years ago
|
||
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.
Comment 24•9 years ago
|
||
(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.
Comment 25•9 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•