1145893 - nsCertVerificationThread calls NS_DispatchToMainThread during xpcom shutdown

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Eric Rahm [:erahm]]

nsCertVerificationThread uses |NS_DispatchToMainThread| [1] during xpcom shutdown.

This can be reproduced when running the TestCertDB unit test, a gdb stack trace pretty clearly shows how this happens:

> 174	  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(thread->Dispatch(aEvent, aDispatchFlags)));
> (gdb) bt
> #0  0x00007ffff39f082b in NS_DispatchToMainThread (aEvent=0x7fffe59193c0, aDispatchFlags=0)
>    at /home/erahm/dev/mozilla-central/xpcom/glue/nsThreadUtils.cpp:174
> #1  0x00007ffff4eda8c2 in nsCertVerificationThread::Run (this=0x7fffe7a68300)
>    at /home/erahm/dev/mozilla-central/security/manager/ssl/src/nsCertVerificationThread.cpp:139
> #2  0x00007ffff79a6268 in _pt_root (arg=0x7fffe9e30100) at /home/erahm/dev/mozilla-central/nsprpub/pr/src/pthreads>/ptthread.c:212
> #3  0x00007ffff7bc4182 in start_thread (arg=0x7fffe3863700) at pthread_create.c:312
> #4  0x00007ffff270a47d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
> 
> (gdb) thread 1
> [Switching to thread 1 (Thread 0x7ffff7f22a80 (LWP 31393))]
> #0  __lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> 135   ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S: No such file or directory.
> (gdb) bt
> #0  __lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> #1  0x00007ffff7bc668d in _L_lock_1082 () from /lib/x86_64-linux-gnu/libpthread.so.0
> #2  0x00007ffff7bc6607 in __GI___pthread_mutex_lock (mutex=0x7fffe430ed40) at ../nptl/pthread_mutex_lock.c:134
> #3  0x00007ffff79a0a49 in PR_Lock (lock=0x7fffe430ed40) at /home/erahm/dev/mozilla-central/nsprpub/pr/src/pthreads/ptsynch.c:177
> #4  0x00007ffff4efac3b in BaseAutoLock (aLock=..., this=0x7fffffffdb98) at ../../../../dist/include/mozilla/Mutex.h:164
> #5  nsPSMBackgroundThread::requestExit (this=0x7fffe7a68300)
>     at /home/erahm/dev/mozilla-central/security/manager/ssl/src/nsPSMBackgroundThread.cpp:81
> #6  0x00007ffff4ed2846 in nsNSSComponent::deleteBackgroundThreads (this=this@entry=0x7fffe7a67d60)
>     at /home/erahm/dev/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:239
> #7  0x00007ffff4ed3e1e in nsNSSComponent::~nsNSSComponent (this=0x7fffe7a67d60, __in_chrg=<optimized out>)
>     at /home/erahm/dev/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:265
> #8  0x00007ffff4ed3f03 in nsNSSComponent::~nsNSSComponent (this=0x7fffe7a67d60, __in_chrg=<optimized out>)
>     at /home/erahm/dev/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:280
> #9  0x00007ffff4ed3f39 in nsNSSComponent::Release (this=0x7fffe430ed40)
>     at /home/erahm/dev/mozilla-central/security/manager/ssl/src/nsNSSComponent.cpp:1226
> #10 0x00007ffff39d2e9f in operator= (aRhs=0x0, this=0x7fffe9ed3eb8) at ../../dist/include/nsCOMPtr.h:850
> #11 FreeFactoryEntries (aCID=..., aEntry=0x7fffe9ed3ea0, aArg=<optimized out>)
>     at /home/erahm/dev/mozilla-central/xpcom/components/nsComponentManager.cpp:1222
> #12 0x00007ffff39d63a4 in nsBaseHashtable<nsIDHashKey, nsFactoryEntry*, nsFactoryEntry*>::s_EnumReadStub (aTable=<optimized out>,
>     aHdr=<optimized out>, aNumber=<optimized out>, aArg=<optimized out>) at ../../dist/include/nsBaseHashtable.h:391
> #13 0x00007ffff39f0bd1 in Enumerate (aArg=<optimized out>, aEtor=<optimized out>, this=0x7fffe9e46370)
>     at /home/erahm/dev/mozilla-central/xpcom/glue/pldhash.cpp:809
> #14 PL_DHashTableEnumerate (aTable=0x7fffe9e46370,
>     aEtor=0x7ffff39d6392 <nsBaseHashtable<nsIDHashKey, nsFactoryEntry*, nsFactoryEntry*>::s_EnumReadStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, aArg=0x7fffffffdc90) at /home/erahm/dev/mozilla-central/xpcom/glue/pldhash.cpp:860
> #15 0x00007ffff39d3659 in EnumerateRead (aUserArg=0x0,
>     aEnumFunc=0x7ffff39d2e85 <FreeFactoryEntries(nsID const&, nsFactoryEntry*, void*)>, this=<optimized out>)
>     at ../../dist/include/nsBaseHashtable.h:176
> #16 nsComponentManagerImpl::FreeServices (this=<optimized out>)
>     at /home/erahm/dev/mozilla-central/xpcom/components/nsComponentManager.cpp:1236
> #17 0x00007ffff39f61c8 in mozilla::ShutdownXPCOM (aServMgr=<optimized out>)
>     at /home/erahm/dev/mozilla-central/xpcom/build/XPCOMInit.cpp:918
> #18 0x0000000000405c1a in main (argc=<optimized out>, argv=<optimized out>)
>     at /home/erahm/dev/mozilla-central/security/manager/ssl/tests/compiled/TestCertDB.cpp:35

