If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED WONTFIX

Status

()

Firefox
Sync
P1
normal
Rank:
1
RESOLVED WONTFIX
3 years ago
a year ago

People

(Reporter: florian, Assigned: tcsc)

Tracking

(Blocks: 1 bug)

Trunk
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox33 unaffected, firefox34- affected)

Details

(Whiteboard: [fxsync])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
Flags: firefox-backlog+
(Reporter)

Updated

3 years ago
Blocks: 1029148
Let's try to get this in Firefox 34.
status-firefox33: --- → unaffected
status-firefox34: --- → affected
tracking-firefox34: --- → +

Updated

3 years ago
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
We're not going to track this (perhaps WONTFIX).
tracking-firefox34: + → -
Priority: -- → P2
See Also: → bug 444284
Priority: P2 → P1
Scope of the work for 43.1 is to better understand scope of the problem.
Iteration: --- → 43.1 - Aug 24

Updated

2 years ago
Whiteboard: [fxsync]
Created attachment 8647745 [details] [diff] [review]
bug-1054960.patch

I implemented a draft patch by creating a "fake" pref inside the preferences engine.
Assignee: nobody → edouard.oger
Status: NEW → ASSIGNED
Attachment #8647745 - Flags: review?(rnewman)

Updated

2 years ago
Attachment #8647745 - Flags: review?(rnewman) → feedback?(rnewman)
(Reporter)

Comment 5

2 years ago
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 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.
Attachment #8647745 - Flags: feedback?(rnewman) → feedback-
Let's back burner this then. Unassigning you eoger.
Assignee: edouard.oger → nobody
Status: ASSIGNED → NEW
Iteration: 43.1 - Aug 24 → ---
Priority: P1 → P2
Blocks: 1269548

Comment 8

a year ago
How to sync search engines and their keywords. Engines, keywords and in Same Sequence order.
Priority: P2 → P3
Priority: P3 → P2
Rank: 1
(Assignee)

Updated

a year ago
Assignee: nobody → tchiovoloni
Status: NEW → ASSIGNED
(Assignee)

Comment 9

a year ago
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?
Flags: needinfo?(florian)
(Reporter)

Comment 10

a year ago
(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.
Flags: needinfo?(florian)
(Assignee)

Updated

a year ago
Priority: P2 → P1
(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?
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.
(Assignee)

Comment 13

a year ago
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)
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.