585706 - nsNSSCertificateDB calls into the preference service off of the main thread

Status: RESOLVED DUPLICATE of bug 714477

(Core :: Security: PSM, defect)

Description

[Shawn Wilsher :sdwilsh]

Bug 580790 makes us assert and return failure when accessing the preference service off of the main thread (which was illegal to do anyway).  However, it cannot land because nsNSSCertificateDB calls into the preference service off of the main thread with the following stack:

###!!! ASSERTION: Cannot be used on a background thread!: 'Error', file /builds/slave/tryserver-macosx-debug/build/modules/libpref/src/nsPrefService.cpp, line 840

  <<<<<<<
NEXT ERROR PROCESS-CRASH | /Users/cltbld/talos-slave/tryserver_leopard-debug_test-xpcshell/build/xpcshell/tests/test_extensionmanager/xpcshell/test_bootstrap.js | application crashed (minidump found)
Operating system: Mac OS X
                  10.5.8 9L31a
CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE
Crash address: 0x0

NEXT ERROR Thread 15 (crashed)
 0  libmozalloc.dylib!TouchBadMemory [mozalloc_abort.cpp:92b3eb4e982c : 64 + 0x5]
    eip = 0x00042f43   esp = 0xb081e130   ebp = 0xb081e138   ebx = 0x00042f66
    esi = 0x052ee490   edi = 0x00000001   eax = 0x00000000   ecx = 0x00042f3d
    edx = 0x00000000   efl = 0x00010286
    Found by: given as instruction pointer in context
 1  libmozalloc.dylib!mozalloc_abort [mozalloc_abort.cpp:92b3eb4e982c : 85 + 0x4]
    eip = 0x00042f9f   esp = 0xb081e140   ebp = 0xb081e158   ebx = 0x00042f66
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
 2  XUL!Abort [nsDebugImpl.cpp:92b3eb4e982c : 379 + 0xa]
    eip = 0x03923b39   esp = 0xb081e160   ebp = 0xb081e178   ebx = 0x03923e3b
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
 3  XUL!NS_DebugBreak_P [nsDebugImpl.cpp:92b3eb4e982c : 366 + 0xd]
    eip = 0x03924162   esp = 0xb081e180   ebp = 0xb081e5a8   ebx = 0x03923e3b
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
 4  XUL!nsPrefService::CheckAndLogBackgroundThreadUse [nsPrefService.cpp:92b3eb4e982c : 840 + 0x31]
    eip = 0x0256010c   esp = 0xb081e5b0   ebp = 0xb081e608   ebx = 0x025600c4
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
 5  XUL!nsPrefBranch::GetIntPref [nsPrefBranch.cpp:92b3eb4e982c : 269 + 0x4]
    eip = 0x0255e1c5   esp = 0xb081e610   ebp = 0xb081e658   ebx = 0x0343d411
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
 6  XUL!nsPrefService::GetIntPref [nsPrefService.h:92b3eb4e982c : 60 + 0x29]
    eip = 0x02563020   esp = 0xb081e660   ebp = 0xb081e678   ebx = 0x0343d411
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
 7  XUL!nsNSSCertificateDB::GetIsOcspOn [nsNSSCertificateDB.cpp:92b3eb4e982c : 1366 + 0x29]
    eip = 0x0343d459   esp = 0xb081e680   ebp = 0xb081e6a8   ebx = 0x0343d411
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
 8  XUL!nsNSSCertificate::hasValidEVOidTag [nsIdentityChecking.cpp:92b3eb4e982c : 1007 + 0x1f]
    eip = 0x0344e860   esp = 0xb081e6b0   ebp = 0xb081e928   ebx = 0x0344e75b
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
 9  XUL!nsNSSCertificate::getValidEVOidTag [nsIdentityChecking.cpp:92b3eb4e982c : 1114 + 0x18]
    eip = 0x0344ec19   esp = 0xb081e930   ebp = 0xb081e968   ebx = 0x0344ed62
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
10  XUL!nsNSSCertificate::GetIsExtendedValidation [nsIdentityChecking.cpp:92b3eb4e982c : 1140 + 0x18]
    eip = 0x0344ee22   esp = 0xb081e970   ebp = 0xb081e9b8   ebx = 0x0344ed62
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
11  XUL!AuthCertificateCallback [nsNSSCallbacks.cpp:92b3eb4e982c : 1014 + 0x21]
    eip = 0x033f00c1   esp = 0xb081e9c0   ebp = 0xb081ea98   ebx = 0x033eff48
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
12  libssl3.dylib!ssl3_HandleCertificate [ssl3con.c : 7898 + 0x2e]
    eip = 0x001c26d8   esp = 0xb081eaa0   ebp = 0xb081eb28   ebx = 0x001c2295
    esi = 0x052ee490   edi = 0x00000001
    Found by: call frame info
