Closed Bug 1073775 Opened 5 years ago Closed 5 years ago

Yahoo is default search engine, but search preferences say Google is my default

Categories

(Firefox for Android :: General, defect)

35 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox36 + fixed
firefox37 + verified
firefox38 + fixed
firefox39 --- fixed
fennec + ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

(Keywords: reproducible, verifyme)

Attachments

(3 files, 1 obsolete file)

I went into my settings to change my default engine from Yahoo, but when I opened settings, it said that Google was currently my default. I'll copy my profile now so I can look to see what's going on here.
Keywords: steps-wanted
This is the search-metadata.json I pulled from my phone:

{
    "[profile]/bugzilla-main-page.xml": {
        "alias": null,
        "order": 5
    },
    "[app]/google.xml": {
        "order": 1
    },
    "[app]/amazondotcom.xml": {
        "order": 3
    },
    "[app]/twitter.xml": {
        "order": 2
    },
    "[app]/wikipedia.xml": {
        "order": 4
    },
    "[app]/bing.xml": {
        "order": 6
    },
    "[app]/yahoo.xml": {
        "order": 6
    }
}

It looks like the Yahoo order got screwed up. Maybe we there's a bug updating the default order? 

Gavin, can you think of anything in the search service that could have caused us update problems when we changed the default order in region.properties?
Flags: needinfo?(gavin.sharp)
If I were to try to make STR, I would try installing a build that pre-dates bug 1071022, change the default search engine, then update to a build that does include bug 1071022. If that doesn't work, I would try installing an additional search engine before updating, since maybe that contributed to the problem.
Blocks: 1071022
As suggested in comment #2

* Set a different default on an older build, performed an upgrade to latest Nightly and my manually set default was retained.

* Set a different default on an older build, installed a new engine as well. On upgrade my manually set default was retained.
The default order is only ever read once, and then copied to search-metadata.json, so changing it shouldn't impact existing users (see bug 1073603).

I'm not sure how comment 1 happened. The code that saves the order saves it for all engines at once:
http://hg.mozilla.org/mozilla-central/annotate/66ab05e2a667/toolkit/components/search/nsSearchService.js#l3697

I suppose if for some reason some of the _setAttr calls fail you could get into trouble, or maybe if you are adding and removing default engines some stale entries could hang around in search-metadata.json and throw things off.
Flags: needinfo?(gavin.sharp)
REOPEN if someone else sees this
Status: NEW → RESOLVED
tracking-fennec: ? → 35+
Closed: 5 years ago
Resolution: --- → WORKSFORME
Seems to be the case for some based on internal survey. In all instances, the user has been updating Beta so at some point with the search changes, the defaults listed in settings do not match the actual default.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
tracking-fennec: 35+ → ?
Does "Reset Defaults" in the Settings > Customize > Search reset to a know state? Do situations where this happens also involve custom search engines?
tracking-fennec: ? → +
(In reply to Mark Finkle (:mfinkle) from comment #7)
> Does "Reset Defaults" in the Settings > Customize > Search reset to a know
> state? Do situations where this happens also involve custom search engines?

I reset in Settings, and I also set Yahoo as the default and then set back to Google. In Beta now I am getting Google searches.

I did the same thing in Nightly and it seems to work there as well too.
adb install -r fennec-33.0b10.multi.android-arm.apk
6538 KB/s (35321516 bytes in 5.275s)
        pkg: /data/local/tmp/fennec-33.0b10.multi.android-arm.apk
Success

Installed a search plugin

adb install -r fennec-34.0b1.multi.android-arm.apk
7171 KB/s (35700379 bytes in 4.861s)
        pkg: /data/local/tmp/fennec-34.0b1.multi.android-arm.apk
Success

$ adb install -r fennec-34.0b10.multi.android-arm.apk
6729 KB/s (35794196 bytes in 5.194s)
        pkg: /data/local/tmp/fennec-34.0b10.multi.android-arm.apk
Success

$ adb install -r fennec-35.0b1.multi.android-arm.apk
6671 KB/s (36288137 bytes in 5.312s)
        pkg: /data/local/tmp/fennec-35.0b1.multi.android-arm.apk
Success

Now I am in a state where Yahoo is first but Amazon is the default according to the settings ui

contents of search-metadata.json look wrong

{"[app]/google.xml":{"order":1},"[app]/yahoo.xml":{"order":2},"[app]/bing.xml":{"order":3},"[app]/amazondotcom.xml":{"order":4},"[app]/twitter.xml":{"order":5},"[app]/wikipedia.xml":{"order":6},"[profile]/bugzillamozilla---bug.xml":{"order":7}}
Can reproduce without installing the search engine step. Note each time I installed Firefox it was launched and I performed a search.
I'm fairly confident this bug is a combination of two bugs.

The obvious one is that, in the preferences dialog, we assume the first engine in the engines list is the default engine.  http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/SearchPreferenceCategory.java#110

This is also true at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7033

If you've never installed/reordered/hidden engines or changed default, you're probably in the clear here.  We don't save your sorted list anywhere, so we fall back to the order prefs, and then we're in the clear.  However, if you've ever modified the list in any of those ways, we've started to save search-metadata.json to disk.

On desktop, this list is orthogonal to your current default, we never reordered the list.  This is why on pre-Fx34 profiles Google is still listed above Yahoo in the prefs on desktop.  If you disable the one-off buttons, you get the old order in the drop-down.  We didn't notice this because we changed UI at the same time as default.

On mobile, the assumption linked above means Google shows up as the default.  It should fix itself if you toggle to something non-Yahoo and then back.  Kevin is confirming this.

What this means is that this bug will not affect all users.  The only users who will have been affected are those who have installed, hidden, reordered, or changed default.  My gut is that's a small minority, but I could be wrong.

To fix this should be trivial. Instead of inferring the first engine in the list as default, we explicit pass the default to Java from browser.js, and then let the pref pane a) show the correct default and b) update the ordering as if the user chose that default.

