1054960 - Fix sync to handle the current search engine being stored outside of preferences

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect, P1)

Description

[Florian Quèze [:florian]]

Bug 1029148 moved the storage of the current search engine outside of preferences to make hijacking search a bit more difficult. Without changes to sync, the current search engine won't be sync'ed anymore.

Comment 1

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

Let's try to get this in Firefox 34.

Comment 2

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

We're not going to track this (perhaps WONTFIX).

Comment 3

[Chris Karlof [:ckarlof]]

Scope of the work for 43.1 is to better understand scope of the problem.

Comment 4

[Edouard Oger [:eoger]]

Attachment: bug-1054960.patch

I implemented a draft patch by creating a "fake" pref inside the preferences engine.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8647745 [details] [diff] [review]
bug-1054960.patch

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

::: services/sync/modules/engines/prefs.js
@@ +263,5 @@
> +          notifyPrefChange();
> +        }
> +        break;
> +      case "browser-search-engine-modified":
> +        if (data === "engine-default") {

Using the 'engine-default' notification (rather than 'engine-current') here seems a bit inconsistent with the rest of this code using Services.search.currentEngine.

On Firefox where currentEngine and defaultEngine should always be the same value, this shouldn't have any impact, but the currentEngine == defaultEngine thing may not be true for other consumers of the search service.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8647745 [details] [diff] [review]
bug-1054960.patch

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

I'm not super keen on the approach. This relies on all clients having this code. The ones that don't -- Firefoxes older than when this lands -- won't update the fake pref correctly.

That, and because pref sync doesn't merge, means this will result in user-facing weirdness.

For example, if you have a third-party search engine installed on one machine, it's going to get lost as soon as one of your other devices wins a pref fight (which happens all the time): the other device won't be able to select the communicated non-default engine, so it'll stick to its current one; next time it uploads, the first machine will take the default engine selection. The support questions will be: "why does my Firefox keep forgetting my search engine?".

Rather than working around that by tracking per-device search engine selections, fallbacks, etc. etc., I suggest simply wontfixing this bug. I don't think syncing the selected search engine makes much sense without first syncing the search engines themselves.

::: services/sync/modules/engines/prefs.js
@@ +127,3 @@
>  
>        try {
>          this._prefs.set(pref, value);

I believe this still runs even for your fake pref, so you'll add a `default-search-engine` pref.

Comment 7

[Chris Karlof [:ckarlof]]

Let's back burner this then. Unassigning you eoger.

Comment 8

[Prashant Savaj]

How to sync search engines and their keywords. Engines, keywords and in Same Sequence order.

Comment 9

[Thom Chiovoloni [:tcsc] (ex-moco)]

Florian: We'd like to implement search engine syncing -- that is, we'd like to sync all your search settings, including custom engines, etc. AFAICT there seem to be two ways this could be done:

1. Grab enough information about every search engine so that we can call addEngineWithDetails, as well as current engine, engine keywords, etc
2. Sync the data currently stored in the search cache (e.g. the search.json.mozlz4 file), more-or-less "as is".

Hopefully these would have the same result. The first seems easier to implement, but the 2nd sounds more error prone. Let me know if these are obviously flawed approaches.

The biggest issue from the sync side seem to be that every engine storing its icon as a base64-encoded image, which is the largest part of the search engine data by a wide margin, and it would be really nice not to store tons of these given that most of them are going to be the same. (Moreover it would be nice to sync all the changes as a single record which is the way that the current pref sync works -- but this would require we keep it under 256k at the moment, which might be impractical, given the icons taking up upwards of 10k).

Also, the issue of users with profiles in multiple locales sounds tricky (I just happened across bug 1305073, which says engines exist is somewhat locale specific). I'm not sure how big of an issue this would be in practice, however.

Anyway, before I get started on one of the above approaches, can you let me know if one (or both) seem obviously flawed? Or if some other method seems obviously better to you?

Comment 10

[Florian Quèze [:florian]]

(In reply to Thom Chiovoloni [:tcsc] from comment #9)
> Florian: We'd like to implement search engine syncing -- that is, we'd like
> to sync all your search settings, including custom engines, etc. AFAICT
> there seem to be two ways this could be done:
> 
> 1. Grab enough information about every search engine so that we can call
> addEngineWithDetails, as well as current engine, engine keywords, etc

This seems the most reasonable solution, as it would rely only exposed API that are hopefully all covered by automated tests.

We'll need to be careful when syncing the current default, as we should only sync it if we are confident it's been set as a result of user action; if there's a suspicion of hijacking, we shouldn't sync the change.
I can help you figure out that part when you get there.

> 2. Sync the data currently stored in the search cache (e.g. the
> search.json.mozlz4 file), more-or-less "as is".

This seems very fragile, and this file is a 'cache', it contains a lot of data that's read from files built into the browser, so a lot of data not worth syncing.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Thom Chiovoloni [:tcsc] from comment #9)
> Florian: We'd like to implement search engine syncing -- that is, we'd like
> to sync all your search settings, including custom engines, etc. 

Bug 444284?

Comment 12

[Richard Newman [:rnewman]]

Indeed, if syncing the actual search engine data is what you plan to do, I suggest taking the work to that bug and WONTFIXing this, per Comment 6.

Comment 13

[Thom Chiovoloni [:tcsc] (ex-moco)]

Ah, alright. Well, we'd be syncing the current search engine as well so I'm not sure that WONTFIX is *quite* accurate, but I'll take your word for it that that's the right resolution. (ISTM that marking as dupe of 444284 would be more accurate)