13  libssl3.dylib!ssl3_HandleHandshakeMessage [ssl3con.c : 8597 + 0x18]
    eip = 0x001c434a   esp = 0xb081eb30   ebp = 0xb081eba8   ebx = 0x001c3e71
    esi = 0x00000301   edi = 0x00000001
    Found by: call frame info
14  libssl3.dylib!ssl3_HandleHandshake [ssl3con.c : 8721 + 0x20]
    eip = 0x001c47ee   esp = 0xb081ebb0   ebp = 0xb081ebe8   ebx = 0x001c4612
    esi = 0x00000301   edi = 0x00000001
    Found by: call frame info
15  libssl3.dylib!ssl3_HandleRecord [ssl3con.c : 9060 + 0x11]
    eip = 0x001c5355   esp = 0xb081ebf0   ebp = 0xb081ece8   ebx = 0x001c4a64
    esi = 0x00000301   edi = 0x00000001
    Found by: call frame info
16  libssl3.dylib!ssl3_GatherCompleteHandshake [ssl3gthr.c : 209 + 0x1d]
    eip = 0x001c64aa   esp = 0xb081ecf0   ebp = 0xb081ed38   ebx = 0x001c63cd
    esi = 0xb081f000   edi = 0x00000000
    Found by: call frame info
17  libssl3.dylib!ssl_GatherRecord1stHandshake [sslcon.c : 1258 + 0x12]
    eip = 0x001c9151   esp = 0xb081ed40   ebp = 0xb081ed88   ebx = 0x001c90cc
    esi = 0xb081f000   edi = 0x00000000
    Found by: call frame info
18  libssl3.dylib!ssl_Do1stHandshake [sslsecur.c : 151 + 0x10]
    eip = 0x001d475c   esp = 0xb081ed90   ebp = 0xb081edb8   ebx = 0x001d4517
    esi = 0xb081f000   edi = 0x00000000
    Found by: call frame info
19  libssl3.dylib!ssl_SecureSend [sslsecur.c : 1213 + 0xa]
    eip = 0x001d6980   esp = 0xb081edc0   ebp = 0xb081ee08   ebx = 0x001d673d
    esi = 0xb081f000   edi = 0x00000000
    Found by: call frame info
20  libssl3.dylib!ssl_SecureWrite [sslsecur.c : 1258 + 0x20]
    eip = 0x001d6b37   esp = 0xb081ee10   ebp = 0xb081ee28   ebx = 0x001de310
    esi = 0xb081f000   edi = 0x00000000
    Found by: call frame info
21  libssl3.dylib!ssl_Write [sslsock.c : 1652 + 0x1e]
    eip = 0x001de3c2   esp = 0xb081ee30   ebp = 0xb081ee68   ebx = 0x001de310
    esi = 0xb081f000   edi = 0x00000000
    Found by: call frame info