Longer-term, we should just fork the search service into Java so we can share code with the search activity, and not depend on a desktop module with different UX and assumptions.
Needs an assignee for 36.
Assignee: nobody → margaret.leibovic
Thanks for the investigation, mconnor.

So it turns out that, yes, our Java code assumes that the first item the the engines array is always the default engine, and we do already correctly update the order when the user sets a new default engine through the settings UI:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7104

I found that I can fix this one of two ways:

1) We can pass along the default engine to put it at the top of the list, similarly to what we do for the awesome screen (we end up with the correct ordering there because we always put the "suggest" engine at the top, and the suggest engine is always the default).

2) We can just alter the array in JS to match what Java is expecting.

However, I found that with either of these solutions, our setting UI still has problems because it does not actually maintain an ordering that reflects the real ordering. We use Preference#setOrder to move around the order of the Preference items, but that just makes sure that the new default ends up at the top of the list, while the old default gets bumped below. I think what we actually need to do is reconstruct the list every time the default changes, similarly to what we do for the home panels preference.

Given that we're trying to get a fix in for 36, and we just want this UI to not be totally broken, I think we should split this ordering problem out into another bug, and just focus on making sure the correct default is selected in the UI here.
(In reply to :Margaret Leibovic from comment #15)

> Given that we're trying to get a fix in for 36, and we just want this UI to
> not be totally broken, I think we should split this ordering problem out
> into another bug, and just focus on making sure the correct default is
> selected in the UI here.

Agreed
/r/4203 - Bug 1073775 - Pass default engine from JS to Java, instead of making assumptions based on engine list order. r=liuche

Pull down this commit:

hg pull review -r 63d2c0c787bbb936823332f3ec347ceb4baf1e82
Attachment #8568195 - Flags: review?(liuche)
(In reply to :Margaret Leibovic from comment #15)

> I found that I can fix this one of two ways:
> 
> 1) We can pass along the default engine to put it at the top of the list,
> similarly to what we do for the awesome screen (we end up with the correct
> ordering there because we always put the "suggest" engine at the top, and
> the suggest engine is always the default).
> 
> 2) We can just alter the array in JS to match what Java is expecting.

