379661 - ###!!! ASSERTION: nsSMimeVerificationListener not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file C:/mozilla/mailnews/mime/src/mimecms.cpp, line 273

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[[e-mail]]

I was reading a message with signed by a certificate issued by an unknown CA.

Note that for whatever reason I didn't get the assert in a few weeks old build.

NS_DebugBreak_P(unsigned int 1, const char * 0x04539cb8, const char * 0x04539c84, const char * 0x04539c58, int 273) line 350 + 12 bytes
nsSMimeVerificationListener::Release(nsSMimeVerificationListener * const 0x06c4fbc8) line 273 + 110 bytes
nsCOMPtr<nsISMimeVerificationListener>::~nsCOMPtr<nsISMimeVerificationListener>() line 584
nsSMimeVerificationJob::~nsSMimeVerificationJob() line 88 + 50 bytes
nsSMimeVerificationJob::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsCertVerificationThread::Run() line 154 + 30 bytes
nsPSMBackgroundThread::nsThreadRunner(void * 0x048bb980) line 45
_PR_NativeRunThread(void * 0x04aa5448) line 436 + 13 bytes
pr_root(void * 0x04aa5448) line 122 + 13 bytes
_threadstartex(void * 0x04aa56d0) line 212 + 13 bytes

Comment 1

[[e-mail]]

Subsequent assertion:

###!!! ASSERTION: PipUIContext not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file C:/mozilla/security/manager/ssl/src/nsNSSComponent.cpp, line 2378

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: both normal and full backtraces

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Confirming. Stumbled on both assertions when switching from an IMAP folder to a newsgroup with new unread messages, and clicking on one unread newsgroup message.

However, strangely, my backtrace on gdb looks different even though it trapped the assertion.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: first backtrace

Better backtrace (normal and full) for comment #0.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: second backtrace

Better backtrace (normal and full) for comment #1.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Just received an email from IMAP exchange on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090210 Lightning/1.0pre Shredder/3.0b2pre:

###!!! ASSERTION: nsSMimeVerificationListener not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/skywalker/comm-central/mailnews/mime/src/mimecms.cpp, line 276
nsGetServiceByCIDWithError::nsGetServiceByCIDWithError(nsID const&, unsigned int*)+0x000035C3 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libmime.dylib +0x00045857]
nsDeque::GetSize() const+0x00000136 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x0000C088]
nsDeque::GetSize() const+0x000001ED [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x0000C13F]
nsAutoLock::~nsAutoLock()+0x000026B9 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x0000B943]
nsAutoMonitor::~nsAutoMonitor()+0x00003479 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x00008EAF]
PR_Select+0x00000491 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/libnspr4.dylib +0x000292CD]
_pthread_start+0x00000141 [/usr/lib/libSystem.B.dylib +0x00032095]
thread_start+0x00000022 [/usr/lib/libSystem.B.dylib +0x00031F52]

Obsoleting my stacktraces.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This comes immediately after the previous stacktrace in comment #6:


###!!! ASSERTION: PipUIContext not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/skywalker/comm-central/mozilla/security/manager/ssl/src/nsNSSComponent.cpp, line 2733
nsAutoLock::unlock()+0x00005AA3 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x00017B8D]
nsGetInterface::nsGetInterface(nsISupports*, unsigned int*)+0x00000380 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x00011EEA]
nsGetServiceByCID::nsGetServiceByCID(nsID const&)+0x00006CE5 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x00055721]
nsGetServiceByCID::nsGetServiceByCID(nsID const&)+0x000068F9 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x00055335]
nsDeque::GetSize() const+0x000000F2 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x0000C044]
nsDeque::GetSize() const+0x000001FB [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x0000C14D]
nsAutoLock::~nsAutoLock()+0x000026B9 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x0000B943]
nsAutoMonitor::~nsAutoMonitor()+0x00003479 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/libpipnss.dylib +0x00008EAF]
PR_Select+0x00000491 [/Users/skywalker/objdir-tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/libnspr4.dylib +0x000292CD]
_pthread_start+0x00000141 [/usr/lib/libSystem.B.dylib +0x00032095]
thread_start+0x00000022 [/usr/lib/libSystem.B.dylib +0x00031F52]

Comment 8

[Kai Engert [:KaiE:]]

PipUIContext doesn't contain any state (no member variables).

I guess it's OK to change file nsNSSComponent.cpp and
  NS_IMPL_ISUPPORTS1(PipUIContext, nsIInterfaceRequestor)
to use
  NS_IMPL_THREADSAFE_ISUPPORTS1

Comment 9

[David :Bienvenu]

Attachment: comm central part of fix - checked in w/ suggested comment.

Comment 10

[David :Bienvenu]

Attachment: moz-central part of fix

I had to change both these files to get rid of the assertions when I click on a signed message.

Comment 11

[David :Bienvenu]

pinging for review...

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 482941 [details] [diff] [review]
moz-central part of fix

This is OK, per comment 8
r=kaie

Comment 13

[Kai Engert [:KaiE:]]

Comment on attachment 482940 [details] [diff] [review]
comm central part of fix - checked in w/ suggested comment.

I was reading code to ensure this is a safe change.
While I did, I wrote down my thinking, and I propose to add it as a comment.

If you agree with the following comment, please add it prior to checkin,
and r=kaie


 class nsSMimeVerificationListener
 {
   ...
   protected:
   // It is safe to declare this implementation as thread safe,
   // despite not using a lock to protect the members.
   // Because of the way the object will be used, we don't expect a race.
   // After construction, the object is passed to another thread,
   // but will no longer be accessed on the original thread.
   // The other thread is unable to access/modify self's data members.
   // When the other thread is finished, it will call into the "Notify"
   // callback. Self's members will be accessed on the other thread,
   // but this is fine, because there is no race with the original thread.
   // Race-protection for XPCOM reference counting is sufficient.

   nsCOMPtr<nsIMsgSMIMEHeaderSink> mHeaderSink;
   ...
 };

Comment 14

[David :Bienvenu]

Thx very much, Kaie. I'm fine with that comment. I'll attach a patch with it in a minute.

Is security/manager/ssl/src under the same checkin rules as the rest of core, i.e., I'd need to get 2.0 approval?

Comment 15

[David :Bienvenu]

Comment on attachment 482941 [details] [diff] [review]
moz-central part of fix

This quiets a thread-safety assertion in Thunderbird whenever a signed message is read, and the object is perfectly threadsafe because it has no member vars (it just inherits from nsIInterfaceRequestor).

Comment 16

[Kai Engert [:KaiE:]]

(In reply to comment #14)
> Thx very much, Kaie. I'm fine with that comment. I'll attach a patch with it in
> a minute.
> 
> Is security/manager/ssl/src under the same checkin rules as the rest of core,
> i.e., I'd need to get 2.0 approval?

yes.
security/manager is part of the core mozilla platform and has the same rules.

Comment 17

[Benjamin Smedberg]

Please attach a final patch with the correct code comment as suggested in the bug.

Comment 18

[David :Bienvenu]

(In reply to comment #17)
> Please attach a final patch with the correct code comment as suggested in the
> bug.

the suggested comment was for the comm-central patch, not the moz-central patch. re-requesting approval.

Comment 19

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

Comment on attachment 482941 [details] [diff] [review]
moz-central part of fix

This was approved on January 20th but not landed. Removing approval as we've closed for the last beta. Please renominate for approval with risk v. reward statement if you think it should go into RC.

Comment 20

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This is likely to go in after RC but setting checkin-needed in case it slips off our radar..

Comment 21

[Henri Sivonen (:hsivonen)]

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