1901387 - {nsIPrincipal, BasePrincipal}::CheckMayLoad needs to be threadsafe to allow workers to stop needing to synchronously block and consult the main thread

Status: RESOLVED FIXED

(Core :: Security: CAPS, enhancement)

Description

[Andrew Sutherland [:asuth] (he/him)]

As this comment in CheckMayLoadHelper expresses, CheckMayLoad performs some accesses that are not threadsafe:

AssertIsOnMainThread();  // Accesses non-threadsafe URI flags and the
                         // non-threadsafe ExtensionPolicyService

This results in situations like in Request where the Worker needs to use a WorkerMainThreadRunnable subclass which is synchronously dispatched to the main thread. Note that while processes that are already async don't need to do this synchronously, the performance impact obviously remains, but in many cases spec text assumes we can do these checks synchronously so a syncloop is necessary. Also note this is not just a performance problem, as we have seen cases where content assumes Gecko workers are fully independent threads that don't depend on the main thread yielding control flow, leading to potential deadlocks (bug 1888109) and OOMs (bug 1797413).

Comment 1

[Nika Layzell [:nika] (ni? for response)]

As far as I remember from bug 1443925, this method isn't threadsafe for 3 major reasons:

1. The method relies on 2 protocol flags which are dynamic. Dynamic flags are only available on the main thread, as nsIProtocolHandlerWithDynamicFlags implementations are not threadsafe. As I noted in bug 1793463 comment 1, there are a number of implementations of these types which vary in difficulty to make threadsafe. Unlike in the original comment, the moz-extension protocol would now be "relatively easy" to make threadsafe (I moved the required methods to WebExtensionPolicyCore already), but the about protocol would most likely require replacing the nsIAboutModule registration and look-up infrastructure for flags (perhaps similar to what I did in bug 1793463 for protocol flags), as nsIAboutModule may be (and is) implemented by JS.

2. The call to SourceMayLoadExtensionURI is non-threadsafe. This could be fixed by changing it to use methods on WebExtensionPolicyCore, as all of the relevant methods are already available on the threadsafe core object.

3. The call to nsScriptSecurityManager::ReportError may be non-threadsafe, though it's unclear to me whether this is the case. bug 1900706 is tracking improving the situation around reporting errors off-main-thread somewhat, and after that change it might be possible to replace the aReport and aInnerWindowId arguments with a single nsIGlobalObject* aGlobalForReport or similar. This also isn't an issue if reporting is disabled.

---

The only significant piece of work here is the change to dynamic protocol flags, however it appears that is not actually necessary. If we query the details which the dynamic flags are being checked for directly, it is possible to access all of the relevant data from off-main-thread. I'll post a patch which does this and should make the method threadsafe.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1901387 - Part 1: Ensure URLInfo is safe to use OMT, r=#extension-reviewers!

Previously accessing Scheme() on a URLInfo was main-thread-only. As more
extension methods are used OMT, this needs to be switched to the threadsafe
NS_Atomize method instead.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1901387 - Part 2: Add EPS::GetCoreByURL, r=#extension-reviewers!

This is a helper method for looking up a WebExtensionPolicyCore off-main-thread
by extension URL, and will be used in part 3.

Depends on D215024

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1901387 - Part 3: Make nsIPrincipal::CheckMayLoad threadsafe, r=#extension-reviewers!,asuth!,ckerschb!

This method was previously non-threadsafe due to it needing to access dynamic
URI flags. These flags were used to check the WEbExtensionPolicy to see if the
webextension resource being loaded should be accessible.

Making dynamic URI flags available off-main-thread in general would
unfortunately be quite difficult, due to some of them depending on things like
JS nsIAboutModule implementations, so that was not the approach taken.

Instead, all information required is already available in the threadsafe
WebExtensionPolicyCore, which is now directly queried, instead of being queried
indirectly through protocol flags.

Depends on D215025

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1901387 - Part 4: Remove the main thread sync dispatch for CheckMayLoad from Request, r=asuth!

The dispatch to the main thread, and specialized worker-thread parsing
should no longer be necessary now that nsIURI and nsIPrincipal are
properly threadsafe.

This patch also collapses the main-thread and off-main-thread logic
together for URI parsing, removing some repetition.

Depends on D215026

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf21612b73f5
Part 1: Ensure URLInfo is safe to use OMT, r=extension-reviewers,robwu
https://hg.mozilla.org/integration/autoland/rev/16fee2b4a43e
Part 2: Add EPS::GetCoreByURL, r=extension-reviewers,robwu
https://hg.mozilla.org/integration/autoland/rev/44acfd66c9be
Part 3: Make nsIPrincipal::CheckMayLoad threadsafe, r=asuth,ckerschb,extension-reviewers,robwu
https://hg.mozilla.org/integration/autoland/rev/2224a1f315a0
Part 4: Remove the main thread sync dispatch for CheckMayLoad from Request, r=asuth

Comment 7

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/cf21612b73f5
https://hg.mozilla.org/mozilla-central/rev/16fee2b4a43e
https://hg.mozilla.org/mozilla-central/rev/44acfd66c9be
https://hg.mozilla.org/mozilla-central/rev/2224a1f315a0