Make NS_URIChainHasFlags (mostly) threadsafe
Categories
(Core :: Networking, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox109 | --- | fixed |
People
(Reporter: nika, Assigned: nika)
References
Details
(Whiteboard: [necko-triaged])
Attachments
(7 files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
This would be useful as part of the work to make nsIPrincipal
threadsafe, as nsIPrincipal
uses protocol flags especially when constructing content principals, which is fairly core functionality.
Currently in order to look up the flags for a URI, we look the nsIProtocolHandler
up in the component manager, and then call either GetProtocolFlags
or nsIProtocolHandlerWithDynamicFlags::GetFlagsForURI
to determine the flags for that URI.
It's likely not practical for us to migrate all implementations of nsIProtocolHandler
to be threadsafe C++ components, as we have some JS-implemented interfaces, and Thunderbird/KaiOS have even more of them, so instead we'll likely want to somehow decouple the flag information from the actual nsIProtocolHandler
instance.
Assignee | ||
Comment 1•2 years ago
|
||
I've considered some of the various protocol handlers, and what we'll need to do in order to support their flags being available off-main-thread as a bit of a summary of what might be required for this bug:
Dynamic (nsIProtocolHandlerWithDynamicFlags
)
This interface is already [builtinclass]
and has 6 implementations. As it is dynamic, the easiest approach for these interfaces is probably to make the logic threadsafe.
BlobURLProtocolHandler
The protocol handler itself currently uses non-threadsafe refcounting, however the implementation appears to already be threadsafe, calling IsBlobURI(aURI)
, which already takes a lock under the hood.
Making the protocol handler threadsafe-refcounted for the GetFlagsForURI
method seems like it may be as simple as changing to NS_DECL_THREADSAFE_ISUPPORTS
. We could also extract the logic to a static method which doesn't depend on the instance fairly easily.
FontTableURIProtocolHandler
The nsIProtocolHandlerWithDynamicFlags
interface is not reported to NS_IMPL_ISUPPORTS
, and the implementation just forwards to GetProtocolFlags
, so it could probably be removed completely.
nsAboutProtocolHandler
This protocol handler depends on the URI flags from the nsIAboutModule
instance for the specific about page, specifically URI_SAFE_FOR_UNTRUSTED_CONTENT
(to enable URI_IS_POTENTIALLY_TRUSTWORTHY
) and MAKE_LINKABLE
(to enable URI_LOADABLE_BY_ANYONE
and disable URI_DANGEROUS_TO_LOAD
) (impl).
As with nsIProtocolHandler
, many implementations of nsIAboutModule
are in JS, and likely cannot be easily made threadsafe. Fortunately, it appears that every implementation of nsIAboutModule::GetURIFlags
only uses the URI to get the about-module (so that components like AboutRedirector
can share a single instance for multiple modules), so we may be able to to use a similar strategy to whatever we end up using for nsIProtocolHandler
for nsIAboutModule
as well, so that the flags can be looked up off-main-thread.
ExtensionProtocolHandler
Unfortunately, this protocol will likely be one of the more difficult ones to make threadsafe.
The initial lookup (EPS().GetByURL(url)
), could be made threadsafe by using a threadsafe subset of the WebExtensionPolicy
, and storing some information which is constant within that threadsafe subset, which would be held by the outer policy. We'll likely need to do that for BaseProtocol::AddonPolicy()
and its various callers as well.
The information required in this case is:
-
WebExtensionPolicy::Type()
- this could be stored on a threadsafe tearoff easily, as it is known at construction time, uses a threadsafe type, and doesn't change. -
WebExtensionPolicy::ManifestVersion()
- could also easily be moved to a threadsafe tearoff, as it is known at construction time. -
WebExtensionPolicy::PrivateBrowsingAllowed()
- would be somewhat annoying, but still possible, to move to a threadsafe tearoff. ThemPermissions
field is anAtomSet
, which is not threadsafe, and can be mutated, though both can be solved by changing types and adding a mutex to the threadsafe tearoff. -
WebExtensionPolicy::IsWebAccessiblePath(...)
- This will likely be the most difficult part to make threadsafe, as it is implemented using a cycle-collected type (WebAccessibleResource
), and contains other cc'd types which can contain JS regex objects. This entire type would need to be re-implemented using threadsafe types. Fortunately for us, it does appear that the wayWebAccessibleResource
is implemented is an implementation detail ofWebExtensionPolicy
, so it should be possible to do without changes to the WebExtension JS code.
Getting this protocol handler to only use threadsafe methods is probably one of the main problems we'll need to figure out a path forward for before we move forward with this bug.
PageThumbProtocolHandler
The implementation of GetFlagsForURI
for this interface always returns the same flags, so it could likely be removed by switching the SubstitutingProtocolHandler
constructor being called, and passing the flags in there.
Either way, the logic is trivially threadsafe.
nsViewSourceHandler
The implementation is not threadsafe currently only because it checks the protocol flags for its inner URI, so it will be threadsafe once we've made checking flags threadsafe.
Non-Dynamic (nsIProtocolHandler
)
In the non-dynamic case, every implementation of nsIProtocolHandler
just exposes a GetProtocolFlags
method (or a protocolFlags
field if implemented in JS), which is fetched to get the flags for the URI. There appear to be no implementations which do anything fancy, meaning that each protocol could have a constant associated with its component registration.
The one instance where a flag appears to be used in the implementation is for BaseWebSocketChannel
, however the mEncrypted
flag is only set at construction time depending on whether the channel was created for the ws:
or wss:
protocols, so should be fine.
Using the nsICategoryManager
As we already register these protocols through the component manager, and occasionally load dynamic protocols during tests, it may make sense for us to encode the flags in the category manager. Instead of using individual contractid
s for each protocol, we could consider instead using a network-protocol
category, where the names of each entry are the protocol, and the values are a comma-separated sequence of options - similar to what is done for the update-timer
category.
For example, the entry for https
could look like this (in manifest format):
category network-protocol https @mozilla.org/network/protocol;1?name=https,std,allows_proxy,allows_proxy_http,loadable_by_anyone,is_potentially_trustworthy
Though it would probably be written out in a static compoents.conf file instead. Perhaps with some sort of 'options'
flag support in gen_static_components.py
it could look like:
{
'cid': '{dccbe7e4-7750-466b-a557-5ea36c8ff24e}',
'contract_ids': ['@mozilla.org/network/protocol;1?name=https'],
'singleton': True,
'type': 'mozilla::net::nsHttpsHandler',
'processes': ProcessSelector.ALLOW_IN_SOCKET_PROCESS,
'categories': {
'network-protocol': {
'name': 'https',
'options': [
'std',
'allows_proxy',
'allows_proxy_http',
'loadable_by_anyone',
'is_potentially_trustworthy',
],
},
},
}
Enumerating components in the component manager is threadsafe, so accessing this information would be safe to do from any thread, and could be done as part of handler lookup. When registering a protocol dynamically from JS, the caller would need to also add an entry to the category manager for it, with the relevant information.
An alternative would also be to leave the contractID out of the category value, and instead declare the component directly like:
Categories = {
'network-protocol-flags': {
'https': 'std,allows_proxy,allows_proxy_http,loadable_by_anyone,is_potentially_trustworthy',
# etc...
},
}
With this system, the nsIProtocolHandler
would still be looked up by ContractID for creating a channel, but would be looked up by category for flags. As the category entry is bypassed in some cases, though, this runs the risk of someone forgetting to add or update the category entry when registering a dynamic handler.
If we take this approach we'll probably want to remove the protocolFlags
field from nsIProtocolHandler
to make it harder for them to get out-of-sync.
As the components.conf
file is a python script, there's also a final possibility which is that instead of using a string of flags, the flags are pre-computed in the Components.conf
, and the hex-encoding of the flag values is stored in the category manager instead, to be parsed at runtime. This may have slight performance advantages but reduced clarity and error checking.
Automatic Caching
It is technically possible to enumerate every ContractID
registered in the component manager (https://searchfox.org/mozilla-central/rev/12a34801fbaa6f14e20515a44b1fd77179b02617/xpcom/components/nsIComponentRegistrar.idl#82-90), meaning that it would be possible for us to discover every nsIProtocolHandler
during startup, instantiate it, and call the GetProtocolFlags
method to determine the flags. We could then cache the flags in a hashtable guarded by a mutex, meaning that it is accessible from every thread. If we added a callback interface to nsIComponentRegistrar
which let us register to be notified when contract IDs are dynamically added/updated, we could then handle all protocols without any changes to how they are registered.
Unfortunately, this would require instantiating every protocol handler very early during startup in order to read the protocol flags from them, which would probably be bad for performance, especially for implementations with large numbers of JS-implemented protocols. It would also be somewhat poor for us, as (as I mention in the dynamic section), we would likely also need to use this strategy for nsIAboutModule
flags.
In general, I'm not sure that a strategy which requires instantiating every protocol eagerly before it is used is feasible long-term, though it may turn out to be fine. It may be worth prototyping (as it is much more simple than the other approach) to see what the performance impact looks like for Firefox.
Ditch the ComponentManager
It would also be possible to use "some other mechanism" for registering protocol handlers, potentially bypassing the component manager alltogether, such as by having a static list of components and flags in the IOService at compile time, and exposing a method like void registerProtocolHandler(in ACString scheme, in nsIProtocolHandler handler, in uint32_t flags)
for dynamic registrations. This may be more difficult for other projects to use, as they won't be able to use components.conf
to statically register protocol handlers/about modules unless we add them as a special feature. This may be the most efficient, but also the most custom way to approach things.
There is already a comment in the nsIOService
to this effect, mentioning it may have a perf improvement:
// XXX we may want to speed this up by introducing our own protocol
// scheme -> protocol handler mapping, avoiding the string manipulation
// and service manager stuff
The automatic caching approach, which pre-instantiates the protocol handlers, may also have a performance improvement on this part of the lookup, as we can hold onto the instances we create during startup for later, though a full custom approach may have better long-term perf impacts.
Preference Lookups
The main other consideration for this is around preferences. There are a few preferences which would need to be made available off-main-thread in some manner, as they're used as part of protocol lookup:
-
The pref
network.protocol-handler.external.{scheme}
is checked to see if the default external protocol handler should be used to handle a given scheme for all schemes other thanfile
,chrome
andresource
(link). This information would need to be cached, perhaps by using a prefix listener, in some threadsafe format which is available off-main-thread. -
On
MOZ_WIDGET_GTK
, the GIO protocol handler will try to handle any protocols listed in thenetwork.gio.supported-protocols
string pref, rather than falling back to the default protocol handler, which will also need to be cached to be made available off-main-thread. This defaults to thesftp:
protocol, and has different protocol flags than the default protocol handler.
Assignee | ||
Comment 2•2 years ago
|
||
Leaving a needinfo for :kmag for insight into both the potential difficulties with the ExtensionProtocolHandler
, and any feedback or ideas around static component registration and non-dynamic nsIAboutModule/nsIProtocolHandler flags.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 3•2 years ago
|
||
These implementations of nsIProtocolHandlerWithDynamicFlags were actually
always returning the same flags, so can be simplified.
Updated•2 years ago
|
Assignee | ||
Comment 4•2 years ago
|
||
After this patch stack, only non-dynamic flags will be accessible
off-main-thread, as dynamic flags will still require access to the protocol
handler. By limiting the set of flags which can be dynamic, we can make the
other flags available off-main-thread.
Depends on D162800
Assignee | ||
Comment 5•2 years ago
|
||
Previously, the conversion to ensure that the key is bytes
was only done in
one place, meaning that some methods were not usable with string keys since the
switch to python 3.
Depends on D162801
Assignee | ||
Comment 6•2 years ago
|
||
This adds a new set of options to static components.conf files to allow
specifying the protocol flags and default ports of a protocol handler, and
generates a separate table just for this purpose.
This will be used in the next part as part of replacing the existing protocol
handler lookup infrastructure.
Depends on D162802
Assignee | ||
Comment 7•2 years ago
|
||
This patch replaces the previous ContractID-based lookup system for protocol
handlers, and replaces it with a new custom system in nsIOService. It will be
pre-populated with non-overridable static protocol handlers using the
StaticComponents infrastructure added in the previous part, and callers can
also dynamically register new protocol handlers at runtime.
This new system is intended to provide access to the default port and
non-dynamic protocol flags off-main-thread, by requiring these values to be
provided up-front as constants, rather than getting them from the xpcom
interface. The data is then guarded by an RWLock.
Callers which look up specific handlers by their contractID are not changed, as
the contract IDs for existing handlers have not been changed, so the lookup
will still succeed.
This change as-implemented breaks the nsGIOProtocolHandler on Linux, as it
removes the special code which would try to use that handler for some
protocols. This will be fixed in a later part by making the
nsGIOProtocolHandler use the dynamic registration APIs to register and
un-register protocol handlers at runtime in response to the GIO pref.
Depends on D162803
Assignee | ||
Comment 8•2 years ago
|
||
This removes all implementations of these types. Some implementations in JS
code were also removed in the previous part, when updating tests to use
Services.io.registerProtocolHandler
.
Code which used to access these members should go through the IOService now
instead.
Depends on D162804
Assignee | ||
Comment 9•2 years ago
|
||
This swaps out the previous code which hard-coded nsGIOProtocolHandler
handling inside of the IOService, and replaces it with
nsGIOProtocolHander dynamically registering and un-registering protocol
handlers at runtime in response to the relevant preference, so that the
protocols can be handled with the normal dynamic handler codepath.
The service is configured to start-up during 'xpcom-startup' through the
category manager, so attempts to use GIO protocols before then will
fail. If this ends up being a problem, we can move it earlier during
startup.
Depends on D162805
Assignee | ||
Comment 10•2 years ago
|
||
The patch stack I've attached doesn't tackle the extra complexity involved with supporting nsIProtocolHandlerWithDynamicFlags implementations off-main-thread, meaning that while most flags are threadsafe to query, some are still not threadsafe (meaning that checking them off-main-thread will lead to assertions). I've updated the description
Updated•2 years ago
|
Comment 11•2 years ago
|
||
Comment 12•2 years ago
|
||
Backed out 7 changesets (Bug 1793463) for causing bustages on nsIOService.cpp.
Backout link
Push with failures <--> Bb
Failure Log
Comment 13•2 years ago
|
||
Assignee | ||
Updated•2 years ago
|
Comment 14•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/793f8605c61c
https://hg.mozilla.org/mozilla-central/rev/3163763eb9f7
https://hg.mozilla.org/mozilla-central/rev/03399898511f
https://hg.mozilla.org/mozilla-central/rev/40dfccca9a71
https://hg.mozilla.org/mozilla-central/rev/d65e4ed67e61
https://hg.mozilla.org/mozilla-central/rev/6c9bf920e7f5
https://hg.mozilla.org/mozilla-central/rev/737e4828685f
Description
•