619201 - nsCertOverrideService needs to initialize on the main thread to use the directory service

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Josh Matthews [:jdm]]

Running Fennec on a restricted wireless network currently brings up an SSL cert error which triggers this assertion:

#0  RealBreak () at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsDebugImpl.cpp:407
#1  0x0258d684 in Break (aMsg=
    0xb00fe35c "###!!! ASSERTION: mozHunspellDirProvider not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /home/t_mattjo/src/firefox/mobilebase/extensions/spellcheck/hunspell/src/mozHunspe"...) at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsDebugImpl.cpp:503
#2  0x0258d649 in NS_DebugBreak_P (aSeverity=1, aStr=0x30169e8 "mozHunspellDirProvider not thread-safe", aExpr=0x30169b4 "_mOwningThread.GetThread() == PR_GetCurrentThread()", aFile=
    0x30168cc "/home/t_mattjo/src/firefox/mobilebase/extensions/spellcheck/hunspell/src/mozHunspellDirProvider.cpp", aLine=45) at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsDebugImpl.cpp:371
#3  0x022f7367 in mozHunspellDirProvider::AddRef (this=0xb40e01c0) at /home/t_mattjo/src/firefox/mobilebase/extensions/spellcheck/hunspell/src/mozHunspellDirProvider.cpp:43
#4  0x02508df3 in NS_TableDrivenQI (aThis=0xb40e01c0, entries=0x328db20, aIID=..., aInstancePtr=0xb00fe86c) at nsISupportsImpl.cpp:49
#5  0x022f75ec in mozHunspellDirProvider::QueryInterface (this=0xb40e01c0, aIID=..., aInstancePtr=0xb00fe86c)
    at /home/t_mattjo/src/firefox/mobilebase/extensions/spellcheck/hunspell/src/mozHunspellDirProvider.cpp:43
#6  0x025053c9 in nsQueryInterface::operator() (this=0xb00fe884, aIID=..., answer=0xb00fe86c) at nsCOMPtr.cpp:47
#7  0x00ff5136 in nsCOMPtr<nsIDirectoryServiceProvider>::assign_from_qi (this=0xb00fe8c8, qi=..., aIID=...) at ../../dist/include/nsCOMPtr.h:1212
#8  0x00ff494c in nsCOMPtr<nsIDirectoryServiceProvider>::nsCOMPtr (this=0xb00fe8c8, qi=...) at ../../dist/include/nsCOMPtr.h:595
#9  0x02548f6a in FindProviderFile (aElement=0xb40e01c0, aData=0xb00fe974) at /home/t_mattjo/src/firefox/mobilebase/xpcom/io/nsDirectoryService.cpp:433
#10 0x02536465 in nsSupportsArray::EnumerateBackwards (this=0xb7dd0d00, aFunc=0x2548d18 <FindProviderFile(nsISupports*, void*)>, aData=0xb00fe974)
    at /home/t_mattjo/src/firefox/mobilebase/xpcom/ds/nsSupportsArray.cpp:639
#11 0x0254922a in nsDirectoryService::Get (this=0xb7de5100, prop=0x2f3440c "ProfD", uuid=..., result=0xabb2d0cc) at /home/t_mattjo/src/firefox/mobilebase/xpcom/io/nsDirectoryService.cpp:468
#12 0x00fea1df in NS_GetSpecialDirectory (specialDirName=0x2f3440c "ProfD", result=0xabb2d0cc) at ../../dist/include/nsDirectoryServiceUtils.h:58
#13 0x01fc5f97 in nsCertOverrideService::Init (this=0xabb2d0b0) at /home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsCertOverrideService.cpp:152
#14 0x01ff0881 in nsCertOverrideServiceConstructor (aOuter=0x0, aIID=..., aResult=0xb00feb38) at /home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsNSSModule.cpp:259
#15 0x02514d28 in mozilla::GenericFactory::CreateInstance (this=0xae173750, aOuter=0x0, aIID=..., aResult=0xb00feb38) at GenericFactory.cpp:48
#16 0x0257244f in nsComponentManagerImpl::CreateInstanceByContractID (this=0xb7dc3220, aContractID=0x2f3c3d4 "@mozilla.org/security/certoverride;1", aDelegate=0x0, aIID=..., aResult=0xb00feb38)
    at /home/t_mattjo/src/firefox/mobilebase/xpcom/components/nsComponentManager.cpp:1315