22  XUL!nsSSLThread::Run [nsSSLThread.cpp:92b3eb4e982c : 1045 + 0x23]
    eip = 0x033e832e   esp = 0xb081ee70   ebp = 0xb081ef18   ebx = 0x033e8011
    esi = 0xb081f000   edi = 0x00000000
    Found by: call frame info
23  XUL!nsPSMBackgroundThread::nsThreadRunner [nsPSMBackgroundThread.cpp:92b3eb4e982c : 44 + 0xe]
    eip = 0x033e7641   esp = 0xb081ef20   ebp = 0xb081ef48   ebx = 0x00082b4d
    esi = 0xb081f000   edi = 0x00000000
    Found by: call frame info
24  libnspr4.dylib!_pt_root [ptthread.c:92b3eb4e982c : 228 + 0x10]
    eip = 0x00082c63   esp = 0xb081ef50   ebp = 0xb081ef78   ebx = 0x00082b4d
    esi = 0xb081f000   edi = 0x00000000
    Found by: call frame info
25  libSystem.B.dylib + 0x32154
    eip = 0x9296b155   esp = 0xb081ef80   ebp = 0xb081efc8   ebx = 0x9296b028
    esi = 0xb081f000   edi = 0x00000000
    Found by: call frame info
26  libSystem.B.dylib + 0x32011
    eip = 0x9296b012   esp = 0xb081efd0   ebp = 0xb081efec
    Found by: previous frame's frame pointer

Comment 1

[Shawn Wilsher :sdwilsh]

Bug 580790 is a blocker, so this is now a blocker too.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, this thing should be caching the pref and using a pref observer...

Comment 3

[timeless]

Attachment: untested

i think this does the right thing (unless of course the db is created on the wrong thread)

Comment 4

[Kai Engert (:KaiE:)]

Question regarding the patch and in particular about the behaviour of the pref observers.

This patch makes the assumption the preference is set to "true" on init, it will cache this "true" value. The code will update the cached value whenever the observe function gets called, and the patch also requests to be notified for changes on this preference.

But... Shouldn't there be some kind of initial request for the real initial value?

What happens if the preference is set to "false", the app is started, and the preference never gets changed?

I assumed the "observe" method only gets called on changes, but I don't see anything that suggest some initial notification about the current state of the preference.

If my assumption is right, this code is wrong.
If my assumption is wrong, the patch is fine.

Thanks in advance for clarification.

Comment 5

[timeless]

NS_NSS_GENERIC_FACTORY_CONSTRUCTOR_INIT(PR_FALSE, nsNSSCertificateDB, Init)
 nsNSSCertificateDB::Init()
  Observe(branch, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID, NS_LITERAL_STRING("enabled").get());

doesn't assume the pref is true on init, it just calls the observer code:

