1745056 - Check thread safety of PermissionManager initialization

Status: RESOLVED FIXED

(Core :: Permission Manager, task)

Description

[Jens Stutte [:jstutte]]

There is at least a known minimal possibility for a race on GetInstance.

Comment 1

[Jens Stutte [:jstutte]]

It looks like PermissionManager::GetInstance wants to avoid to take the sCreationMutex lock that GetXPCOMSingleton actually takes. I assume this "just works" as is currently because we can kind of rely on finding either nullptr or the entirly written correct pointer in gPermissionManager due to pseudo-atomical behavior of 64Bit writes on most of the systems of interest. But we could make this explicit either by:

1. Making gPermissionManager atomic or
2. just always call GetXPCOMSingleton directly, if we think it is not too performance critical to take the creation lock.

It is worth mentioning that PermissionManager is NS_DECL_THREADSAFE_ISUPPORTS which implies IMHO that at least creation and destruction of the singleton should be entirely thread safe.

Comment 2

[Jens Stutte [:jstutte]]

Attachment: Bug 1745056 - Make PermissionManager lifecycle truely thread safe. r?asuth

This patch makes PermissionManager::GetInstance do the instantiation
work and GetXPCOMSingleton just uses it.
We always acquire the creation mutex and return an already_AddRefed to
avoid any possible race. We count on callers of GetInstance to
(shortly) keep a local reference if they have more work to do.
On async shutdown in the parent process, we first mark our singleton
dead, then close all our resources async and finally null our instance
holder only after the async shutdown has finished.

Comment 3

[Jens Stutte [:jstutte]]

Attachment: Bug 1745056 - Synchronize TemporaryAccessGrantObserver shutdown with PermissionManager. r?asuth

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2636a0d71a2f
Make PermissionManager lifecycle truely thread safe. r=asuth,cookie-reviewers,anti-tracking-reviewers,permissions-reviewers,valentin,timhuang
https://hg.mozilla.org/integration/autoland/rev/b2b47f64fd35
Synchronize TemporaryAccessGrantObserver shutdown with PermissionManager. r=asuth,anti-tracking-reviewers,timhuang

Comment 5

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/2636a0d71a2f
https://hg.mozilla.org/mozilla-central/rev/b2b47f64fd35