I decided to go with option #2 here, because doing the simple thing for option #1 resulted in the default engine still being second in the list (although at least labeled as "default"). Fixing this would require futzing around with the Preference#setOrder logic, and that seems more regression prone.
(In reply to :Margaret Leibovic from comment #15)

> However, I found that with either of these solutions, our setting UI still
> has problems because it does not actually maintain an ordering that reflects
> the real ordering. We use Preference#setOrder to move around the order of
> the Preference items, but that just makes sure that the new default ends up
> at the top of the list, while the old default gets bumped below. I think
> what we actually need to do is reconstruct the list every time the default
> changes, similarly to what we do for the home panels preference.
> 
> Given that we're trying to get a fix in for 36, and we just want this UI to
> not be totally broken, I think we should split this ordering problem out
> into another bug, and just focus on making sure the correct default is
> selected in the UI here.

Bug 924461 would probably be the fix for this.
https://reviewboard.mozilla.org/r/4203/#review3345

Okay, that's a pretty straightforward fix to reorder the engines to match what we expect them to be. I think conceptually this is a better option than 1) because 1) opens up the situation of the default engine not being first in the list of search suggestions, which could be a little confusing if you just hit "enter" and see results from a lower search engine. That discussion should go into bug 924461 though.
Attachment #8568195 - Flags: review?(liuche) → review+
https://hg.mozilla.org/integration/fx-team/rev/0f792fdfedd6

I can request uplift once QA verifies this.
https://hg.mozilla.org/mozilla-central/rev/0f792fdfedd6
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Margaret, we discussed about this bug during the channel meeting with Kevin. We would like to test on the release branch. Could you fill the uplift request? Thanks
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8568195 [details]
MozReview Request: bz://1073775/margaret

Approval Request Comment
[Feature/regressing bug #]: bug 1071022 exposed this problem in our search settings code
[User impact if declined]: the search settings UI will show the wrong search engine selected as the default
[Describe test coverage new/current, TreeHerder]: no automated tests, but tested locally and landed on m-c
[Risks and why]: there is a small risk that this could cause issues with our search engine ordering, but it's the smallest patch possible to address this problem
[String/UUID change made/needed]: none
Flags: needinfo?(margaret.leibovic)
Attachment #8568195 - Flags: approval-mozilla-release?
Attachment #8568195 - Flags: approval-mozilla-beta?
Attachment #8568195 - Flags: approval-mozilla-aurora?
Comment on attachment 8568195 [details]
MozReview Request: bz://1073775/margaret

As this is the blocker for 36 mobile, taking it on all channels to get maximum coverage.
Thanks
Attachment #8568195 - Flags: approval-mozilla-release?
Attachment #8568195 - Flags: approval-mozilla-release+
Attachment #8568195 - Flags: approval-mozilla-beta?
Attachment #8568195 - Flags: approval-mozilla-beta+
Attachment #8568195 - Flags: approval-mozilla-aurora?
Attachment #8568195 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Verified as fixed by performing the following steps:
1. Install 33.0b10;
2. Install a search plugin (I installed AOL search from add-ons);
3. Install 34.0b1;
- At this step the Default Search engine is Yahoo but in search preferences say Google is default;
4. Install 37.0b1;
- At this step the Default Search engine is matching search preferences which is Google.

Tested on Nexus 7 (Android 5.0.2).
I did some further investigation and I realize that this is also fixed in 36.0b10.
After step 3 (comment 28) I installed build 36.0b10 and the issue does not reproduce;


As for the Firefox 36 release I can not reproduce the bug at all.
I installed multiple builds, installed search plugins, changed default search engines and still could not reproduce the bug.
Can you please provide some STR?
Flags: needinfo?(kbrosnan)
Comment on attachment 8568195 [details]
MozReview Request: bz://1073775/margaret

https://reviewboard.mozilla.org/r/4201/#review3617

Ship It!
Attachment #8568195 - Flags: review+
Attachment #8568195 - Attachment is obsolete: true
Attachment #8618380 - Flags: review+
Flags: needinfo?(kbrosnan)
You need to log in before you can comment on or make changes to this bug.