if (!strcmp(topic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {
 if (nsDependentString(data).EqualsLiteral("enabled")) {
  PRInt32 ocspEnabled;
  nsresult rv = prefBranch->GetIntPref("enabled", &ocspEnabled);
   if (NS_SUCCEEDED(rv))
    mOcspOn = ocspEnabled != 0;

which sets the pref to the initial pref value.

Your assumptions are wrong.

Comment 6

[Shawn Wilsher :sdwilsh]

No dice - Init can be called off of the main thread:

>	mozalloc.dll!mozalloc_abort(const char * const msg=0x06d9eef0)  Line 77	C++
 	xul.dll!Abort(const char * aMsg=0x06d9eef0)  Line 379 + 0xa bytes	C++
 	xul.dll!NS_DebugBreak_P(unsigned int aSeverity=0x00000001, const char * aStr=0x6800a9d0, const char * aExpr=0x6800a9c8, const char * aFile=0x6800a960, int aLine=0x00000367)  Line 366 + 0xc bytes	C++
 	xul.dll!nsPrefService::CheckAndLogBackgroundThreadUse()  Line 871 + 0x1b bytes	C++
 	xul.dll!nsPrefService::GetBranch(const char * aPrefRoot=0x6806f504, nsIPrefBranch * * _retval=0x06d9f384)  Line 286 + 0x5 bytes	C++
 	xul.dll!nsNSSCertificateDB::Init()  Line 113 + 0x36 bytes	C++
 	xul.dll!nsNSSCertificateDBConstructor(nsISupports * aOuter=0x00000000, const nsID & aIID={...}, void * * aResult=0x06d9f460)  Line 198 + 0xbd bytes	C++
 	xul.dll!mozilla::GenericFactory::CreateInstance(nsISupports * aOuter=0x00000000, const nsID & aIID={...}, void * * aResult=0x06d9f460)  Line 48 + 0x14 bytes	C++
 	xul.dll!nsComponentManagerImpl::CreateInstanceByContractID(const char * aContractID=0x68078ed4, nsISupports * aDelegate=0x00000000, const nsID & aIID={...}, void * * aResult=0x06d9f460)  Line 1284 + 0x25 bytes	C++
 	xul.dll!nsComponentManagerImpl::GetServiceByContractID(const char * aContractID=0x68078ed4, const nsID & aIID={...}, void * * result=0x06d9f4c8)  Line 1646 + 0x34 bytes	C++
 	xul.dll!CallGetService(const char * aContractID=0x68078ed4, const nsID & aIID={...}, void * * aResult=0x06d9f4c8)  Line 95	C++
 	xul.dll!nsGetServiceByContractID::operator()(const nsID & aIID={...}, void * * aInstancePtr=0x06d9f4c8)  Line 278 + 0x13 bytes	C++
 	xul.dll!nsCOMPtr<nsIX509CertDB>::assign_from_gs_contractid(const nsGetServiceByContractID gs={...}, const nsID & aIID={...})  Line 1252 + 0xf bytes	C++
 	xul.dll!nsCOMPtr<nsIX509CertDB>::operator=(const nsGetServiceByContractID rhs={...})  Line 714	C++
 	xul.dll!nsNSSCertificate::hasValidEVOidTag(SECOidTag & resultOidTag=SEC_OID_UNKNOWN, int & validEV=0x00000000)  Line 1006	C++
 	xul.dll!nsNSSCertificate::getValidEVOidTag(SECOidTag & resultOidTag=SEC_OID_UNKNOWN, int & validEV=0x00000000)  Line 1114 + 0x10 bytes	C++
 	xul.dll!nsNSSCertificate::GetIsExtendedValidation(int * aIsEV=0x06d9f86c)  Line 1140 + 0x13 bytes	C++
 	xul.dll!AuthCertificateCallback(void * client_data=0x00000000, PRFileDesc * fd=0x046c1d80, int checksig=0x00000001, int isServer=0x00000000)  Line 1017	C++
 	ssl3.dll!ssl3_HandleCertificate(sslSocketStr * ss=0x04327138, unsigned char * b=0x044162c6, unsigned int length=0x00000000)  Line 7899 + 0x21 bytes	C
 	ssl3.dll!ssl3_HandleHandshakeMessage(sslSocketStr * ss=0x04327138, unsigned char * b=0x04415f4c, unsigned int length=0x0000037a)  Line 8597 + 0x11 bytes	C
 	ssl3.dll!ssl3_HandleHandshake(sslSocketStr * ss=0x04327138, sslBufferStr * origBuf=0x04327390)  Line 8721 + 0x19 bytes	C
 	ssl3.dll!ssl3_HandleRecord(sslSocketStr * ss=0x04327138, SSL3Ciphertext * cText=0x06d9fa24, sslBufferStr * databuf=0x04327390)  Line 9060 + 0xd bytes	C
 	ssl3.dll!ssl3_GatherCompleteHandshake(sslSocketStr * ss=0x04327138, int flags=0x00000000)  Line 209 + 0x17 bytes	C
 	ssl3.dll!ssl_GatherRecord1stHandshake(sslSocketStr * ss=0x04327138)  Line 1258 + 0xb bytes	C
 	ssl3.dll!ssl_Do1stHandshake(sslSocketStr * ss=0x04327138)  Line 151 + 0xf bytes	C
 	ssl3.dll!ssl_SecureSend(sslSocketStr * ss=0x04327138, const unsigned char * buf=0x0441d4d8, int len=0x000002a7, int flags=0x00000000)  Line 1213 + 0x9 bytes	C
 	ssl3.dll!ssl_SecureWrite(sslSocketStr * ss=0x04327138, const unsigned char * buf=0x0441d4d8, int len=0x000002a7)  Line 1258 + 0x13 bytes	C
 	ssl3.dll!ssl_Write(PRFileDesc * fd=0x046c1d80, const void * buf=0x0441d4d8, int len=0x000002a7)  Line 1652 + 0x17 bytes	C
 	xul.dll!nsSSLThread::Run()  Line 1045 + 0x1c bytes	C++
 	xul.dll!nsPSMBackgroundThread::nsThreadRunner(void * arg=0x0466c3f8)  Line 45	C++
 	nspr4.dll!_PR_NativeRunThread(void * arg=0x04536430)  Line 426 + 0xf bytes	C
 	nspr4.dll!pr_root(void * arg=0x04536430)  Line 122 + 0xf bytes	C
 	msvcr90d.dll!_callthreadstartex()  Line 348 + 0xf bytes	C
 	msvcr90d.dll!_threadstartex(void * ptd=0x046b0e40)  Line 331	C
 	kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes	
 	ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
 	ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes

Comment 7

[Shawn Wilsher :sdwilsh]

Easily reproduced by running this test:
toolkit\mozapps\extensions\test\xpcshell\test_AddonRepository_cache.js

Comment 8

[Boris Zbarsky [:bzbarsky]]

Again, see comment 2.  That does involve setting up the initial value before Init, or ensuring Init() happens early enough on the main thread, which should be fine, I think.

Comment 9

[timeless]

Attachment: untested v2

ok, are people opposed to me just adding a do_GetService for NSSCertDB from the main thread somewhere in startup?

Comment 10

[Boris Zbarsky [:bzbarsky]]

YES!  We don't want NSS in the startup path.  It's slow to load an initialize.  The fact that we still have it there sometimes due to safebrowsing is a bug.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 482788 [details] [diff] [review]
untested v2

I don't really know when this code runs, so I don't think I can really provide you with any decent feedback here.  Maybe bz could (since he seems to know a bit)?

Comment 12

[timeless]

bz: i'm not sure if this is really that. it might be, but it might be the case that nss init is really one of the other things that already gets called by the time this would be called....

sorry, this isn't really a priority for me, so i'm only poking it a bit.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, I see.  You're making PSM init initialize the cert db in addition to whatever other parts of NSS it initializes....

That should be ok, generally speaking.

Comment 14

[Kai Engert (:KaiE:)]

Comment on attachment 482788 [details] [diff] [review]
untested v2

Thanks for the patch and the clarification.
The approach is OK, but I have some proposals:


(a)
(In reply to comment #5)
> NS_NSS_GENERIC_FACTORY_CONSTRUCTOR_INIT(PR_FALSE, nsNSSCertificateDB, Init)
>  nsNSSCertificateDB::Init()
>   Observe(branch, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID,
> NS_LITERAL_STRING("enabled").get());
> 
> ...
> Your assumptions are wrong.

Thanks for clarifying. We should make sure this can be more easily noticed by future readers of the code by adding a comment. I propose:

+ // Simulate a prefchange event, allowing the observer 
+ // to fetch and cache the initial value.
  Observe(branch, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID,
    NS_LITERAL_STRING("enabled").get());


(b)
+nsresult
+nsNSSCertificateDB::Init()
+{
+  nsCOMPtr<nsIPrefService> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
+  if (!prefs)
+    return NS_OK;
+
+  nsCOMPtr<nsIPrefBranch> ocspBranch;
+  prefs->GetBranch("security.OCSP.", getter_AddRefs(ocspBranch));
+  if (!ocspBranch)
+    return NS_OK;
+
+  nsCOMPtr<nsIPrefBranch2> branch(do_QueryInterface(ocspBranch));
+  if (!branch)
+    return NS_OK;


Shouldn't we return a failure error code in all above failure scenarios?
IMHO we should return failure if the Init doesn't work as expected.

(c)
+
+  PRBool mOcspOn;

Class nsNSSCertificateDB is declared as threadsafe.
Now you are introducing its first member variable, but you're not introducing any locks or other synchronization mechanism.

This means a race for mOcspOn will be possible.
I propose to play it safe.

You could use 
  PR_AtomicAdd(&mOcspOn, 0)
for reading the value and
  PR_AtomicSet(&mOcspOn, true/false)
for setting the value.

Comment 15

[Kai Engert (:KaiE:)]

(In reply to comment #14)
> 
> +
> +  PRBool mOcspOn;
> 
> Class nsNSSCertificateDB is declared as threadsafe.
> Now you are introducing its first member variable, but you're not introducing
> any locks or other synchronization mechanism.
> 
> This means a race for mOcspOn will be possible.
> I propose to play it safe.
> 
> You could use 
>   PR_AtomicAdd(&mOcspOn, 0)
> for reading the value and
>   PR_AtomicSet(&mOcspOn, true/false)
> for setting the value.


If you follow this advice, we should probably use type PRInt32 instead of PRBool for mOcspOn.

(The alternative is to introduce a new PRLock, but IMHO that's overkill.)

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> You could use 
>   PR_AtomicAdd(&mOcspOn, 0)
> for reading the value and
>   PR_AtomicSet(&mOcspOn, true/false)
> for setting the value.
I've been told in the past that we shouldn't do that because we have no atomic read methods.  Got to use a mutex.

Comment 17

[timeless]

Attachment: untested v3 with comment for (a)

i don't consider failure of the oscp pref to be fatal or interesting. the code will work without getting the pref and it defaults to on.

Worst cases:
1. we don't load prefs in which case the user pays for OCSP even though the user doesn't want it - oh well, still secure
2. the user is actively twiddling the OCSP bit in which case there's a potential race while the user is twiddling it, this isn't interesting, the user has no way of knowing if the connection started before or after the pref was twiddled and thus the user can't prove we did anything wrong. An action created long after the OCSP pref twiddle will do the right thing.

I'm willing to add a comment if someone writes it...

Comment 18

[Kai Engert (:KaiE:)]

Comment on attachment 492169 [details] [diff] [review]
untested v3 with comment for (a)

Imagine someone in the future decides the compile time default of OCSP should be "false = off". That might be fully acceptable for some applications. Or maybe the Gecko maintainers decide to do it in 10 years, because by then there might be a newer cooler technology available.

However, a user decides to override that compile time default, the user now has a configuration value of "true". Or imagine a XUL application ships a prefs.js that changes the default pref.

Now the application starts up. What could happen?

If the code fails to start the pref service (e.g. of out-of-memory), then the pref remains at the "unsafe" value. That's why I ask the service should return failure if pref service construction fails; because you are introducing code that depends on the preference cache to be correct.

r- because of the above. Please return failure in ::Init.


Regarding the need of synchronization for mOcspOn:

I can't fully disprove your arguments for today's situation. Yes, we have only one place where mOcspOn gets set after init. Yes, the effect of an overlapping assignment and reading "might" be limited to a minor delay until the new pref value is effective.

However, I'm still worried, partly because of processor architectures where multi-byte writes might not be ordered. Maybe some weird compiler will create assembler code that will read, and xor the existing value, in order to get it to a different value?

I think we shouldn't have to discuss whether synchronization is necessary. It should be good practice to play safe and avoid potential risks whenever variables are known to be shared.

Comment 19

[timeless]

kaie: this is an important bug. for 4.0. not for 10000.12562.262115.

there is a standard guarantee that natural size assignments are atomic and will be. any cpu which fails this is broken. I'm fairly certain brendan will concur.

99.9999% of users don't give an * about OCSP. They do want their browser to start. They are not likely to debug why their browser doesn't. If it doesn't start, they'll get another browser.

We could add a comment to the member variable saying "this must only be set on the main thread" if that helps.

If an app needs ocsp to be in a certain state in order to function, it's free to check the state of ocsp after the app has started. When it discovers ocsp is not in the state it likes, it can either show the user an error message or try to twiddle the pref. -- during startup neither crashing nor showing the user an incomprehensible dialog are welcome.

Also, startup is perf sensitive, so adding useless locking for a case which can't happen today is insane.

Comment 20

[Kai Engert (:KaiE:)]

This is not about OCSP. I'm requesting that OCSP should be guaranteed to work. In fact, by default we fail gracefully on any OCSP failure.

I request, if you add code that makes assumptions, that these assumptions are being enforced, and if the assumptions fail, that the code fails.

You're adding code that introduces a dependency on the pref service to be available. If it's not, the assumptions for the state of your variables are not met and that code shouldn't run.

Comment 21

[Kai Engert (:KaiE:)]

(In reply to comment #20)
> This is not about OCSP. I'm 

not

> requesting that OCSP should be guaranteed to work.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: Patch v4

Here's v3 + my proposed changes.
Now we need a new reviewer.

Comment 23

[Kai Engert (:KaiE:)]

Comment on attachment 496595 [details] [diff] [review]
Patch v4

Honza, could you please review?

This patch is mostly from timeless, which I had reviewed, but then I did additional work.

If you want to focus on the new changes made by me, you could limit your review to the interdiff between v3 and v4.

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 496595 [details] [diff] [review]
Patch v4

- please rename 'mutex' to 'mMutex'
- init mMutex in Init, not in the constructor and probably fail Init on mutex init failure? (though, after we have infalliable malloc we won't have to do any checks => so, up to you on this point)
- the observation mechanism doesn't work this way, I'll attach an update to make it work
- shouldn't Observe return failure when the pref could not be read/found and shouldn't this be checked in Init?

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: update to make pref observer work

Comment 26

[Shawn Wilsher :sdwilsh]

Doesn't block anymore since the preferences changes we wanted have been moved to bug 619487 and will not assert.  We'd likely approve a patch though.

Comment 27

[Kaspar]

Note that after this commit here:

  http://hg.mozilla.org/mozilla-central/rev/d2856d5970b6

this is getting a real issue with PSM - it breaks TLS client auth, among other. Here's a stack showing what happens when NSS is calling PSM, to ask for a client cert:

  nsPrefService::CheckAndLogBackgroundThreadUse
  nsPrefBranch::GetCharPref
  nsPrefService::GetCharPref
  nsGetUserCertChoice
  nsNSS_SSLGetClientAuthData
  ssl3_HandleCertificateRequest
  [...]

What happens is that

  pref->GetCharPref("security.default_personal_cert", &mode);

in nsNSSIOLayer.cpp:nsGetUserCertChoice fails (GetCharPref returns NS_ERROR_UNEXPECTED), which means that nsNSS_SSLGetClientAuthData won't ever supply a client cert to NSS (irrespective of whether security.default_personal_cert is set to "Ask Every Time" or "Select Automatically").

Comment 28

[Kaspar]

Hmm, looking again at the stack in comment 27, I realize that this isn't really an issue with nsNSSCertificateDB, but with nsNSSIOLayer... so it's actually better to remove the dependency of bug 624075 (which I previously added) and treat that one as a separate issue - which needs a separate fix, I guess.

(There's another stack, with nsNSSCertificateDB::GetIsOcspOn, which now produces the "Invalid use of the preferences on a background thread!" messages in the console, that's why I got to this bug, actually. nsNSSCertificateDB::GetIsOcspOn probably returns "random" results right now, as ocspEnabled isn't explictly initialized, and GetIsOcspOn checks for "ocspEnabled == 0" after the GetIntPref call - cf. http://mxr.mozilla.org/mozilla-central/ident?i=GetIsOcspOn)

Comment 29

[Josh Matthews [:jdm]]

We should be avoiding the addition of new PR_Lock pointer members and using mozilla::Mutex members as much as possible.

Comment 30

[Kai Engert (:KaiE:)]

Attachment: Patch v5

Comment 31

[Kai Engert (:KaiE:)]

Attachment: Patch v6

Comment 32

[Kai Engert (:KaiE:)]

Comment on attachment 502554 [details] [diff] [review]
Patch v6

(In reply to comment #24)
> 
> - please rename 'mutex' to 'mMutex'

done


> - the observation mechanism doesn't work this way, I'll attach an update to
> make it work

thanks, new patch uses your code


> - shouldn't Observe return failure when the pref could not be read/found and
> shouldn't this be checked in Init?

I agree. Changed.


> - init mMutex in Init, not in the constructor and probably fail Init on mutex
> init failure? (though, after we have infalliable malloc we won't have to do any
> checks => so, up to you on this point)

no longer applies, because converted to mozilla::Mutex


(In reply to comment #29)
> We should be avoiding the addition of new PR_Lock pointer members and using
> mozilla::Mutex members as much as possible.

Thanks for educating me, code converted.

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 502554 [details] [diff] [review]
Patch v6

r=honzab

Comment 34

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

I am working on a new patch that fixes this issue more generally.

Comment 35

[Kai Engert (:KaiE:)]

Brian, should this patch land anyway?

I'm confused by the status "blocking2.0 = .x"

I've been told on IRC that it's supposed to mean "after 4.0".
Is that true?
You want us to delay this fix until after FF 4.0 final shipped?

Comment 36

[Boris Zbarsky [:bzbarsky]]

.x means that we won't block 4.0 for it but should block 4.next or so (whatever that means).

If a patch is ready before 4.0 and approved, it should just land, I'd think.

Comment 37

[Honza Bambas (:mayhemer)]

This blocks betaN+ blocker bug 	619487 which I don't understand why is landing so late in the cycle.  It's not a small change IMO.

Comment 38

[Josh Matthews [:jdm]]

Brian, did anything ever happen with your "more general" patch?

Comment 39

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

This will be fixed by the patch to bug 714477.

Comment 40

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

Comment on attachment 502554 [details] [diff] [review]
Patch v6

Instead of this patch, we should fix this by fixing bug 714477 instead.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to fix by switching to AddIntVarCache

I am *so* tired of the constant stream of assertions in any long-running debug build resulting from this bug leading to corruption of the recursion level on the pref hash table.

This patch should be an clear win even without the threadsafety issue; it switches to using the new preferences API, which makes this getter much faster.  Adding a single cache (since the nsNSSCertificateDB is used as a service, so there should be only one of them) is fine.  If the cache gets updated from another thread around the same time as the getter runs -- the getter might get the old value or might get the new value; writes to a 32-bit int should, I believe, be atomic.  If whether it gets the new value or the old value when the pref changes dynamically is an actual problem for some reason, we have bigger problems.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 703857 [details] [diff] [review]
patch to fix by switching to AddIntVarCache

Actually, this isn't quite sufficient since the nsNSSCertificateDB is constructed off the main thread.

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to fix by switching to AddIntVarCache

Comment 44

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

This was fixed by the patch for bug 714477.