Closed Bug 1793463 Opened 2 years ago Closed 2 years ago

Make NS_URIChainHasFlags (mostly) threadsafe

Categories

(Core :: Networking, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
109 Branch
Tracking Status
firefox109 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(7 files)

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.

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:

  1. 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.

  2. WebExtensionPolicy::ManifestVersion() - could also easily be moved to a threadsafe tearoff, as it is known at construction time.

  3. WebExtensionPolicy::PrivateBrowsingAllowed() - would be somewhat annoying, but still possible, to move to a threadsafe tearoff. The mPermissions field is an AtomSet, which is not threadsafe, and can be mutated, though both can be solved by changing types and adding a mutex to the threadsafe tearoff.

  4. 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 way WebAccessibleResource is implemented is an implementation detail of WebExtensionPolicy, 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 contractids 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:

https://searchfox.org/mozilla-central/rev/12a34801fbaa6f14e20515a44b1fd77179b02617/netwerk/base/nsIOService.cpp#923-925

// 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:

  1. 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 than file, chrome and resource (link). This information would need to be cached, perhaps by using a prefix listener, in some threadsafe format which is available off-main-thread.

  2. On MOZ_WIDGET_GTK, the GIO protocol handler will try to handle any protocols listed in the network.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 the sftp: protocol, and has different protocol flags than the default protocol handler.

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.

Flags: needinfo?(kmaglione+bmo)
Depends on: 1793995
Flags: needinfo?(kmaglione+bmo)

These implementations of nsIProtocolHandlerWithDynamicFlags were actually
always returning the same flags, so can be simplified.

Assignee: nobody → nika
Status: NEW → ASSIGNED

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

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

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

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

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

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

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

Summary: Make NS_URIChainHasFlags threadsafe → Make NS_URIChainHasFlags (mostly) threadsafe
Severity: -- → N/A
Priority: -- → P2
Whiteboard: [necko-triaged]
Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/68a3eb8a3057 Part 1: Remove unused nsIProtocolHandlerWithDynamicFlags impls, r=necko-reviewers,valentin https://hg.mozilla.org/integration/autoland/rev/eba1123c8b56 Part 2: Limit the set of protocol flags which can be dynamic, r=necko-reviewers,valentin https://hg.mozilla.org/integration/autoland/rev/a116e6f089f2 Part 3: Handle non-bytes keys more consistently in perfecthash, r=emilio https://hg.mozilla.org/integration/autoland/rev/d0f18d300818 Part 4: Generate static components entries for protocols, r=necko-reviewers,xpcom-reviewers,valentin,kmag https://hg.mozilla.org/integration/autoland/rev/d168599a269e Part 5: Stop using contractids to fetch protocol handlers, r=necko-reviewers,xpcom-reviewers,webdriver-reviewers,whimboo,valentin,kmag https://hg.mozilla.org/integration/autoland/rev/71afe900eb17 Part 6: Remove nsIProtocolHandler.{defaultPort,protocolFlags}, r=necko-reviewers,valentin https://hg.mozilla.org/integration/autoland/rev/47c3acb30de2 Part 7: Use dynamic registration for protocols handled by nsGIOProtocolHandler, r=necko-reviewers,valentin

Backed out 7 changesets (Bug 1793463) for causing bustages on nsIOService.cpp.
Backout link
Push with failures <--> Bb
Failure Log

Flags: needinfo?(nika)
Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/793f8605c61c Part 1: Remove unused nsIProtocolHandlerWithDynamicFlags impls, r=necko-reviewers,valentin https://hg.mozilla.org/integration/autoland/rev/3163763eb9f7 Part 2: Limit the set of protocol flags which can be dynamic, r=necko-reviewers,valentin https://hg.mozilla.org/integration/autoland/rev/03399898511f Part 3: Handle non-bytes keys more consistently in perfecthash, r=emilio https://hg.mozilla.org/integration/autoland/rev/40dfccca9a71 Part 4: Generate static components entries for protocols, r=necko-reviewers,xpcom-reviewers,valentin,kmag https://hg.mozilla.org/integration/autoland/rev/d65e4ed67e61 Part 5: Stop using contractids to fetch protocol handlers, r=necko-reviewers,xpcom-reviewers,webdriver-reviewers,whimboo,valentin,kmag https://hg.mozilla.org/integration/autoland/rev/6c9bf920e7f5 Part 6: Remove nsIProtocolHandler.{defaultPort,protocolFlags}, r=necko-reviewers,valentin https://hg.mozilla.org/integration/autoland/rev/737e4828685f Part 7: Use dynamic registration for protocols handled by nsGIOProtocolHandler, r=necko-reviewers,valentin
Flags: needinfo?(nika)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: