1405670 - Stop importing old search settings from search-metadata.json

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Florian Quèze [:florian]]

search-metadata.json files were created by Firefox up to 44. Bug 1203167 changed the storage format for 45. We are still attempting to import search settings from this old file, which we could stop doing.

2 ESR cycles (45 and 52) happened since that. And the few users upgrading from a 44 or older to a current release will be forced to go through 48 anyway (https://chuttenblog.wordpress.com/2017/02/14/the-most-satisfying-graph/).

Comment 1

[Florian Quèze [:florian]]

Attachment: part 1 - stop importing old search plugins from <profile>/searchplugins/*.xml

Comment 2

[Florian Quèze [:florian]]

Attachment: part 2 - stop saving the modification date of directories

This could be folded into part 1, but part 1 was already big enough.

Comment 3

[Florian Quèze [:florian]]

Attachment: part 3 - stop reading from search-metadata.json

Comment 4

[Florian Quèze [:florian]]

Attachment: part 4 - remove support for NS_APP_SEARCH_DIR_LIST and NS_APP_SEARCH_DIR from the directory service

Comment 5

[Florian Quèze [:florian]]

This breaks 25 xpcshell tests of the search service; these tests rely on dropping a test engine in the profile folder before initializing the search service.

Comment 6

[Mike Kaply [:mkaply]]

> This breaks 25 xpcshell tests of the search service; these tests rely on dropping a test engine in the profile folder before initializing the search service.

What is the plan for that?

Comment 7

[Florian Quèze [:florian]]

(In reply to Mike Kaply [:mkaply] from comment #6)
> > This breaks 25 xpcshell tests of the search service; these tests rely on dropping a test engine in the profile folder before initializing the search service.
> 
> What is the plan for that?

Assuming the other patches I attached here are wanted, I'll need to fixup the tests. Likely use addEngineWithDetails instead.

Comment 8

[Florian Quèze [:florian]]

Attachment: part 5 - fix tests

This fixes the otherwise still relevant tests that were relying on:
- us importing automatically engines from .xml files dropped in the searchplugins/ folder of the profile.
- directory service constants I'm removing.

Comment 9

[Florian Quèze [:florian]]

Attachment: part 6 - remove tests for search-metadata.json migration

These tests are obsolete. I also removed lots of attempts to remove the cache file at the very beginning or very end of an xpcshell test. This was just doing a lot of pointless main thread I/O while running the tests. Each xpcshell test starts with a new process and profile folder anyway.

Comment 10

[Florian Quèze [:florian]]

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

Comment 11

[Drew Willcoxon :adw]

This looks straightforward to me -- shutting off avenues for installing/using engines that we don't want to be available anymore -- but I'm not super familiar with this code.  Most of this is code removal, which makes sense, and I don't see any problems in any of the other changes, not even any nits.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74f28655378e
stop importing old search plugins from <profile>/searchplugins/*.xml when the cache file is missing, r=adw.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3beeb64fb0da
stop saving the last modification date of directories we loaded engines from, r=adw.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05f84b0effd
stop importing old search metadata from search-metadata.json, r=adw.
https://hg.mozilla.org/integration/mozilla-inbound/rev/297b54c04fb1
remove support for NS_APP_SEARCH_DIR_LIST and NS_APP_SEARCH_DIR from the directory service, r=adw.
https://hg.mozilla.org/integration/mozilla-inbound/rev/12152163c058
fix search service tests relying on dropping .xml files in the profile folder, r=adw.
https://hg.mozilla.org/integration/mozilla-inbound/rev/160a2d1e4540
remove tests for search-metadata.json migration, r=adw.

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/74f28655378e
https://hg.mozilla.org/mozilla-central/rev/3beeb64fb0da
https://hg.mozilla.org/mozilla-central/rev/e05f84b0effd
https://hg.mozilla.org/mozilla-central/rev/297b54c04fb1
https://hg.mozilla.org/mozilla-central/rev/12152163c058
https://hg.mozilla.org/mozilla-central/rev/160a2d1e4540

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8928529 [details] [diff] [review]
part 1 - stop importing old search plugins from <profile>/searchplugins/*.xml

Approval Request Comment
[Feature/Bug causing the regression]: not a regression, this is long overdue follow-up cleanup after the changes from bug 1203167. But this is more relevant now that we dropped legacy add-ons in 57, as we want to avoid carrying forward customizations that were done in the pre-WebExtension world.
[User impact if declined]: Easier to get search plugins injected against the user's will by dropping files in the user profile.
[Is this code covered by automated tests?]: Yes. But it's mostly a code removal, so also removing tests.
[Has the fix been verified in Nightly?]: It has baked on Nightly for a couple days without us hearing complaints, but I don't think QA verified.
[Needs manual test from QE? If yes, steps to reproduce]: currently being discussed with QA directly
[List of other uplifts needed for the feature/fix]: most other patches in this bug. I would like to let "part 4" ride the trains without uplifting it though, to avoid needlessly breaking eg. Thunderbird.
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: it's a code removal.
[String changes made/needed]: none.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8928529 [details] [diff] [review]
part 1 - stop importing old search plugins from <profile>/searchplugins/*.xml

Let's take that.
Sheriff, please don't land part 4.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0abc56d5a1b1
https://hg.mozilla.org/releases/mozilla-beta/rev/fe0befa1aa9d
https://hg.mozilla.org/releases/mozilla-beta/rev/6ad58e06da8c
https://hg.mozilla.org/releases/mozilla-beta/rev/e4307e3d7f1f
https://hg.mozilla.org/releases/mozilla-beta/rev/1922cbee6cf4
https://hg.mozilla.org/releases/mozilla-beta/rev/827c49cd8d40

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/17e3d0eb9cdf
https://hg.mozilla.org/releases/mozilla-release/rev/1f25e81939ea
https://hg.mozilla.org/releases/mozilla-release/rev/037f3ba3a7b6
https://hg.mozilla.org/releases/mozilla-release/rev/12f0d3bf63bd
https://hg.mozilla.org/releases/mozilla-release/rev/61902e3ade32
https://hg.mozilla.org/releases/mozilla-release/rev/7ea28f7a42fe

Comment 18

[Adrian Florinescu [:aflorinescu]]

Basic QA was indirectly covered on bug 1419941 for the reset search testing.
Marking this issue as a qe- after consulting with :florian about what extra QA should be done for this issue in particular.