Stop importing old search settings from search-metadata.json

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: florian, Assigned: florian)

Tracking

Trunk
Firefox 59
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57+ fixed, firefox58 fixed, firefox59 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
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/).
(Assignee)

Updated

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

Comment 2

a year ago
This could be folded into part 1, but part 1 was already big enough.
(Assignee)

Comment 5

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

Comment 7

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

Comment 8

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

Comment 9

a year ago
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.
Attachment #8928529 - Flags: review?(adw)
Attachment #8928530 - Flags: review?(adw)
Attachment #8928531 - Flags: review?(adw)
Attachment #8928532 - Flags: review?(adw)
Attachment #8929068 - Flags: review?(adw)
Attachment #8929069 - Flags: review?(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.
Attachment #8928529 - Flags: review?(adw) → review+
Attachment #8928530 - Flags: review?(adw) → review+
Attachment #8928531 - Flags: review?(adw) → review+
Attachment #8928532 - Flags: review?(adw) → review+
Attachment #8929068 - Flags: review?(adw) → review+
Attachment #8929069 - Flags: review?(adw) → review+

Comment 12

a year ago
Pushed by florian@queze.net:
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.
(Assignee)

Updated

a year ago
Depends on: 1418953
(Assignee)

Comment 14

a year ago
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.
Attachment #8928529 - Flags: approval-mozilla-beta?
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.
Attachment #8928529 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8928530 - Flags: approval-mozilla-release+
Attachment #8928530 - Flags: approval-mozilla-beta+
Attachment #8928531 - Flags: approval-mozilla-release+
Attachment #8928531 - Flags: approval-mozilla-beta+
Attachment #8929068 - Flags: approval-mozilla-release+
Attachment #8929068 - Flags: approval-mozilla-beta+
Attachment #8929069 - Flags: approval-mozilla-release+
Attachment #8929069 - Flags: approval-mozilla-beta+
Attachment #8928529 - Flags: approval-mozilla-release+
Priority: -- → P1
Whiteboard: [fxsearch]
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.
Flags: qe-verify-
(Assignee)

Updated

a year ago
Depends on: 1424343
You need to log in before you can comment on or make changes to this bug.