Bug 1443925 Comment 11 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

I looked into this a bit again recently, and here are a few of my current thoughts:

## Mutable State
There are a few pieces of mutable state (after `Init()`) on principal objects, which makes them not fully immutable. In order to get this working sooner-rather-than-later, we probably can't eliminate all of these pieces of mutable state, but we can at least make them threadsafe:

* For `nsIContentSecurityPolicy` on `ExpandedPrincipal` it should be possible to instead store a `DataMutex<nsMainThreadPtrHandle<nsIContentSecurityPolicy>>` which would avoid most of the issues there. It wouldn't be settable from OMT, but things like cloning the principal with different `OriginAttributes` would be fine. This would let us avoid waiting for CSP to be moved off of the type.
* The `nsCOMPtr<nsIURI> mDomain` on `ContentPrincipal` could be also guarded by a `DataMutex<...>` without too much difficulty, with `bool mHasExplicitDomain` changed to a `std::atomic<bool>`. However, that could potentially have a performance impact. I think the impact could be minimized by checking the `mHasExplicitDomain` bool before even trying to acquire the mutex to make the checks more efficient. Alternatively, we could make the field unguarded and main-thread accessible only, but that feels less obviously safe and means some methods won't be able to work OMT.
* For `WeakPtr<WebExtensionPolicy>` on `ContentPrincipal`, it is a lazily initialized field which is created whenever `AddonPolicy()` is called. I think one of the easier ways to handle it would be to have a reference counted type using `NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_DELETE_ON_MAIN_THREAD` which contains the `WeakPtr<WebExtensionPolicy>` and store a pointer to that instead (perhaps behind a mutex to be safe, even if the field is only ever touched on the main thread or in the destructor, it can of course be optimized if needed)

In general, I think these pieces of mutable state are workable. Many of them could be optimized further if they end up being performance issues, but I think that wrapping specific fields in `DataMutex` and marking the other fields explicitly as `const` is probably our best bet for getting an initial threadsafe principal type.