#17 0x025730c2 in nsComponentManagerImpl::GetServiceByContractID (this=0xb7dc3220, aContractID=0x2f3c3d4 "@mozilla.org/security/certoverride;1", aIID=..., result=0xb00fec2c)
    at /home/t_mattjo/src/firefox/mobilebase/xpcom/components/nsComponentManager.cpp:1676
#18 0x025063a2 in CallGetService (aContractID=0x2f3c3d4 "@mozilla.org/security/certoverride;1", aIID=..., aResult=0xb00fec2c) at nsComponentManagerUtils.cpp:94
#19 0x02506890 in nsGetServiceByContractID::operator() (this=0xb00fec44, aIID=..., aInstancePtr=0xb00fec2c) at nsComponentManagerUtils.cpp:278
#20 0x01fdfb48 in nsCOMPtr<nsICertOverrideService>::assign_from_gs_contractid (this=0xb00fed70, gs=..., aIID=...) at ../../../../dist/include/nsCOMPtr.h:1252
#21 0x01fdf504 in nsCOMPtr<nsICertOverrideService>::nsCOMPtr (this=0xb00fed70, gs=...) at ../../../../dist/include/nsCOMPtr.h:627
#22 0x01fe9efa in nsNSSBadCertHandler (arg=0xb058f710, sslSocket=0xae1bf0e0) at /home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsNSSIOLayer.cpp:3519
#23 0x0036aaa6 in ssl3_HandleCertificate (ss=0xae162000, b=
    0xae1a0388 "\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245\245"..., length=0)
    at ssl3con.c:7911
#24 0x0036c49a in ssl3_HandleHandshakeMessage (ss=0xae162000, b=0xae1a0004 "", length=900) at ssl3con.c:8603
#25 0x0036c928 in ssl3_HandleHandshake (ss=0xae162000, origBuf=0xae162250) at ssl3con.c:8727
#26 0x0036d40a in ssl3_HandleRecord (ss=0xae162000, cText=0xb00ff08c, databuf=0xae162250) at ssl3con.c:9066
#27 0x0036e527 in ssl3_GatherCompleteHandshake (ss=0xae162000, flags=0) at ssl3gthr.c:209
#28 0x0037117b in ssl_GatherRecord1stHandshake (ss=0xae162000) at sslcon.c:1258
#29 0x0037c824 in ssl_Do1stHandshake (ss=0xae162000) at sslsecur.c:151
#30 0x0037ea68 in ssl_SecureSend (ss=0xae162000, buf=
    0xae110000 "GET /en-US/mobile/api/1.5/list/featured/all/4/Linux/4.0b3pre HTTP/1.1\r\nHost: services.addons.mozilla.org\r\nUser-Agent: Mozilla/5.0 (X11; Linux i686; rv:2.0b8pre) Gecko/20101213 Firefox/4.0b8pre Fennec/"..., len=631, flags=0) at sslsecur.c:1213
#31 0x0037ec22 in ssl_SecureWrite (ss=0xae162000, buf=
    0xae110000 "GET /en-US/mobile/api/1.5/list/featured/all/4/Linux/4.0b3pre HTTP/1.1\r\nHost: services.addons.mozilla.org\r\nUser-Agent: Mozilla/5.0 (X11; Linux i686; rv:2.0b8pre) Gecko/20101213 Firefox/4.0b8pre Fennec/"..., len=631) at sslsecur.c:1258
