624490 - nsPrefService should be threadsafe

Status: RESOLVED WONTFIX

(Core :: Preferences: Backend, defect)

Description

[Kai Engert (:KaiE:)]

We are seeing lots of fallout regressions from the recent decision to strictly disallow access to the pref service from non-main threads.

For example, the SSL client authentication is broken.

Given we're a multithreaded application, this seems like a very fundamental limitation.

Rather than to deal with lots of fallout regressions, I propose we urgently make nsPrefService threadsafe.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I believe there is no really safe way to do that, actually.  The API, especially the observer bits, fundamentally assumes single-threaded operation.

Comment 2

[Boris Zbarsky [:bzbarsky]]

And as a minimum your proposal assumes that all preference observers are themselves threadsafe.  Fixing just that part is almost certainly more work than sorting out the PSM issues, as far as I can see.

Comment 3

[Joshua Cranmer [:jcranmer]]

If there's no other way, it could be possible to force the method to sync-proxy to the main thread.

Alternatively, what would happen if the observer calls were merely proxied to the main thread/source event target they came from?

Comment 4

[Kai Engert (:KaiE:)]

Could we allow read-only access from non-main threads?

Comment 5

[Shawn Wilsher :sdwilsh]

bug 619487 was checked in with the wrong commit message...

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> Could we allow read-only access from non-main threads?
That's not safe unless we serialize access to the hash table (which is going to be slower).

Comment 7

[Benjamin Smedberg]

No really, we don't want to pay the performance penalty or code complexity cost of either making the code natively threadsafe using locking, or doing some sort of thread proxy magic.

If you need preferences, you should retrieve and store them on the main thread before starting worker-thread tasks which might need preference data.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to comment #7)
> No really, we don't want to pay the performance penalty or code complexity cost
> of either making the code natively threadsafe using locking, or doing some sort
> of thread proxy magic.
> 
> If you need preferences, you should retrieve and store them on the main thread
> before starting worker-thread tasks which might need preference data.

RE performance: The code already checks to see if it is on the main thread. So, the read-only functions can check if they are running on the main thread and then, if not, grab the mutex. For functions that aren't read-only, is the performance a real concern?

RE code complexity: If we don't make read-only access thread safe them we have to distribute a bunch of mutexes in lots of code that needs to read preferences. Making read-only access from other threads safe would make the code simpler, in aggregate.

> If you need preferences, you should retrieve and store
> them on the main thread before starting worker-thread
> tasks which might need preference data.

Do we have time to find and fix all of these places before FF4.0?

Comment 9

[Kai Engert (:KaiE:)]

You effectly require that we do full a code inspection for any preference access in our code.

For many preferences it will mean we have to keep another variable and register observers.

PSM uses several preferences for influencing the behaviour of SSL threads and the amount of warnings you get.

Who will do this work?

Comment 10

[Benjamin Smedberg]

Yes, or leave them broken. They were *already* probably a source of persistent minor crashes in prior versions. And in the near future I plan to make the XPCOM service manager itself main-thread-only.

Comment 11

[Benjamin Smedberg]

How many of these prefs are exposed to users? For developer/debugging prefs, you should just read the pref once at startup, and then don't bother with observers and mutexes and such.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> check if they are running on the main thread and then, if not, grab the mutex.

That doesn't work, as you'll realize if you actually think about the proposal.

Comment 13

[Kai Engert (:KaiE:)]

(In reply to comment #11)
> How many of these prefs are exposed to users? For developer/debugging prefs,
> you should just read the pref once at startup, and then don't bother with
> observers and mutexes and such.

Finding this answer is part of the investigation that needs to happen.

Comment 14

[Ben Bucksch (:BenB)]

I agree with Brian's assessment in comment 8.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> For functions that aren't read-only, is the performance a real concern?

Fwiw... it can be.  Some things (printing, say) set many preferences all at once.