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

RESOLVED FIXED in Firefox 8

Status

()

Core
Security: PSM
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jdm, Assigned: timeless)

Tracking

unspecified
mozilla7
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox6 wontfix, firefox7 wontfix, firefox8+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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
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?
Component: Security → Security: PSM
QA Contact: toolkit → psm
(Reporter)

Updated

7 years ago
Summary: ASSERTION: mozHunspellDirProvider not thread-safe → nsCertOverrideService uses directory service off the main thread
(Assignee)

Updated

7 years ago
Summary: nsCertOverrideService uses directory service off the main thread → nsCertOverrideService needs to initialize on the main thread to use the directory service
(Assignee)

Comment 2

7 years ago
Created attachment 497777 [details] [diff] [review]
add to the ball of wax
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #497777 - Flags: review?(kaie)
Attachment #497777 - Flags: approval2.0?

Comment 3

7 years ago
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.
Attachment #497777 - Flags: review?(kaie) → review+

Comment 4

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

patch is sufficient.
Comment on attachment 497777 [details] [diff] [review]
add to the ball of wax

If Fennec wants this, Fennec drivers can ask for it.
Attachment #497777 - Flags: approval2.0? → approval2.0-
http://hg.mozilla.org/mozilla-central/rev/999905af7783
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Depends on: 650892
Depends on: 650858
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
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

6 years ago
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.
Target Milestone: mozilla5 → mozilla7

Comment 10

6 years ago
Also backed out of aurora7:

  http://hg.mozilla.org/releases/mozilla-aurora/rev/699d996d5e5d
status-firefox6: --- → unaffected
status-firefox7: --- → unaffected
status-firefox8: --- → affected
tracking-firefox8: --- → +

Updated

6 years ago
status-firefox6: unaffected → wontfix
status-firefox7: unaffected → wontfix
status-firefox8: affected → fixed
qa- as QA does not need to verify this fix.
Whiteboard: [qa-]
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().
You need to log in before you can comment on or make changes to this bug.