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

VERIFIED FIXED in Firefox 45

Status

()

Firefox
Search
P1
normal
Rank:
10
VERIFIED FIXED
2 years ago
5 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on: 1 bug, {dev-doc-complete, feature})

Trunk
Firefox 45
dev-doc-complete, feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 verified)

Details

(Whiteboard: [hijacking][fxsearch])

Attachments

(15 attachments, 3 obsolete attachments)

14.70 KB, patch
adw
: review+
Details | Diff | Splinter Review
5.24 KB, patch
adw
: review+
Details | Diff | Splinter Review
30.80 KB, patch
adw
: review+
Details | Diff | Splinter Review
9.19 KB, patch
adw
: review+
Details | Diff | Splinter Review
18.06 KB, patch
adw
: review+
Details | Diff | Splinter Review
33.17 KB, patch
adw
: review+
Details | Diff | Splinter Review
7.43 KB, patch
adw
: review+
Details | Diff | Splinter Review
3.63 KB, patch
adw
: review+
Details | Diff | Splinter Review
70.56 KB, patch
adw
: review+
Details | Diff | Splinter Review
17.32 KB, patch
adw
: review+
Details | Diff | Splinter Review
26.20 KB, patch
adw
: review+
Details | Diff | Splinter Review
3.60 KB, patch
adw
: review+
Details | Diff | Splinter Review
9.25 KB, patch
adw
: review+
Details | Diff | Splinter Review
6.20 KB, patch
adw
: review+
Details | Diff | Splinter Review
22.14 KB, patch
adw
: review+
Details | Diff | Splinter Review
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...).
Blocks: 1203168

Updated

2 years ago
Rank: 10
Priority: -- → P1
Created attachment 8672648 [details] [diff] [review]
Part 1 - stop serializing user-installed engines to XML files
Attachment #8672648 - Flags: review?(adw)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attachment #8672648 - Attachment description: stop serializing user-installed engines to XML files → Part 1 - stop serializing user-installed engines to XML files
Created attachment 8672649 [details] [diff] [review]
Part 2 - Keep user-installed engines when refreshing an outdated cache
Attachment #8672649 - Flags: review?(adw)
Created attachment 8672650 [details] [diff] [review]
Part 3 - Make the engine shortName independent of actual files
Attachment #8672650 - Flags: review?(adw)
Created attachment 8672651 [details] [diff] [review]
Part 4 - Make the lastModifiedTime optional in cache entries, but preserve it when it exists
Attachment #8672651 - Flags: review?(adw)
Created attachment 8672652 [details] [diff] [review]
Part 5 - Stop checking the file modification times of default engines at startup
Attachment #8672652 - Flags: review?(adw)
Created attachment 8672653 [details] [diff] [review]
Part 6 - Stop relying on _file or _uri being set
Attachment #8672653 - Flags: review?(adw)
Created attachment 8672654 [details] [diff] [review]
Part 7 - Remove legacy .xml file from the profile when a user-installed engine is removed
Attachment #8672654 - Flags: review?(adw)
Created attachment 8672655 [details] [diff] [review]
Part 8 - Fix the loadPath for engines installed from the web or add-ons
Attachment #8672655 - Flags: review?(adw)
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

2 years ago
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.
Attachment #8672648 - Flags: review?(adw) → review+
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.
Created attachment 8674890 [details] [diff] [review]
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).
Attachment #8674890 - Flags: review?(adw)

Comment 13

2 years ago
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?
Attachment #8672649 - Flags: review?(adw) → review+

Comment 14

2 years ago
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.
Attachment #8672650 - Flags: review?(adw) → review+

Updated

2 years ago
Attachment #8672651 - Flags: review?(adw) → review+

Updated

2 years ago
Attachment #8672652 - Flags: review?(adw) → review+

Updated

2 years ago
Attachment #8672653 - Flags: review?(adw) → review+

Updated

2 years ago
Attachment #8672654 - Flags: review?(adw) → review+

Updated

2 years ago
Attachment #8672655 - Flags: review?(adw) → review+

Updated