#32 0x0038628f in ssl_Write (fd=0xae1bf0e0, buf=0xae110000, len=631) at sslsock.c:1652
#33 0x01fccf3c in nsSSLThread::Run (this=0xb01705e0) at /home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsSSLThread.cpp:1045
#34 0x01fcb26b in nsPSMBackgroundThread::nsThreadRunner (arg=0xb01705e0) at /home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsPSMBackgroundThread.cpp:44
---Type <return> to continue, or q <return> to quit---
#35 0x00182797 in _pt_root (arg=0xb037fa80) at /home/t_mattjo/src/firefox/mobilebase/nsprpub/pr/src/pthreads/ptthread.c:187
#36 0x008ba925 in start_thread () from /lib/libpthread.so.0
#37 0x007e407e in clone () from /lib/libc.so.6
(gdb) fr 13
#13 0x01fc5f97 in nsCertOverrideService::Init (this=0xabb2d0b0) at /home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsCertOverrideService.cpp:152
152	  NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mSettingsFile));
(gdb) list
147	
148	  mDottedOidForStoringNewHashes = dotted_oid;
149	  PR_smprintf_free(dotted_oid);
150	
151	  // cache mSettingsFile
152	  NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mSettingsFile));
153	  if (mSettingsFile) {
154	    mSettingsFile->AppendNative(NS_LITERAL_CSTRING(kCertOverrideFileName));
155	  }
156

Comment 1

[Benjamin Smedberg]

Yeah, quite clearly nsCertOverrideService::Init should not be calling into the directory service off the main thread. This can be a cause of crashes. The possibilities are:

* We should pre-create nsCertOverrideService on the main thread
* We should not be creating the nsCertOverrideService off the main thread
* ?

Who knows the code?

Comment 2

[timeless]

Attachment: add to the ball of wax

Comment 3

[Kai Engert (:KaiE:)]

Comment on attachment 497777 [details] [diff] [review]
add to the ball of wax

Is it sufficent to create the service once, because XPCOM will keep it alive?
If yes, r=kaie

Or will the service be deleted on leaving the scope, and will need to be constructed again later?
If yes, r- and please add a member variable nsNSSComponent::mCertOverrServ to keep a reference.

Comment 4

[Kai Engert (:KaiE:)]

question has been answered, once a service is alive, XPCOM will keep it alive until shutdown.

patch is sufficient.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 497777 [details] [diff] [review]
add to the ball of wax

If Fennec wants this, Fennec drivers can ask for it.

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/999905af7783

Comment 7

[Johnny Stenback (:jst)]

Backed out of the beta channel due to bug 650858.

http://hg.mozilla.org/releases/mozilla-beta/rev/279072c1136c
http://hg.mozilla.org/releases/mozilla-beta/rev/19e59b25e886

Comment 8

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

If we implement the design in bug 511393 comment 14, then we can make nsICertOverrideService main-thread only, which will provide an alternate solution for this bug. 

Besides causing bug 650858, the current solution to this bug has the added disadvantage of adding disk I/O on the main thread at startup. So, my recommendation is to backout this patch on all trees (including mozilla-central) and then fix bug 511393.

If backing out the patch is problematic (i.e. we have some evidence that it really does cause crashes), then I will hack together an alternate fix for this bug that avoids causing bug 650858.

Comment 9

[LegNeato]

I backed this out of beta6 too:

   http://hg.mozilla.org/releases/mozilla-beta/rev/a9bc7596542b

So bug 650858 doesn't affect it anymore. I have not backed it out of aurora or central.

Comment 10

[LegNeato]

Also backed out of aurora7:

  http://hg.mozilla.org/releases/mozilla-aurora/rev/699d996d5e5d

Comment 11

[u279076]

qa- as QA does not need to verify this fix.

Comment 12

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

Also backed out of beta-8:
https://hg.mozilla.org/releases/mozilla-beta/rev/dc9713bcaa93

The patch for bug 679140 fixes this in a different way, by ensuring that we only use the cert override service on the main thread, so that it is guaranteed to be initialized on the main thread without having to do so in nsNSSComponent::Init().