## Reference Counting
The reference counting for principal types is special, as the `refcount` field is inherited from `JSPrincipals`. However, this field is already atomic (https://searchfox.org/mozilla-central/rev/e3c34d34cc7dce56df377f05183ad379ac1de123/js/public/Principals.h#25), so making reference counting atomic for principals should only require removing the assertions from `nsJSPrincipals.cpp` (https://searchfox.org/mozilla-central/rev/e3c34d34cc7dce56df377f05183ad379ac1de123/caps/nsJSPrincipals.cpp#27,36)

## Init Methods
Currently, principals are generally created with a default constructor and then initialized by calling an init method of some form, which ends in calling `BasePrincipal::FinishInit`. The principal isn't usable until this method is called. This is a fairly simple change in the majority of cases, as the distance between the constructor & init calls aren't very wide, and the `Init` methods are generally not exposed on `nsIPrincipal`. The default constructors will need to be removed and replaced with explicit constructors which take arguments similar to the init methods (although some may need to be made private so that the more ergonomic init API can be exposed as a static `::Create` method or similar).

The main issue here is likely to be the remaining uses of principals through the component system, which can all be dealt with in one way or another:
1. Instances which create a null principal with `"@mozilla.org/nullprincipal;1"` can be switched to use `Services.scriptSecurityManager` in JS or invoking `NullPrincipal::CreateWithoutOriginAttributes` in C++.
2. The System Principal can remain a singleton but with the init method removed, and the relevant initialization instead happening in the constructor. It can still be fetched using the service manager
3. Other service types are not accessed directly as components through the service manager, so should be able to have their contract IDs removed (though not their CIDs because of `nsISerializable`).

### nsISerializable
The last issue with the component manager is that every `nsIPrincipal` implements `nsISerializable`. There is no longer support for writing out new serialized forms of these types with `nsISerializable::Write()` (they're all stubbed out now), but they can still be read with `::Read()`. This will probably need to be solved in a similar way to how it was for `nsIURI`, where the CID is changed to map to a different object (e.g. `ContentPrincipal::Deserializer`) which is non-threadsafe & implements `nsISerialzable`, and when QI-ed to `nsIPrincipal` will return the constructed principal from the read call.

The only type which won't need this change is `SystemPrincipal` whose `Read` method is already a no-op, so it can continue to directly implement the interface.

## Determining which methods are Threadsafe
Once all of these are changed, I think that the core principal object will be mostly threadsafe at rest, though many methods will still be main-thread only. We'll need to go through and check which ones are using main-thread-only services etc. From a quick scan, many methods will not work off-main-thread, and will need to be documented as such.

* Many methods access `AddonPolicy()` (even if just to null-check it) which is main-thread-only due to `WebExtensionPolicy` being cycle-collected. These methods won't work off-main-thread and some important ones may need to be changed.
* Many methods use various global components which either are not or may not be fully threadsafe, such as: `nsIIOService`/`nsINetUtil`, `nsIEffectiveTLDService`, `mozIThirdPartyUtil`, `BlobURLProtocolHandler`, `nsScriptSecurityManager`, and the various `nsIProtocolHandler` implementations.
* Finally some methods will just never work of-main-thread as they intentionally call into other components for specific purposes, such as `ContentPrincipal::SetDomain` which calls into the JSAPI to update compartment wrappers etc.

These include some pretty important methods, like `ContentPrincipal::GenerateOriginNoSuffixFromURI`, which might make a threadsafe `nsIPrincipal` somewhat unusable off-main-thread. I imagine that at a minimum we'd want to support the relevant APIs for creating principals.

If we do end up doing this change, we'll probably want to start by annotating basically every method as being main-thread-only, adding a `MOZ_ASSERT(NS_IsMainThread())` to it, and then iteratively remove these assertions as we determine that a specific method is actually safe to call off-main-thread.
I looked into this a bit again recently, and here are a few of my current thoughts:

## Mutable State
There are a few pieces of mutable state (after `Init()`) on principal objects, which makes them not fully immutable. In order to get this working sooner-rather-than-later, we probably can't eliminate all of these pieces of mutable state, but we can at least make them threadsafe:

* For `nsIContentSecurityPolicy` on `ExpandedPrincipal` it should be possible to instead store a `DataMutex<nsMainThreadPtrHandle<nsIContentSecurityPolicy>>` which would avoid most of the issues there. It wouldn't be settable from OMT, but things like cloning the principal with different `OriginAttributes` would be fine. This would let us avoid waiting for CSP to be moved off of the type.
* The `nsCOMPtr<nsIURI> mDomain` on `ContentPrincipal` could be also guarded by a `DataMutex<...>` without too much difficulty, with `bool mHasExplicitDomain` changed to a `std::atomic<bool>`. However, that could potentially have a performance impact. I think the impact could be minimized by checking the `mHasExplicitDomain` bool before even trying to acquire the mutex to make the checks more efficient. Alternatively, we could make the field unguarded and main-thread accessible only, but that feels less obviously safe and means some methods won't be able to work OMT.
* For `WeakPtr<WebExtensionPolicy>` on `ContentPrincipal`, it is a lazily initialized field which is created whenever `AddonPolicy()` is called. I think one of the easier ways to handle it would be to have a reference counted type using `NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_DELETE_ON_MAIN_THREAD` which contains the `WeakPtr<WebExtensionPolicy>` and store a pointer to that instead (perhaps behind a mutex to be safe, even if the field is only ever touched on the main thread or in the destructor, it can of course be optimized if needed)

In general, I think these pieces of mutable state are workable. Many of them could be optimized further if they end up being performance issues, but I think that wrapping specific fields in `DataMutex` and marking the other fields explicitly as `const` is probably our best bet for getting an initial threadsafe principal type.

## Reference Counting
The reference counting for principal types is special, as the `refcount` field is inherited from `JSPrincipals`. However, this field is already atomic (https://searchfox.org/mozilla-central/rev/e3c34d34cc7dce56df377f05183ad379ac1de123/js/public/Principals.h#25), so making reference counting atomic for principals should only require removing the assertions from `nsJSPrincipals.cpp` (https://searchfox.org/mozilla-central/rev/e3c34d34cc7dce56df377f05183ad379ac1de123/caps/nsJSPrincipals.cpp#27,36)

## Init Methods
Currently, principals are generally created with a default constructor and then initialized by calling an init method of some form, which ends in calling `BasePrincipal::FinishInit`. The principal isn't usable until this method is called. This is a fairly simple change in the majority of cases, as the distance between the constructor & init calls aren't very wide, and the `Init` methods are generally not exposed on `nsIPrincipal`. The default constructors will need to be removed and replaced with explicit constructors which take arguments similar to the init methods (although some may need to be made private so that the more ergonomic init API can be exposed as a static `::Create` method or similar).

The main issue here is likely to be the remaining uses of principals through the component system, which can all be dealt with in one way or another:
1. Instances which create a null principal with `"@mozilla.org/nullprincipal;1"` can be switched to use `Services.scriptSecurityManager` in JS or invoking `NullPrincipal::CreateWithoutOriginAttributes` in C++.
2. The System Principal can remain a singleton but with the init method removed, and the relevant initialization instead happening in the constructor. It can still be fetched using the service manager
3. Other service types are not accessed directly as components through the service manager, so should be able to have their contract IDs removed (though not their CIDs because of `nsISerializable`).

### nsISerializable
The last issue with the component manager is that every `nsIPrincipal` implements `nsISerializable`. There is no longer support for writing out new serialized forms of these types with `nsISerializable::Write()` (they're all stubbed out now), but they can still be read with `::Read()`. This will probably need to be solved in a similar way to how it was for `nsIURI`, where the CID is changed to map to a different object (e.g. `ContentPrincipal::Deserializer`) which is non-threadsafe & implements `nsISerialzable`, and when QI-ed to `nsIPrincipal` will return the constructed principal from the read call.

The only type which won't need this change is `SystemPrincipal` whose `Read` method is already a no-op, so it can continue to directly implement the interface.

## Determining which methods are Threadsafe
Once all of these are changed, I think that the core principal object will be mostly threadsafe at rest, though many methods will still be main-thread only. We'll need to go through and check which ones are using main-thread-only services etc. From a quick scan, many methods will not work off-main-thread, and will need to be documented as such.

* Many methods access `AddonPolicy()` (even if just to null-check it) which is main-thread-only due to `WebExtensionPolicy` being cycle-collected. These methods won't work off-main-thread and some important ones may need to be changed.
* Many methods use various global components which either are not or may not be fully threadsafe, such as: `nsIIOService`/`nsINetUtil`, `nsIEffectiveTLDService`, `mozIThirdPartyUtil`, `BlobURLProtocolHandler`, `nsScriptSecurityManager`, NSPR, and the various `nsIProtocolHandler` implementations.
* Finally some methods will just never work of-main-thread as they intentionally call into other components for specific purposes, such as `ContentPrincipal::SetDomain` which calls into the JSAPI to update compartment wrappers etc.

These include some pretty important methods, like `ContentPrincipal::GenerateOriginNoSuffixFromURI`, which might make a threadsafe `nsIPrincipal` somewhat unusable off-main-thread. I imagine that at a minimum we'd want to support the relevant APIs for creating principals.

If we do end up doing this change, we'll probably want to start by annotating basically every method as being main-thread-only, adding a `MOZ_ASSERT(NS_IsMainThread())` to it, and then iteratively remove these assertions as we determine that a specific method is actually safe to call off-main-thread.

Back to Bug 1443925 Comment 11