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

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

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

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

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.

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.

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

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

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.

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

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 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)?

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.

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 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.

(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.)

(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.

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 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.

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.

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.

(In reply to comment #20) > This is not about OCSP. I'm not > requesting that OCSP should be guaranteed to work.

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

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 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?

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.

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").

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)

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

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 on attachment 502554 [details] [diff] [review] Patch v6 r=honzab

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

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?

.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.

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.

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

This will be fixed by the patch to bug 714477.

Comment on attachment 502554 [details] [diff] [review] Patch v6 Instead of this patch, we should fix this by fixing bug 714477 instead.

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 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.

This was fixed by the patch for bug 714477.