885351 - Google is always used in about:home and the URL bar when setting the default search engine to an invalid one

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Ioana (away)]

Reproducible on:
Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130620 Firefox/23.0
Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20130620 Firefox/24.0

STR:
1. Launch Firefox.
2. Change the search engine in the search bar to anything except Google.
3. Restart Firefox (this step is only necessary on Fx23, not on 24).
4. Go to about:config and change browser.search.defaultenginename to a random string.
5. Restart Firefox.

Expected Results:
The default search engine stays the same in the search bar, URL bar and about:home - the engine selected at step 2.

Actual Results:
The search bar keeps the last selected engine (step 2), while about:home and the URL bar use Google.

Comment 1

[u279076]

Thinking about this bug, should this not be the expected case? Should we not fall back to the default in case of failure?

Comment 2

[Ioana (away)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #1)
> Thinking about this bug, should this not be the expected case? Should we not
> fall back to the default in case of failure?

It would. When I selected an engine at step 2, that engine was marked as default per bug 738818. The more concerning part for me is having different engines in different places (see  comment 0 Actual Results) though.

Comment 3

[u279076]

Gavin, what do you think?

Comment 4

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

Yes, definitely a valid bug. If an addon messes with your search prefs you shouldn't be able to get into this state.

Comment 5

[u279076]

Given this relates back to bug 738818 which has been rel-noted for Firefox 23 I'm nominating this for tracking.

Comment 6

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

I'm going back and forth on whether this is worth fixing "soon", it may be complicated.

Comment 7

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

Attachment: patch

This should fix both this bug and bug 880882, by ensuring on every startup that defaultEngine == currentEngine. It does this by observing search service initialization, and syncing those properties immediately (the patch in bug 860560 ensures that they then remain in sync).

This has the potential to conflict with search add-ons that try to set defaultenginename on every startup, but that is somewhat incompatible with the design implemented in bug 738818 anyways.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 769806 [details] [diff] [review]
patch

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

::: browser/components/nsBrowserGlue.js
@@ +325,5 @@
>          // its currentEngine. The search service will notify us any time either
>          // of them are changed (either by directly setting the relevant prefs,
>          // i.e. if add-ons try to change this directly, or if the
>          // nsIBrowserSearchService setters are called).
> +        // No need to initialize the search service, since it's garanteed to be

Nit: s/garanteed/guaranteed/

@@ +422,5 @@
>      os.removeObserver(this, "keyword-search");
>  #endif
>      os.removeObserver(this, "browser-search-engine-modified");
> +    try {
> +      os.removeObserver(this, "browser-search-service");

We could try checking Services.search.isInitialized here instead of the try…catch but the try…catch is more defensive which is good.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f349618cab1

Comment 10

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

Comment on attachment 769806 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 738818
User impact if declined: existing users and users of add-ons may encounter discrepancies between search bar and other search forms, which goes against the design in bug 738818
Testing completed (on m-c, etc.): manually
Risk to taking this patch (and alternatives if risky): low-risk
String or IDL/UUID changes made by this patch: none

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f349618cab1

Comment 12

[u279076]

Ioana, can you please verify this is fixed in today's Nightly?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffa6dcc0a622
https://hg.mozilla.org/releases/mozilla-beta/rev/25ac2f74bbfe

Comment 14

[Ioana (away)]

Verified as fixed on Firefox 23.0b3 (07/03), 24.0a2 (07/04) and 25.0a1 (07/04).