1053921 - breakdown: mechanism for shipping search engine additions off-cycle

Status: RESOLVED INVALID

(Firefox :: Search, defect)

Description

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

I'd like the ability to start shipping a new search engine as an option in the dropdown in the middle of a cycle (i.e. without respinning a build with different defaults).

This would likely be accomplished with a hotfix add-on. The requirements are:

- on day X, make search engine Foo available to all Firefox release users
- we need to be able to filter by build locale (hotfix system allows this)
- for Firefox builds produced after day X, we would ship the engine as a normal default engine. We need to ensure that the existing "temporary" engines do not conflict with the new default engine.
- on upgrade to a build that ships that engine by default, we want to ideally maintain the users customized order/alias for engine Foo

Comment 1

[Mark Hammond [:markh] [:mhammond]]

My investigation tells me:

* It would be non-trivial for a hotfix to install a new search provider as a "builtin" one - on Windows at least, it will require a UAC prompt to obtain permission to modify the Fx install directory.  This means it would be far simpler and a better UX to install the provider as though the user manually chose to install it.

* A search provider installed in the profile directory effectively "overrides" a provider with the same title installed with the app.  So when a new builtin provider is installed the search service effectively ignores it.  This is good - it means the "user" engine's order, default status and keyword are all maintained.

* 2 (relatively minor) flies in the ointment relate to removal:

** If the user removes the engine *before* the new release comes out, that new release will re-add the engine back.  I'm not sure if this is considered a problem (nor how it would best be fixed if it was).

** If the user removes the engine *after* the new release, the "user" version of the engine is removed, but the builtin engine is not marked as hidden.  Due to the implementation, the engine will *not* appear in the list until the engine cache is rebuilt (ie, probably after the *next* version is released).  IOW, the engine will *appear* to have been correctly removed, but is likely to magically spring back to life later.

The mitigation for the latter problem could be to change the engine removal code, such that if a "user" engine if removed, it also looks for a builtin engine of the same name and removes (actually hides) that too.  Or just ignore it :)

So I see 2 bugs:

* Create the hotfix with the relevant locale checks.  The real payload of this addon is simply:

  let url = /* eg */ "https://addons.cdn.mozilla.net/user-media/addons/161972/duckduckgo_https_ssl-20140120.xml";
  let dataType = Ci.nsISearchEngine.DATA_XML;
  Services.search.addEngine(url, dataType, null, callback);

but with the caveat this might fail due to network issues - so inside the callback we'd check for success and have the hotfix uninstall itself.  We should also verify no engine with that name is already installed.  I'd guess this at being p=13 for someone not experienced with hotfixes and taking into account the testing and overseeing the deployment.

* Change nsSearchService.js's removeEngine code to also look for a builtin provider of the same name and also hide that.  I'd guess this at p=5

I'll liaise with Gavin tomorrow about when to create these specific bugs.

Comment 2

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