[1] https://hg.mozilla.org/mozilla-central/annotate/4d2d97b3ba34/security/manager/ssl/src/nsCertVerificationThread.cpp#l68

Comment 1

[Benjamin Smedberg]

It appears at first glance that nsNSSComponent::deleteBackgroundThreads and the other important shutdown tasks ought to occur at xpcom-shutdown, instead of triggered by the destructor: http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#1304

And we should be asserting that all threads are actually shut down by the end of xpcom-shutdown-threads: it's clear that the cert verification thread is still alive after that, and that assert will be more informative than this one which happens later.

Comment 2

[Eric Rahm [:erahm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> It appears at first glance that nsNSSComponent::deleteBackgroundThreads and
> the other important shutdown tasks ought to occur at xpcom-shutdown, instead
> of triggered by the destructor:
> http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/
> nsNSSComponent.cpp#1304

Adding |deleteBackgroundThreads| to that block does seem to have fixed the unit test crash. I'll do a try push and see what turns up.

> And we should be asserting that all threads are actually shut down by the
> end of xpcom-shutdown-threads: it's clear that the cert verification thread
> is still alive after that, and that assert will be more informative than
> this one which happens later.

Which part of the codebase are you referring to? 'xpcom-shutdown-threads' and various aliases show up in several spots, perhaps in mozilla::ShutdownXPCOM [1]? How would you propose we assert that all threads are shutdown? Or are you just referring to the cert verification thread?

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/build/XPCOMInit.cpp?from=XPCOMInit.cpp:918&case=true#868

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 1: Shutdown nsNSSComponent background threads during xpcom-shutdown

Call |deleteBackgroundThreads| during xpcom-shutdown rather than during destruction.

Comment 4

[Benjamin Smedberg]

Comment on attachment 8581828 [details] [diff] [review]
Part 1: Shutdown nsNSSComponent background threads during xpcom-shutdown

I am not a PSM peer.

In terms of shutdown-threads, I'm in particular talking about at http://hg.mozilla.org/mozilla-central/annotate/bc85c479668a/xpcom/build/XPCOMInit.cpp#l866 asserting that only the main thread exists in the thread manager after that point.

Comment 5

[Eric Rahm [:erahm]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> In terms of shutdown-threads, I'm in particular talking about at
> http://hg.mozilla.org/mozilla-central/annotate/bc85c479668a/xpcom/build/
> XPCOMInit.cpp#l866 asserting that only the main thread exists in the thread
> manager after that point.

That sounds reasonable. Side note: AFAICT that wouldn't have helped in this case as this was not an nsThread, but it's possible I'm missing something.

Comment 6

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #5)
> That sounds reasonable.

Sorry I take that back, we actually shut down the thread manager a few lines later [1]. I don't think we should be asserting prior to that.

[1] http://hg.mozilla.org/mozilla-central/annotate/bc85c479668a/xpcom/build/XPCOMInit.cpp#l878

Comment 7

[Benjamin Smedberg]

Sure, the point is that we should assert no active threads then, not only asserting on calls to NS_Dispatch*

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8581828 [details] [diff] [review]
Part 1: Shutdown nsNSSComponent background threads during xpcom-shutdown

Review of attachment 8581828 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should also assert(!mCertVerificationThread) in ~nsNSSComponent instead of calling deleteBackgroundThreads there (unless I'm misunderstanding what the goal is here). r=me with that.

Comment 9

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=446209444b12

Comment 10

[Eric Rahm [:erahm]]

(see comment 9, we only care about the cpp tests in that run)

Comment 11

[Eric Rahm [:erahm]]

Attachment: Shutdown nsNSSComponent background threads during xpcom-shutdown

Comment 12

[Eric Rahm [:erahm]]

Comment on attachment 8617474 [details] [diff] [review]
Shutdown nsNSSComponent background threads during xpcom-shutdown

Updated w/ assertion, carrying forward r+

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8999e3228a19

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8999e3228a19