1203167 - Store user-installed search plugins in a JSON file and stop loading [profile]/searchplugins/*.xml

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Florian Quèze [:florian]]

Once the search cache is no longer optional (bug 1203161), we could use the cache file as the place to store user-installed engines.

We may also want to merge the search-metadata.json and search.json files if that simplifies the code. The file where user engines are stored should have a verification hash, to prevent tampering. There are already several things in search-metadata.json that have such verification hashes; maybe it's time to unify this, and checksum the whole file at once.

Having the user-installed engines stored in a JSON file should let us easily add metadata related to these search plugins, and we should store information about how the engine was installed (eg. website hostname; conversion from an existing .xml file found in the profile while migrating, etc...).

Comment 1

[Florian Quèze [:florian]]

Attachment: Part 1 - stop serializing user-installed engines to XML files

Comment 2

[Florian Quèze [:florian]]

Attachment: Part 2 - Keep user-installed engines when refreshing an outdated cache

Comment 3

[Florian Quèze [:florian]]

Attachment: Part 3 - Make the engine shortName independent of actual files

Comment 4

[Florian Quèze [:florian]]

Attachment: Part 4 - Make the lastModifiedTime optional in cache entries, but preserve it when it exists

Comment 5

[Florian Quèze [:florian]]

Attachment: Part 5 - Stop checking the file modification times of default engines at startup

Comment 6

[Florian Quèze [:florian]]

Attachment: Part 6 - Stop relying on _file or _uri being set

Comment 7

[Florian Quèze [:florian]]

Attachment: Part 7 - Remove legacy .xml file from the profile when a user-installed engine is removed

Comment 8

[Florian Quèze [:florian]]

Attachment: Part 8 - Fix the loadPath for engines installed from the web or add-ons

Comment 9

[Florian Quèze [:florian]]

This patch queue isn't finished yet, I intend to add 2-3 more patches (I need to at least change the search.json file name so that a user downgrading wouldn't lose all the engines installed by the newer version). But I'm attaching what I have now so that I can start getting feedback. With the 8 patches attached here, I'm done refactoring the search service to not rely on having a file for each engine. Tests pass locally.

I'm now going to look at what to do with search-metadata.json.

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8672648 [details] [diff] [review]
Part 1 - stop serializing user-installed engines to XML files

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

Thanks for breaking up these patches, makes it easier to review.  I probably won't be able to review everything in one shot but the plan sounds OK to me.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8672652 [details] [diff] [review]
Part 5 - Stop checking the file modification times of default engines at startup

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

::: toolkit/components/search/nsSearchService.js
@@ +2978,2 @@
>      if (cache.directories) {
> +      cacheOtherPathsPaths = [path for (path in cache.directories)]

Changed to 'cacheOtherPaths' (ie. removed the duplicated 'Paths') locally already.

Comment 12

[Florian Quèze [:florian]]

Attachment: Part 9 - store metadata in the main JSON file and remove the engine metadata service,

This moves metadata storage from search-metadata.json to search.json, as it made no sense to have 2 separate JSON files written to the profile by the search service.
This patch is WIP because I still need to handle importing existing metadata from the legacy search-metadata.json for users upgrading.

After finishing this patch, I expect 2 more patches:
- one to cleanup the way we handle verification hashes, and potentially store verified data about how engines were installed.
- one to change the file name of search.json so that it doesn't conflict with the old cache file for upgrading users.

I've also noticed that the 'part 3' patch broke the browser/components/search/test/browser_contextmenu.js test (I have been testing and fixing the xpcshell tests while coding, but I paid less attention to the browser chrome tests).

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8672649 [details] [diff] [review]
Part 2 - Keep user-installed engines when refreshing an outdated cache

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

::: toolkit/components/search/nsSearchService.js
@@ +3328,5 @@
>          engineUpdateService.scheduleNextUpdate(aEngine);
>      }
>    },
>  
> +  _loadEnginesFromCache: function SRCH_SVC__loadEnginesFromCache(cache,

Nit: I don't think there's any point in keeping this ugly function naming convention anymore, given how the JS engine now nicely identifies anonymous functions.  Here and elsewhere.  I'd write this like:

_loadEnginesFromCache(cache, skipReadOnly) {

@@ +3333,5 @@
> +                                                                 skipReadOnly) {
> +    if (!cache.directories)
> +      return;
> +
> +    for each (let dir in cache.directories) {

`for each` is deprecated: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for_each...in

Can't you use for...in/of (not sure which you need here) instead?

Comment 14

[Drew Willcoxon :adw]

Comment on attachment 8672650 [details] [diff] [review]
Part 3 - Make the engine shortName independent of actual files

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

::: toolkit/components/search/nsSearchService.js
@@ +2995,5 @@
>      let buildID = Services.appinfo.platformBuildID;
> +    let cachePaths = [];
> +    if (cache.directories) {
> +      cachePaths = [path for (path in cache.directories)]
> +                     .filter(p => p != "[user-installed]");

This string should probably be in a const at the top of the file since it's used in more than one place.

Comment 15

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

Comment on attachment 8672654 [details] [diff] [review]
Part 7 - Remove legacy .xml file from the profile when a user-installed engine is removed

Would this be a good opportunity to transition this to an async OS.File operation, rather than using nsIFile?

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8672651 [details] [diff] [review]
Part 4 - Make the lastModifiedTime optional in cache entries, but preserve it when it exists

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

::: toolkit/components/search/nsSearchService.js
@@ +3435,2 @@
>            yield checkForSyncCompletion(addedEngine._asyncInitFromFile());
> +          if (!isInProfile && addedEngine._isDefault) {

A '!' is missing here before "addedEngine._isDefault" (the code currently in the patch is inconsistent with the sync version).

I'm fixing this.

Comment 17

[Florian Quèze [:florian]]

Attachment: Part 10 - migrate metadata from search-metadata.json

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8676681 [details] [diff] [review]
Part 10 - migrate metadata from search-metadata.json

Part 9 was large enough already, I handled the migration of legacy metadata as a separate patch.

Comment 19

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #13)

> > +  _loadEnginesFromCache: function SRCH_SVC__loadEnginesFromCache(cache,
> 
> Nit: I don't think there's any point in keeping this ugly function naming
> convention anymore, given how the JS engine now nicely identifies anonymous
> functions.  Here and elsewhere.  I'd write this like:

In general I agree. In the specific case of the toJSON and _initWithJSON methods that exist in both the Engine and the EngineURL objects, I like having the function names to help me understand immediately which code I'm looking at. I could be convinced that this specific case could be addressed with a code comment though.
This suggested change is rather large and would touch the whole file, so I would rather do it either as a separate attachment, or preferably leave it for a follow-up bug.

> @@ +3333,5 @@
> > +                                                                 skipReadOnly) {
> > +    if (!cache.directories)
> > +      return;
> > +
> > +    for each (let dir in cache.directories) {
> 
> `for each` is deprecated:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/
> for_each...in
> 
> Can't you use for...in/of (not sure which you need here) instead?

Neither for..in nor for..of works here, because this is a plain object and Symbol.iterator isn't implemented for them.

For the specific case you pointed, I'm getting rid of the cache.directories object, so the for each naturally disappears.

We still have lots of for each (including some that are added by my patches) used on this._engines, which is also a plain object. The good solution would be to change this._engines from being a plain object to being a Map, but again I would rather do that in a follow-up, as the scope of the changes made in this bug is already larger than I would like.

(In reply to Drew Willcoxon :adw from comment #14)
> Comment on attachment 8672650 [details] [diff] [review]
> Part 3 - Make the engine shortName independent of actual files
> 
> Review of attachment 8672650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/search/nsSearchService.js
> @@ +2995,5 @@
> >      let buildID = Services.appinfo.platformBuildID;
> > +    let cachePaths = [];
> > +    if (cache.directories) {
> > +      cachePaths = [path for (path in cache.directories)]
> > +                     .filter(p => p != "[user-installed]");
> 
> This string should probably be in a const at the top of the file since it's
> used in more than one place.

This is actually a remnant from the old structure of the cache file. Now that we no longer rely on poking so many files (except for extension-shipped engines), I think we should just flatten a bit more the cache file. Patch coming.

Comment 20

[Florian Quèze [:florian]]

Attachment: Part 11 - remove the 'directories' object from the structure of the cache file

Comment 21

[Florian Quèze [:florian]]

Attachment: Fix browser_contextmenu.js

Attached separately for easier review, but I intend to fold this patch into 'part 3' once it's reviewed.

Comment 22

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51ce0a224002

Comment 23

[Florian Quèze [:florian]]

Attachment: Part 12 - clean-up handling of verification hashes, and hash the loadpath of user-installed engines

Comment 24

[Florian Quèze [:florian]]

Attachment: fix Places and Telemetry xpcshell tests, fix browser_aboutHome.js bc test

I may fold these various changes into the relevant previous patches once this is r+.

Comment 25

[Florian Quèze [:florian]]

Attachment: Part 13 Option 1 - rename search.json to search-settings.json to avoid losing data when downgrading

To avoid losing data when a user downgrades to a version of Firefox that blows away the cache file without preserving user data from it, we need to change the name of the file. This patch is just a straightforward rename of the file to search-settings.json. In an alternative patch, I'm going to try to compress the file using OS.File and see where I get.

Comment 26

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f039a2474fc

Comment 27

[Drew Willcoxon :adw]

Comment on attachment 8676681 [details] [diff] [review]
Part 10 - migrate metadata from search-metadata.json

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

Could you pretty-print metadata.json?

Comment 28

[Drew Willcoxon :adw]

Comment on attachment 8677473 [details] [diff] [review]
Part 13 Option 1 - rename search.json to search-settings.json to avoid losing data when downgrading

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

Sounds fine to me.

Comment 29

[Florian Quèze [:florian]]

Attachment: Part 13 Option 2 - compress the cache file using OS.File

This is an alternative to the previous patch. It saves the cache file to search.json.mozlz4 and uses OS.File's compression mechanism. One downside is that in the (hopefully very rare) case of a synchronous initialization of the search service being forced early at startup before we have had an opportunity to read the file, we have to spin a nested event loop. I added a telemetry probe to collect data about how often that happens.

Comment 30

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #21)
> Created attachment 8676753 [details] [diff] [review]
> Fix browser_contextmenu.js
> 
> Attached separately for easier review, but I intend to fold this patch into
> 'part 3' once it's reviewed.

This doesn't fold cleanly into part3, as the test fix depends on things that are introduced in part6. I'm folding the test fix into part6 instead, and I'll have to accept that there's one bc test that's broken when the patch queue is applied up to part3, 4 or 5.

Comment 31

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> Created attachment 8677470 [details] [diff] [review]
> fix Places and Telemetry xpcshell tests, fix browser_aboutHome.js bc test
> 
> I may fold these various changes into the relevant previous patches once
> this is r+.

Folded this into part 3 for the browser_aboutHome.js fix, into part 8 for the duplicated 'let packageName' fix, and part 6 for everything else.

Comment 32

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7bf7f79d335

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8678167 [details] [diff] [review]
Part 13 Option 2 - compress the cache file using OS.File

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

The nested event loop is a bit too footgunish for my tastes.
If you really need the code to be sync, what about making module workerlz4 compatible with the main thread? This would take a little boilerplate in toolkit/components/workerlz4 (and possibly renaming the directory :) ) but it would be less risky.

Comment 34

[Florian Quèze [:florian]]

Attachment: Part 13 - compress the cache file, v2

Comment 35

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9ee05593f13

Comment 36

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #34)
> Created attachment 8679542 [details] [diff] [review]
> Part 13 - compress the cache file, v2

This new patch depends on the change in bug 1218882.

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8679542 [details] [diff] [review]
Part 13 - compress the cache file, v2

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

f=me

::: toolkit/components/search/nsSearchService.js
@@ +2708,5 @@
>      }
>      this._addObservers();
>  
>      gInitialized = true;
> +    this._cacheFileJSON = null;

Could you document this field?

@@ +3120,5 @@
>        }
>      }.bind(this));
>    },
>  
>    _readCacheFile: function SRCH_SVC__readCacheFile() {

Could you take the opportunity to document this method? Little by little, we can hope to make this module vaguely readable :)

Comment 38

[Florian Quèze [:florian]]

Attachment: Part 13 - compress the cache file, v3

Addressed comment 37.

Comment 39

[Florian Quèze [:florian]]

Comment on attachment 8677473 [details] [diff] [review]
Part 13 Option 1 - rename search.json to search-settings.json to avoid losing data when downgrading

It looks like compressing the file is now reasonably possible, so Part 13 Option 1 is obsolete.

Comment 40

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b8e6f3f52b2

Comment 41

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/5fd12ba6de56890528cc921b2c448812e123dd6c
Bug 1203167 - stop serializing user-installed engines to XML files, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/c31604db0e39b41dcc1298a32e22a583287c919d
Bug 1203167 - Keep user-installed engines when refreshing an outdated cache, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/2bc25894ecedcf4621ca605400e58a00e182f74e
Bug 1203167 - Make the engine shortName independent of actual files, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/2b024785855c6b10ee928b244204932d05c2a11f
Bug 1203167 - Make the lastModifiedTime optional in cache entries, but preserve it when it exists, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/bf09271c6adbb1280042c8b2fa4e7bb60ab948d0
Bug 1203167 - Stop checking the file modification times of default engines at startup, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/fb5b5f5334bfebed3531b37164b2f8b26472e96f
Bug 1203167 - Stop relying on _file or _uri being set, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/5fcbbf605e9fb5c40e7a1c901d81fc0c08461744
Bug 1203167 - Remove legacy .xml file from the profile when a user-installed engine is removed, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/8a60f27fe0749bdd4e40276f0acfdc1ea0775ce0
Bug 1203167 - Fix the loadPath for engines installed from the web or add-ons, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/bba9634ab696d49e53269b4d1a9b95a014e3a6d3
Bug 1203167 - store metadata in the main JSON file and remove the engine metadata service, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/322fda6e323684202ab0a5141e1eeabf2fe1eea7
Bug 1203167 - migrate metadata from search-metadata.json, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/360838236c537e34c84a7446e47c8c44f08571dc
Bug 1203167 - remove the 'directories' object from the structure of the cache file, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/048b6ce05bfd313b731018f90abd4e0f000606f2
Bug 1203167 - clean-up handling of verification hashes, and hash the loadpath of user-installed engines, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/2d9c58098eb3fd5d79a05ccea2c1457dd4b2ec25
Bug 1203167 - compress the cache file, r=adw.

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5fd12ba6de56
https://hg.mozilla.org/mozilla-central/rev/c31604db0e39
https://hg.mozilla.org/mozilla-central/rev/2bc25894eced
https://hg.mozilla.org/mozilla-central/rev/2b024785855c
https://hg.mozilla.org/mozilla-central/rev/bf09271c6adb
https://hg.mozilla.org/mozilla-central/rev/fb5b5f5334bf
https://hg.mozilla.org/mozilla-central/rev/5fcbbf605e9f
https://hg.mozilla.org/mozilla-central/rev/8a60f27fe074
https://hg.mozilla.org/mozilla-central/rev/bba9634ab696
https://hg.mozilla.org/mozilla-central/rev/322fda6e3236
https://hg.mozilla.org/mozilla-central/rev/360838236c53
https://hg.mozilla.org/mozilla-central/rev/048b6ce05bfd
https://hg.mozilla.org/mozilla-central/rev/2d9c58098eb3

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5fd12ba6de56
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c31604db0e39
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2bc25894eced
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2b024785855c
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bf09271c6adb
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fb5b5f5334bf
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5fcbbf605e9f
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8a60f27fe074
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bba9634ab696
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/322fda6e3236
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/360838236c53
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/048b6ce05bfd
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2d9c58098eb3

Comment 44

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Florian, do you think we should relnote that? Merci!

Comment 46

[Florian Quèze [:florian]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #45)
> Florian, do you think we should relnote that? Merci!

This is more a question for kev (do we want any attention on this, or do we prefer this to be quiet?). I'm leaning toward answering 'no', but if you do want to relnote this, there's a blog post explaining the changes here: http://blog.queze.net/post/2015/11/02/What-about-search-hijacking

Comment 47

[Kev Needham [:kev]]

I don't think it needs to be in the relnotes, because it's something that pretty much no one should run into from a user side of things. It's probably something that should go into the MDN firefox-for-developers post for the release, though. Need-infoing sheppy so MDN team is aware.

Comment 48

[Sylvestre Ledru [:Sylvestre]]

OK, no release notes, thanks

Comment 49

[Brindusa Tot, Desktop QA]

Verified as fixed using the latest Nightly 45 on: Windows 7, Windows 8, Windows 8.1, Windows 10, Windows XP, Windows Vista, Ubuntu 14.04, Ubuntu 15.04, Mac OS x 10.10, Mac OS x 10.11.

Comment 50

[bugzilla1]

In the spirit of tinkering:

Since the search plug-ins are now stored in an encrypted file, please suggest how a "power user" can do the following:
* Add another search plugin manually (not via web sites).
* Modify an existing installed search plugin.

Thank you!

PS,
It would be very disappointing if Mozilla is embracing the concept of a walled garden in the name of security

Comment 51

[Florian Quèze [:florian]]

(In reply to bugzilla1 from comment #50)
> In the spirit of tinkering:
> 
> Since the search plug-ins are now stored in an encrypted file,

The file is compressed, not encrypted.

> please
> suggest how a "power user" can do the following:
> * Add another search plugin manually (not via web sites).

Services.search.addEngine("file:///Users/.../file.xml", null, null, false); from the browser console works for me (I gave more detailed steps in bug 1236498 comment 7).

> * Modify an existing installed search plugin.

This isn't currently possible, see bug 1195005.

Comment 52

[bugzilla1]

> Services.search.addEngine("file:///Users/.../file.xml", null, null, false); from the browser console works for me (I gave more detailed steps in bug 1236498 comment 7).

This is hardly user friendly, or even documented.

> This isn't currently possible, see bug 1195005.

That's a problem.  If I cannot edit an existing search engine, and I also cannot export it to an XML file for editing and re-importing, then my options are too limited.

Comment 53

[Eric Shepherd [:sheppy]]

This has now been noted on Firefox 45 for developers, here: https://developer.mozilla.org/en-US/Firefox/Releases/45#Search_plugins

Comment 54

[Mark Banner (:standard8)]

Dropping obsolete NI that was completed in comment 53.