2 years ago
Attachment #8674890 - Flags: review?(adw) → review+
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 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.
Created attachment 8676681 [details] [diff] [review]
Part 10 - migrate metadata from search-metadata.json
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.
Attachment #8676681 - Attachment description: migrate metadata from search-metadata.json, → Part 10 - migrate metadata from search-metadata.json
Attachment #8676681 - Flags: review?(adw)
Attachment #8674890 - Attachment description: Part 9 - WIP store metadata in the main JSON file and remove the engine metadata service, → Part 9 - store metadata in the main JSON file and remove the engine metadata service,
(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.
Created attachment 8676686 [details] [diff] [review]
Part 11 - remove the 'directories' object from the structure of the cache file
Attachment #8676686 - Flags: review?(adw)
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.
Attachment #8676753 - Flags: review?(adw)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51ce0a224002
Created attachment 8677468 [details] [diff] [review]
Part 12 - clean-up handling of verification hashes, and hash the loadpath of user-installed engines
Attachment #8677468 - Flags: review?(adw)
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+.
Attachment #8677470 - Flags: review?(adw)
Created attachment 8677473 [details] [diff] [review]
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.
Attachment #8677473 - Flags: review?(adw)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f039a2474fc

Comment 27

2 years ago
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?
Attachment #8676681 - Flags: review?(adw) → review+

Updated

2 years ago
Attachment #8676686 - Flags: review?(adw) → review+

Updated

2 years ago
Attachment #8676753 - Flags: review?(adw) → review+

Updated

2 years ago
Attachment #8677468 - Flags: review?(adw) → review+

Updated

2 years ago
Attachment #8677470 - Flags: review?(adw) → review+

Comment 28

2 years ago
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.
Attachment #8677473 - Flags: review?(adw) → review+
Created attachment 8678167 [details] [diff] [review]
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.
Attachment #8678167 - Flags: feedback?(dteller)
(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.
(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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7bf7f79d335
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.
Attachment #8678167 - Flags: feedback?(dteller)
Created attachment 8679542 [details] [diff] [review]
Part 13 - compress the cache file, v2
Attachment #8679542 - Flags: review?(adw)
Attachment #8679542 - Flags: feedback?(dteller)
Attachment #8678167 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9ee05593f13
(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.
Depends on: 1218882
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 :)
Attachment #8679542 - Flags: feedback?(dteller) → feedback+
Created attachment 8679953 [details] [diff] [review]
Part 13 - compress the cache file, v3

Addressed comment 37.
Attachment #8679953 - Flags: review?(adw)
Attachment #8679542 - Attachment is obsolete: true
Attachment #8679542 - Flags: review?(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

It looks like compressing the file is now reasonably possible, so Part 13 Option 1 is obsolete.
Attachment #8677473 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8679953 - Flags: review?(adw) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b8e6f3f52b2
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

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Depends on: 1220246

Comment 43

2 years ago
bugherderuplift
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
status-b2g-v2.5: --- → fixed

Updated

2 years ago
Depends on: 1221513
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: --- → ---
Keywords: feature
Florian, do you think we should relnote that? Merci!
status-firefox43: affected → ---
relnote-firefox: --- → ?
Flags: needinfo?(florian)
Depends on: 1224150
(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
Flags: needinfo?(florian) → needinfo?(kev)

Comment 47

2 years ago
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.
Flags: needinfo?(kev) → needinfo?(eshepherd)
Depends on: 1226608
No longer depends on: 1226608
OK, no release notes, thanks
relnote-firefox: ? → ---
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.
Status: RESOLVED → VERIFIED
status-firefox45: fixed → verified

Updated

2 years ago
Depends on: 1236117

Updated

2 years ago
Depends on: 1236498
Depends on: 1254694

Updated

2 years ago
Depends on: 1255605

Comment 50

a year ago
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
(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

a year ago
> 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.
This has now been noted on Firefox 45 for developers, here: https://developer.mozilla.org/en-US/Firefox/Releases/45#Search_plugins
Keywords: dev-doc-complete

Updated

a year ago
Depends on: 1274545
Dropping obsolete NI that was completed in comment 53.
Flags: needinfo?(eshepherd)
You need to log in before you can comment on or make changes to this bug.