Closed Bug 1610870 Opened 5 years ago Closed 4 years ago

If a Search Engine WebExtension is updated, the search service should apply the new engine

Categories

(Firefox :: Search, task, P3)

Product:
Firefox
Component:
Search
Type:
task
Points:
3

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1631898

People

(Reporter: standard8, Unassigned)

References

Details

If we get a search engine update and it is flagged as a system add-on, then we should ensure that if the engine is installed, it is updated correctly and the new settings applied.

I just had a look at how this currently works for normal WebExtensions with Search Engines. It appears the add-on manager currently uninstalls and then reinstalls the Search Engine.

That's a little concerning for maintaining default engines, though at the moment it works, because we don't reset the default when we remove the engine. That is something we'd like to change at some stage (bug 1575649), so we'll need to be careful unless we can think of a way around this.

For now, we probably don't need to do much here once the support for updates is in add-on manager/Normandy. means we won't need to do anything extra, though we should still add a test for it.

To clarify slight, what I think we should look at here. Once the Normandy/add-on manager work is done, we should test updating of built-in add-ons and look at the effects for:

  • Do all the parameters get updated correctly?
  • Are the user's preferences kept correctly, e.g. does it stay hidden if already hidden, does it stay in the right order?

We should manually test these, and of course make sure we have tests for these as well.

Depends on: 1575948

Mark, what's the connection to bug 1575948? It looks like bug 1571876 would be a more appropriate dependency?

(In reply to Andrew Swan [:aswan] from comment #3)

Mark, what's the connection to bug 1575948? It looks like bug 1571876 would be a more appropriate dependency?

This is the bug that Rehan told us the work to do what we need is being implemented in.

Rehan, what's the connection between this bug and 1575948?

Flags: needinfo?(rdalal)

Updates will be delivered via Normandy Addon Rollouts which are blocked by bug 1575948

Flags: needinfo?(rdalal)

The I don't understand the bug. If a search engine exists as a built-in and then Normandy delivers an update (either the way it works today or post-bug 1575948) then the addon manager handles that as an upgrade, not as an uninstall-then-install. There's the issue of what happens when the browser itself is updated and there's a newer builtin version, but I think that's bug 1613647?

We dont currently handle runtime upgrades of builtIn engines in the SearchService, we use the data from the cache and sans cache the data the addon manager gives us on startup, that means if we get a runtime update of a builtIn search extension from Normandy, it will be ignored, then on startup we will use the previous version from the cache, its not until the cache is invalidated (firefox upgrades) that we are sure to see the upgraded data.

(https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm#2511)

(In reply to Dale Harvey (:daleharvey) from comment #8)

We dont currently handle runtime upgrades of builtIn engines in the SearchService, we use the data from the cache and sans cache the data the addon manager gives us on startup, that means if we get a runtime update of a builtIn search extension from Normandy, it will be ignored, then on startup we will use the previous version from the cache, its not until the cache is invalidated (firefox upgrades) that we are sure to see the upgraded data.

(https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm#2511)

Sorry to keep going here but I still don't understand this bug. The code linked above references the .builtIn property of addonData which is only true for addons that are loaded from resource: urls (as opposed to from xpi files on disk). Updates delivered by Normandy will of course be xpi files (either in the profile/extensions directory or some new directory to be created in bug 1575948) but installing those addons won't follow the code path linked in comment 8.

If the upgrade installed via Normandy does not have the .builtIn property, then on upgrade as far as I know the addon manager will try to remove the original engine [1] and add a new one, since the original engine does have the builtIn property it wont get removed but hidden [2] then the later attempt to install will fail [3]

https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-chrome-settings-overrides.js#462
https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm#2815
https://searchfox.org/mozilla-central/source/toolkit/components/search/SearchService.jsm#2480

I am also not confident how the SearchService will handle the AddonManager reporting engines that are not .builtIn but are in the builtInEngines list

Once Normandy updates are implemented, we need to test updating a SearchEngine and fix any of the things that break which is what this bug is for, although I expect there is some work we can do to improve the situation before the Normandy updates actually lands

If you manually install an update to a builtin engine, what happens? Fixing that (if it doesn't work) is probably a step forward on this.

Thanks for the clarifications. To be pedantic, this is an existing bug that could be triggered, as Shane says, by a user manually installing a search engine that is also built-in. It doesn't strictly depend on bug 1575948 but delivering updates via Normandy will trigger this bug. Sounds like the direction of the dependency should be reversed? (i.e., we can't deliver updates via Normandy unless/until this bug is fixed)

(In reply to Andrew Swan [:aswan] from comment #12)

Thanks for the clarifications. To be pedantic, this is an existing bug that could be triggered, as Shane says, by a user manually installing a search engine that is also built-in.

You can simulate some of the functionality like that, but not all. We have yet been told for definite what flags we'll be getting on the updates received from Normandy to distinguish between user-installed vs Normandy, and it is a requirement that we can do this.

It doesn't strictly depend on bug 1575948 but delivering updates via Normandy will trigger this bug. Sounds like the direction of the dependency should be reversed? (i.e., we can't deliver updates via Normandy unless/until this bug is fixed).

You can deliver updates via Normandy as long as they don't contain search engines.

We cannot close this bug until we know exactly what flags we're getting from Normandy and the add-on manager, we've implemented it, and we've tested the full system.

We are planning on doing this in the same cycle (76) as the Normandy/Add-on manager code are planned to be implemented (assuming it doesn't come in at the last moment).

(In reply to Mark Banner (:standard8) from comment #13)

It doesn't strictly depend on bug 1575948 but delivering updates via Normandy will trigger this bug. Sounds like the direction of the dependency should be reversed? (i.e., we can't deliver updates via Normandy unless/until this bug is fixed).

You can deliver updates via Normandy as long as they don't contain search engines.

Of course, that remark was in the context of this bug, referring to updates of search engines.

We cannot close this bug until we know exactly what flags we're getting from Normandy and the add-on manager, we've implemented it, and we've tested the full system.

Who said anything about closing the bug?

Anyway, I came across this after being asked for feedback on bug 1575948. I'm happy to try to give feedback but this whole effort is frustrating since goals/requirements/plans are not clearly laid out. If you all want meaningful feedback, the clearer that these things are described, the better. And from my past experience when I was a Mozilla employee, I think that's especially important when trying to coordinate a project like this that falls at the intersection of 3 different groups (addons, normandy, search) with different expertise.

Hi Andrew! So you're right that we haven't provided all the necessary details for you to get the full picture, but please be assured that we do have those! It might be the case that there's some details that are not written down, but everything we do have I'd be more than happy to share!
And everything that we're trying to get done here is in heavy, explicit collaboration with the Addons, Normandy and Search teams.

Perhaps a small part at play here is that we need to get used to you not being an employee anymore, but still an active contributor? Which is truly awesome! But we really missed you at the last All Hands in Berlin 😢

I'll share the stuff that I can find & remember atm.

No longer blocks: search-modernization
Blocks: 1635249

The search service work for this is already being handled in bug 1631898, based on what we've been told. Bug 1635249 will track any remaining work we find we need to do later.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.