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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

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.

Comment 1

[:Margaret Leibovic]

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?

Comment 2

[:Margaret Leibovic]

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.

Comment 3

[Aaron Train [:aaronmt]]

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.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

REOPEN if someone else sees this

Comment 6

[Aaron Train [:aaronmt]]

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.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Does "Reset Defaults" in the Settings > Customize > Search reset to a know state? Do situations where this happens also involve custom search engines?

Comment 8

[Marcia Knous [:marcia]]

(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.

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

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}}

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Address bar search UI

Comment 11

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Setting search UI

Comment 12

[Kevin Brosnan [Ex-Mozilla]]

Can reproduce without installing the search engine step. Note each time I installed Firefox it was launched and I performed a search.

Comment 13

[Mike Connor [:mconnor]]

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.

Comment 14

[Aaron Train [:aaronmt]]

Needs an assignee for 36.

Comment 15

[: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.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 17

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1073775/margaret

/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

Comment 18

[:Margaret Leibovic]

(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.

Comment 19

[:Margaret Leibovic]

(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.

Comment 20

[Chenxia Liu [:liuche]]

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.

Comment 21

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/0f792fdfedd6

I can request uplift once QA verifies this.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f792fdfedd6

Comment 23

[Sylvestre Ledru [:Sylvestre]]

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

Comment 24

[: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

Comment 25

[Sylvestre Ledru [:Sylvestre]]

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

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-release/rev/784a4c059370
https://hg.mozilla.org/releases/mozilla-beta/rev/21ec3b2d3da5

mozilla-aurora is currently closed. I'll try to get it soon.

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Aurora opened up:
https://hg.mozilla.org/releases/mozilla-aurora/rev/57f387aaa54b

Comment 28

[Flaviu Cos]

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).

Comment 29

[Flaviu Cos]

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?

Comment 30

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/4203/#review3615

Ship It!

Comment 31

[Chenxia Liu [:liuche]]

Comment on attachment 8568195 [details]
MozReview Request: bz://1073775/margaret

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

Ship It!

Comment 33

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1073775 - Pass default engine from JS to Java, instead of making assumptions based on engine list order. r=liuche