(In reply to Mark Hammond [:markh] from comment #1)
> ** If the user removes the engine *before* the new release comes out, that
> new release will re-add the engine back.  I'm not sure if this is considered
> a problem (nor how it would best be fixed if it was).

I think we can live with this. Few people will remove the engine, since managing engines is relatively uncommon. Those that do will know how to re-remove it.

> ** If the user removes the engine *after* the new release, the "user"
> version of the engine is removed, but the builtin engine is not marked as
> hidden.  Due to the implementation, the engine will *not* appear in the list
> until the engine cache is rebuilt (ie, probably after the *next* version is
> released).  IOW, the engine will *appear* to have been correctly removed,
> but is likely to magically spring back to life later.

We could mitigate this another way: shortly after the release that includes the engine, ship another hotfix (targeted at only that release) that removes the user-version (and ports over any customizations if needed). I suppose it would need to deal with that cache rebuild bug - maybe we could mitigate that in-product ahead of time. This complicates things significantly and introduces a lot of potential for error, though...

> So I see 2 bugs:
> 
> * Create the hotfix with the relevant locale checks.  The real payload of
> this addon is simply:
> 
>   let url = /* eg */
>   let dataType = Ci.nsISearchEngine.DATA_XML;
>   Services.search.addEngine(url, dataType, null, callback);

We probably want to actually ship the engine definition in the hotfix, to avoid another download being necessary and to make sure it matches what we later plan to ship as-is.

Something to keep in mind here: user-installed search engines are not able to use MozParams, which might constrain us re: what type of "dynamic" parameters we can use.

> * Change nsSearchService.js's removeEngine code to also look for a builtin
> provider of the same name and also hide that.  I'd guess this at p=5

I'm not sure this is worth it, need to think about this more.

Comment 3

[Justin Dolske [:Dolske]]

One case I just chatted with Mark about (and, I guess, is kind of alluded to in comment 0) is doing "something" in the release where a new engine is included by default to normalize things.

For example, if the hotfix adds waldo.xml to a user's profile, it would seem wise to remove it when stock Firefox includes waldo.xml OOTB. Partially just for good housekeeping, but also so that (1) we don't accidentally footgun ourselves 5 years down the road when everyone forgets we did this and so (2) if we need to update/remove a search provider, we don't need to fiddle with stuff in the profile dir.

We don't need to do so immediately, but should at least have a plan for doing so. (EG, we need to be user we can differentiate a user who had previously installed a search engine vs a user who only got it from our hotfix.) That may be as simple as ensuring the hotfix's .xml file has a sufficiently unique name, eg waldosearch-firefox-hotfix-omg-whee.xml instead of waldo.xml.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #2)
> (In reply to Mark Hammond [:markh] from comment #1)
> > ** If the user removes the engine *before* the new release comes out, that
> > new release will re-add the engine back.  I'm not sure if this is considered
> > a problem (nor how it would best be fixed if it was).
> 
> I think we can live with this. Few people will remove the engine, since
> managing engines is relatively uncommon. Those that do will know how to
> re-remove it.
> 
> > ** If the user removes the engine *after* the new release, the "user"
...

> We could mitigate this another way: ... This complicates things significantly and
> introduces a lot of potential for error, though...

Yeah - given this is really just another flavor of the first point, maybe we can live with this too?

> We probably want to actually ship the engine definition in the hotfix, to
> avoid another download being necessary and to make sure it matches what we
> later plan to ship as-is.

I think that's doable - the addon can call _loadEnginesFromDir() - even though that's "private" I can't see anything that will prevent us calling that.

> Something to keep in mind here: user-installed search engines are not able
> to use MozParams, which might constrain us re: what type of "dynamic"
> parameters we can use.

This might be doable too by "tricking" the existing code - and it will also dovetail into what Dolske says in comment 3.

The MozParam check is at http://hg.mozilla.org/mozilla-central/file/5dc522d36d59/toolkit/components/search/nsSearchService.js#l1730 - which allows any engines for which _isDefault is true - and _isDefault is just !_isInProfile.  _isInProfile is pretty dumb - it just checks if the engine is *exactly* in the profile's "searchplugins" directory.  It looks as though it might be possible to add the new engine to, eg, a sub-directory in searchplugins.  This should trick the code into thinking it is a default *and* give us a sentinal we can use to know when we should remove the addon engine as mentioned in comment 3.

> > * Change nsSearchService.js's removeEngine code to also look for a builtin
> > provider of the same name and also hide that.  I'd guess this at p=5
> 
> I'm not sure this is worth it, need to think about this more.

Fair enough - but given comment 3, it sounds like it might make sense to change the engine manager to check for a "hotfix" engine when it rebuilds the engine cache.  ie, as it enumerates the app directory, it could check this new magic (say) "searchplugins/hotfix" subdir for an engine of the same name, migrate the user settings accordingly then delete the file (and directory if possible) from the profile dir.

Gavin, do you want these sub-bugs opened now?  I'm not sure if we should open these bugs with a specific search-provider specified...