1628016 - Rethink provider concurrency in the Urlbar

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P3)

Description

[Harry Twyford]

Suggested by Marco here.

we should rethink the whole concurrency handling so that we have a different provider instance for each input, or use the input name as key in this.queries, rather than the queryContext.

This would make it safer to keep member variables across queries, such as UrlbarProviderSearchSuggestions._lastLowResultsSearchSuggestion.

Comment 1

[Marco Bonardo [:mak] (Away 10-21 August)]

Pretty much we don't have a very good way of keeping query state for a specific input now.

Comment 2

[Marco Bonardo [:mak] (Away 10-21 August)]

I started on this and then the patch began to grow, so I must split changes into parts, a few cleanups can land sooner.

There's a few things to consider. The simplest approach has 1 manager per input, and one provider instance per manager. So pretty much everything becomes per-window, things that should be global can be stored in each module's global scope and accessed through static methods on the class.
Though, when a provider is registered with a manager, it should be registered for all the same-id inputs, not just for that window. That likely means I'll have to keep registered providers per input id in the manager's module scope.

An alternative would be to have everything per input id, but then we need Maps to keep and retrieve those objects, I suspect it would become even more complex. Or there may be a manager per input id, but then we'd need a manager of managers, it sounds like a complication again.

For now I'm sticking to the per-window + global scopes approach. It seems to work, but I didn't go through all consumers yet. XPCShell tests will have to create their own manager, since there's no real input field.

Comment 3

[Marco Bonardo [:mak] (Away 10-21 August)]

Small update: I'm discussing this with Drew, we're debating the approach, whether to go for a per-input, per-input-per-window or per-query approach and how to handle runtime added (extension) providers in those cases.
In the meanwhile, I'm landing some cleanups for providers boilerplate to potentially simplify the above concurrency change.

Comment 4

[Marco Bonardo [:mak] (Away 10-21 August)]

we'll go for instance-per-input-type. This is pretty much the de-facto situation today, because we have only one consumer. The upside of this approach thus is simplicity. The downside is there's no sandboxing of state across windows.

The alternative approach instance-per-input would be a natural evolution of this, providing also cross-window sandboxing of state, but it's more complex requiring the manager to be per window, and a lot more compex handling of the WebExtensions side, indeed the WebExtension scope is global and we should still track per-window state in it.
Currently we don't have use-cases for the more complex approach, thus it's not worth the cost.

Comment 5

[Marco Bonardo [:mak] (Away 10-21 August)]

Attachment: WIP: Bug 1628016 - Make urlbar providers instanced per input type.

Comment 6

[Marco Bonardo [:mak] (Away 10-21 August)]

I'm not actively working on this, but posted my very old and likely bitrotted patch as inspiration for future work. It could probably be broken down further.
The idea is having one provider manager per input field id, so all the "urlbar" fields will share the same manager. The manager will own provider instances and track static data that those instances want to persist. No provider should use their "global" to store static data unless it's valid for the whole process (and thus all the input fields can use that data, regardless what they are).

Comment 7

[Marco Bonardo [:mak] (Away 10-21 August)]

Reposting my comment to Marc for future reference:

The overall scope, long term, is to have the urlbar components/modules usable in more than one field. Currently a lot of the code is a singleton and it only works with the single urlbar field.
I started some time ago to do some of the work in bug 1628016 (ad you can see from some of the fixed dependencies) that consisted mostly in making providers instantiatable, unifying some of the code, making some methods static...
Then I started looking into the one-instance-per-input-field problem, with the WIP patch here that I didn't finish.
The idea was to have the providersManager own the providers instances, so each providersManager has its own providers instances. Then I wanted each input field have its own providersManager...
But there was a problem there, the urlbars in different windows often want to share some states.
So I ended up thinking to have one providerManager per input type, where by input type I mean "urlbar", "searchbar", "virtual-reality bar" and so on... so the urlbar in different windows share the same providers manager, the legacy search bars in different windows share their own manager.
My WIP here was an initial attempt but it's likely unclean, and there may be a better way to do that.
What you want is probably a providersManager getter in UrlbarInput that gets the right instance depending on the input DOM node id. Since js modules are unique per process, you could have a providersManagers' Manager (sorry for the ugly name) that has an id -> providersManager instance Map.
Then it's pretty much matter of ensuring each provider doesn't store state in its global scope but rather keeps it in their instance.

Comment 8

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 9

[Moritz Beier [:mbeier]]

Attachment: Bug 1628016 - Part 1: Allow multiple instances of ProvidersManager and Providers. r?dao

Comment 10

[Moritz Beier [:mbeier]]

Attachment: Bug 1628016 - Part 2: Allow more than one instance of urlbar providers. r?dao

This bulk of this commit is about making those members of urlbar providers static that should be shared among instances.

Comment 11

[Moritz Beier [:mbeier]]

Attachment: Bug 1628016 - Part 3: Fix quicksuggest tests. r?adw

Part 2 changed UrlbarProviderQuickSuggest to check whether the SearchSuggestions provider is really active in the current query. If it's not active, the suggested index of all quick suggestions will be -1. In tests, it's almost never active, so this patch fixes those tests.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/75d61c0ae20a
https://hg.mozilla.org/integration/autoland/rev/4d7fafefbd96
Part 1: Allow multiple instances of ProvidersManager and Providers. r=dao,urlbar-reviewers
https://github.com/mozilla-firefox/firefox/commit/46983e5c5d4c
https://hg.mozilla.org/integration/autoland/rev/22bc7737007d
Part 2: Allow more than one instance of urlbar providers. r=dao,urlbar-reviewers
https://github.com/mozilla-firefox/firefox/commit/1cec84461790
https://hg.mozilla.org/integration/autoland/rev/24f9492bb601
Part 3: Fix quicksuggest tests. r=adw,urlbar-reviewers

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4d7fafefbd96
https://hg.mozilla.org/mozilla-central/rev/22bc7737007d
https://hg.mozilla.org/mozilla-central/rev/24f9492bb601