Closed Bug 819445 Opened 11 years ago Closed 11 years ago

~nsHTTPListener() destroys condition variable on which other threads are blocked.

Categories

(Core :: Networking, defect)

All
Linux
defect
Not set
critical

Tracking

()

RESOLVED INVALID

People

(Reporter: ishikawa, Unassigned)

References

Details

TB (and presumably FF) does not check for proper thread
synchronization, and when ~nsHTTPListener() is called,
it destroys a condition variable on which some other threads are
waiting (!)

This is bad.

Once this is done. Anything goes in terms of thread safety.

E.g.
http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_cond_init.html

> Attempting to destroy a condition variable upon which other threads
> are currently blocked results in undefined behavior.

I suspect this may be ONE of the evil causes of thread races
in TB, which may cause the failures in 

Bug 814438 - Invalid Read of 4 bytes through bogus pointer in nssCertificate_Destroy (thunderbird) 

Bug 713624 - Thunderbird locks (deadlock?) on a flaky connection with rss feeds
or
Bug 819386 - Intermittent Android shutdown crash [@nss_certificate_hash] 
etc., for example.

(But it seems that there *ARE* more places where proper
locking/unlocking is missing in TB/FF code :-( 
And so this problem reported here is not solely responsible for the
hard-to-reproduce race issues :-(

To witness: the following is a warning from a helgrind thread checker (a tool
in valgrind suite.) when TB is run under helgrind. (I tweaked
MOZOBJ/mozilla/dist/bin/thunderbird so that it will run
thunderbird-bin under valgrind. I ran "make check" to see if there
are thread-related problems, and this bugzilla entry is one of the
outstanding problem I notice in the session log.

==23074== ----------------------------------------------------------------
==23074==
==23074== Thread #1: pthread_cond_destroy: destruction of condition variable being waited upon
==23074==    at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==23074==    by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==23074==    by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==23074==    by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==23074==    by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==23074==    by 0x603EFCA: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.h:442)
==23074==    by 0x4C8569A: nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (nsCOMPtr.h:624)
==23074==    by 0x4C84EA5: nsStreamListenerTee::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (nsStreamListenerTee.cpp:49)
==23074==    by 0x4D12A45: mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (nsHttpChannel.cpp:4994)
==23074==    by 0x4C650F8: nsInputStreamPump::OnStateStop() (nsInputStreamPump.cpp:552)
==23074==    by 0x4C652AD: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:374)
==23074==    by 0x606E28D: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:82)
==23074==
{



------------------------
versions (comm-central)
-----------------------
cd /home/ishikawa/TB-NEW/TB-3HG/new-src
hg identify
99fd4744910d+ tip

-----------------------
mozilla subdirectory
-----------------------
cd /home/ishikawa/TB-NEW/TB-3HG/new-src/mozilla
hg identify
3b71d63eafd5+ tip
Blocks: 814438
BTW, would it not be great if helgrind can tell us which thread, and in which function is blocking on the cond variable being destroyed?
(Maybe helgrind on my PC is running out of memory [linux 32 bits] to record such information since there are so many thread-related warnings. It says after 100 possible errors/warnings, the future errors/warnings may not be detailed.)

In order to track down the real causes of races, I am afraid we have to place many proper locking even if there are cases they would slow TB (FF). 
With the current state of affairs, the sheer volume of warnings from valgrind makes it rather tough to figure out what is really going on.

I would post a few more cases where condition variable being waited on is destroyed.
A few more logs to show where the problem may happen.


==32661== ----------------------------------------------------------------
==32661==
==32661== Thread #1: pthread_cond_destroy: destruction of condition variable being waited upon
==32661==    at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==32661==    by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==32661==    by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==32661==    by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==32661==    by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==32661==    by 0x603EFCA: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.h:442)
==32661==    by 0x4C8569A: nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (nsCOMPtr.h:624)
==32661==    by 0x4C84EA5: nsStreamListenerTee::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (nsStreamListenerTee.cpp:49)
==32661==    by 0x4D12A45: mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (nsHttpChannel.cpp:4994)
==32661==    by 0x4C650F8: nsInputStreamPump::OnStateStop() (nsInputStreamPump.cpp:552)
==32661==    by 0x4C652AD: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:374)
==32661==    by 0x606E28D: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:82)
==32661==




==32661== ----------------------------------------------------------------
==32661==
==32661== Thread #30: pthread_cond_destroy: destruction of condition variable being waited upon
==32661==    at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==32661==    by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==32661==    by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==32661==    by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==32661==    by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==32661==    by 0x594755A: nsNSSHttpRequestSession::~nsNSSHttpRequestSession() (nsCOMPtr.h:410)
==32661==    by 0x477207D: CERT_CheckOCSPStatus (ocsp.c:3509)
==32661==    by 0x47748E1: CERT_VerifyCert (certvfy.c:1330)
==32661==    by 0x4774981: CERT_VerifyCertNow (certvfy.c:1365)
==32661==    by 0x595642D: mozilla::psm::(anonymous namespace)::AuthCertificate(mozilla::psm::TransportSecurityInfo*, CERTCertificateStr*) (SSLServerCertVerification.cpp:646)
==32661==    by 0x5956759: mozilla::psm::(anonymous namespace)::SSLServerCertVerificationJob::Run() (SSLServerCertVerification.cpp:1046)
==32661==    by 0x60866C3: nsThreadPool::Run() (nsThreadPool.cpp:194)
==32661==



The following seems to be the same as first one.
==740== ----------------------------------------------------------------
==740==
==740== Thread #1: pthread_cond_destroy: destruction of condition variable being waited upon
==740==	   at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==740==	   by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==740==	   by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==740==	   by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==740==	   by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==740==	   by 0x603EFCA: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.h:442)
==740==	   by 0x4C8569A: nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (nsCOMPtr.h:624)
==740==	   by 0x4D12A45: mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (nsHttpChannel.cpp:4994)
==740==	   by 0x4C650F8: nsInputStreamPump::OnStateStop() (nsInputStreamPump.cpp:552)
==740==	   by 0x4C652AD: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:374)
==740==	   by 0x606E28D: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:82)
==740==	   by 0x6083B4B: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:642)
==740==


PKIX related context.
==740== ----------------------------------------------------------------
==740==
==740== Thread #39: pthread_cond_destroy: destruction of condition variable being waited upon
==740==	   at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==740==	   by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==740==	   by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==740==	   by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==740==	   by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==740==	   by 0x594755A: nsNSSHttpRequestSession::~nsNSSHttpRequestSession() (nsCOMPtr.h:410)
==740==	   by 0x4803FFA: PKIX_PL_Object_DecRef (pkix_pl_object.c:891)
==740==	   by 0x47C1BF4: pkix_OcspChecker_CheckExternal (pkix_ocspchecker.c:341)
==740==	   by 0x47C270B: PKIX_RevocationChecker_Check (pkix_revocationchecker.c:390)
==740==	   by 0x47D404D: pkix_CheckChain (pkix_validate.c:823)
==740==	   by 0x47D6EDF: pkix_Build_ValidateEntireChain (pkix_build.c:1307)
==740==	   by 0x47DAD24: pkix_BuildForwardDepthFirstSearch (pkix_build.c:2530)
==740==



From within JavaScript???
==1345== ----------------------------------------------------------------
==1345==
==1345== Thread #1: pthread_cond_destroy: destruction of condition variable being waited upon
==1345==    at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==1345==    by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==1345==    by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==1345==    by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==1345==    by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==1345==    by 0x603EFCA: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.h:442)
==1345==    by 0x57DE619: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:3081)
==1345==    by 0x57E5ED7: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1488)
==1345==    by 0x6768315: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==1345==    by 0x6759A10: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2334)
==1345==    by 0x6767339: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:326)
==1345==    by 0x67684D2: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:384)
==1345==


Same as the earlier PKIX context example
==1345== ----------------------------------------------------------------
==1345==
==1345== Thread #29: pthread_cond_destroy: destruction of condition variable being waited upon
==1345==    at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==1345==    by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==1345==    by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==1345==    by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==1345==    by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==1345==    by 0x594755A: nsNSSHttpRequestSession::~nsNSSHttpRequestSession() (nsCOMPtr.h:410)
==1345==    by 0x4803FFA: PKIX_PL_Object_DecRef (pkix_pl_object.c:891)
==1345==    by 0x47C1BF4: pkix_OcspChecker_CheckExternal (pkix_ocspchecker.c:341)
==1345==    by 0x47C270B: PKIX_RevocationChecker_Check (pkix_revocationchecker.c:390)
==1345==    by 0x47D404D: pkix_CheckChain (pkix_validate.c:823)
==1345==    by 0x47D6EDF: pkix_Build_ValidateEntireChain (pkix_build.c:1307)
==1345==    by 0x47DAD24: pkix_BuildForwardDepthFirstSearch (pkix_build.c:2530)
==1345==


Shutdown context:
==1597== ----------------------------------------------------------------
==1597==
==1597== Thread #1: pthread_cond_destroy: destruction of condition variable being waited upon
==1597==    at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==1597==    by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==1597==    by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==1597==    by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==1597==    by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==1597==    by 0x603EFCA: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.h:442)
==1597==    by 0x5947AC7: nsCancelHTTPDownloadEvent::Run() (nsCOMPtr.h:624)
==1597==    by 0x6083B4B: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:642)
==1597==    by 0x60439A4: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:221)
==1597==    by 0x6084C26: nsThread::Shutdown() (nsThread.cpp:484)
==1597==    by 0x59A7382: mozilla::storage::(anonymous namespace)::AsyncCloseConnection::Run() (mozStorageConnection.cpp:369)
==1597==    by 0x6083B4B: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:642)
==1597==



Another java script context?
==1702== ----------------------------------------------------------------
==1702==
==1702== Thread #1: pthread_cond_destroy: destruction of condition variable being waited upon
==1702==    at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==1702==    by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==1702==    by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==1702==    by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==1702==    by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==1702==    by 0x603EFCA: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.h:442)
==1702==    by 0x57DE619: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:3081)
==1702==    by 0x57E5ED7: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1488)
==1702==    by 0x6768315: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==1702==    by 0x6759A10: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2334)
==1702==    by 0x6767339: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) (jsinterp.cpp:326)
==1702==    by 0x67684D2: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:384)
==1702==


Similar to the two earlier PKIX related context.
==1702== ----------------------------------------------------------------
==1702==
==1702== Thread #28: pthread_cond_destroy: destruction of condition variable being waited upon
==1702==    at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==1702==    by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==1702==    by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==1702==    by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==1702==    by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==1702==    by 0x594755A: nsNSSHttpRequestSession::~nsNSSHttpRequestSession() (nsCOMPtr.h:410)
==1702==    by 0x4803FFA: PKIX_PL_Object_DecRef (pkix_pl_object.c:891)
==1702==    by 0x47C1BF4: pkix_OcspChecker_CheckExternal (pkix_ocspchecker.c:341)
==1702==    by 0x47C270B: PKIX_RevocationChecker_Check (pkix_revocationchecker.c:390)
==1702==    by 0x47D404D: pkix_CheckChain (pkix_validate.c:823)
==1702==    by 0x47D6EDF: pkix_Build_ValidateEntireChain (pkix_build.c:1307)
==1702==    by 0x47DAD24: pkix_BuildForwardDepthFirstSearch (pkix_build.c:2530)
==1702==

Similar to an earlier example.
==2466== ----------------------------------------------------------------
==2466==
==2466== Thread #1: pthread_cond_destroy: destruction of condition variable being waited upon
==2466==    at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==2466==    by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==2466==    by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==2466==    by 0x5947C40: nsHTTPListener::~nsHTTPListener() (CondVar.h:56)
==2466==    by 0x5947D82: nsHTTPListener::Release() (nsNSSCallbacks.cpp:536)
==2466==    by 0x603EFCA: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.h:442)
==2466==    by 0x4C8569A: nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (nsCOMPtr.h:624)
==2466==    by 0x4D04866: mozilla::net::HttpBaseChannel::DoNotifyListener() (HttpBaseChannel.cpp:1463)
==2466==    by 0x57DE619: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:3081)
==2466==    by 0x57E5ED7: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1488)
==2466==    by 0x6768315: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:364)
==2466==    by 0x6759A10: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2334)
==2466==
I  wonder if such destruction of condition variable while other tasks
are waiting on it lead to the following problematic log and similar.

e.g.
==1980== ----------------------------------------------------------------
==1980==
==1980== Thread #1: pthread_cond_destroy: destruction of unknown cond var
==1980==    at 0x4027A7F: pthread_cond_destroy_WRK (hg_intercepts.c:940)
==1980==    by 0x4029781: pthread_cond_destroy@* (hg_intercepts.c:958)
==1980==    by 0x47193BA: PR_DestroyCondVar (ptsynch.c:372)
==1980==    by 0x47A555A: IssuerCache_Destroy (crl.c:1103)
==1980==    by 0x47A5620: FreeIssuer (crl.c:1243)
==1980==    by 0x403FAE9: PL_HashTableEnumerateEntries (plhash.c:374)
==1980==    by 0x47A56CB: ShutdownCRLCache (crl.c:1322)
==1980==    by 0x476B02F: nss_Shutdown (nssinit.c:1086)
==1980==    by 0x476B168: NSS_Shutdown (nssinit.c:1149)
==1980==    by 0x594E456: nsNSSComponent::ShutdownNSS() (nsNSSComponent.cpp:1866)
==1980==    by 0x594EE91: nsNSSComponent::DoProfileBeforeChange(nsISupports*) (nsNSSComponent.cpp:2549)
==1980==    by 0x594F08B: nsNSSComponent::Observe(nsISupports*, char const*, unsigned short const*) (nsNSSComponent.cpp:2176)
==1980==
{
Summary: ~nsHTTPListener() destroys conditon variable on which other threads are blocked. → ~nsHTTPListener() destroys condition variable on which other threads are blocked.
(In reply to ISHIKAWA, chiaki from comment #3)
> I  wonder if such destruction of condition variable while other tasks
> are waiting on it lead to the following problematic log and similar.
> 

The original report still stands, but the above question
seem to be due to false-positive of helgrind.

See https://bugs.kde.org/show_bug.cgi?id=307082
 HG false positive: pthread_cond_destroy: destruction of unknown cond var 

helgrind was not handling pthread_cond_init() correctly, and thus 
cond var was considered unknown.

In the kde bug entry, a patch was posted (created by me) and is being reviewed.
I am using the patch to test thunderbird under helgrind and "destruction of unknown cond var" messages do not seem to be generated very often. So the question in comment 3 can be ignored for now.

The original question of "pthread_cond_destroy: destruction of condition variable being waited upon" is still being investigated.
I am trying to figure out how to do the following:
>BTW, would it not be great if helgrind can tell us which thread, and in which >function is blocking on the cond variable being destroyed?

At least, I think I can print the thread id of the waiting task(s) without
much difficulty, so intend to find out what tasks are waiting on the
destroyed cond variables.
A good news.
TB source code was OK.

It seems that stock valgrind/helgrind version 3.8.1 produced false
positives for pthread_cond_destroy().  I produced a patch to fix the
problem and no longer see the warning (also no warnings from
my audited source code to print error warnings if
pthread_cond_destroy() fails.)

Details is in Bug 819599,
( "~nsEventQueue() destroys condition variable on which other 
  threads are blocked."
specifically comment 
https://bugzilla.mozilla.org/show_bug.cgi?id=819599#c7

So I think we can safely close this for now.

Sorry for the noise.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
Did you run the tests with nsHttpTransaction::Release annotated as
shown below?  Without that, all thread checking tools will show false
errors since they do not understand threadsafe refcounting and
destructors.


diff --git a/netwerk/protocol/http/nsHttpTransaction.cpp b/netwerk/protocol/http/nsHttpTransaction.cpp
--- a/netwerk/protocol/http/nsHttpTransaction.cpp
+++ b/netwerk/protocol/http/nsHttpTransaction.cpp
@@ -24,16 +24,20 @@
 #include "nsMultiplexInputStream.h"
 #include "nsStringStream.h"
 
 #include "nsComponentManagerUtils.h" // do_CreateInstance
 #include "nsServiceManagerUtils.h"   // do_GetService
 #include "nsIHttpActivityObserver.h"
 #include "nsSocketTransportService2.h"
 
+#ifdef MOZ_VALGRIND
+# include <valgrind/helgrind.h>
+#endif
+
 
 using namespace mozilla;
 
 //-----------------------------------------------------------------------------
 
 #ifdef DEBUG
 // defined by the socket transport service while active
 extern PRThread *gSocketThread;
@@ -1597,23 +1601,29 @@ NS_IMPL_THREADSAFE_ADDREF(nsHttpTransact
 
 NS_IMETHODIMP_(nsrefcnt)
 nsHttpTransaction::Release()
 {
     nsrefcnt count;
     NS_PRECONDITION(0 != mRefCnt, "dup release");
     count = NS_AtomicDecrementRefcnt(mRefCnt);
     NS_LOG_RELEASE(this, count, "nsHttpTransaction");
+    // HGANN 018 BEGIN REQD_ANN thread-safe-ref-counting
     if (0 == count) {
+        ANNOTATE_HAPPENS_AFTER(&mRefCnt);
+        ANNOTATE_HAPPENS_BEFORE_FORGET_ALL(&mRefCnt);
         mRefCnt = 1; /* stablize */
         // it is essential that the transaction be destroyed on the consumer 
         // thread (we could be holding the last reference to our consumer).
         DeleteSelfOnConsumerThread();
         return 0;
+    } else {
+        ANNOTATE_HAPPENS_BEFORE(&mRefCnt);
     }
+    // HGANN 018 END
     return count;
 }
 
 NS_IMPL_THREADSAFE_QUERY_INTERFACE2(nsHttpTransaction,
                                     nsIInputStreamCallback,
                                     nsIOutputStreamCallback)
 
 //-----------------------------------------------------------------------------
(In reply to Julian Seward from comment #6)
> Did you run the tests with nsHttpTransaction::Release annotated as
> shown below?  Without that, all thread checking tools will show false
> errors since they do not understand threadsafe refcounting and
> destructors.
>

Thank you for you comment.

Actually, I took a previous patch posted to the following
bugzilla entry, 

Bug 551155 - mark up Fx codebase with race-detector annotations (edit) 

and modified it slightly so that the patch can be applied cleanly to
comm-central source files:
https://bugzilla.mozilla.org/attachment.cgi?id=678284
" Re-created patch against later C-C mozilla subdirectory (with USEHELGRIND macro) "

I notice that my patch doesn't include valgrind/valgrind.h directory (not sure
if this is fatal or not. I suspect that this causes a compile-time warning.)

(Also, maybe I should use MOZ_HELGRIND for #if/#endif condition?)

The problem with helgrind 3.8.1 was that it did not handle
pthread_cond_init() correctly (it did not handle it at all!), thus the cond variable was not known to the rest of valgrind. This is why we saw "unknown cond var", and I also fixed the problem of helgrind prematurely destroyed internal
data structure to handle cond var  when pthread_cond_destroy() would return EBUSY and retain the cond var.
(The details are in  http://bugsfiles.kde.org/attachment.cgi?id=75985
"HG false positive: pthread_cond_destroy: destruction of unknown cond var" )

After applying the patch in the above URL at kde.org to helgrind, 
I did not see any similar warnings (and I also added the check to verify the code for pthread_cond_destroy() to make doubly sure that pthread_cond_destroy() is returning a non-zero value. So far, the log looks good.)

TIA
You need to log in before you can comment on or make changes to this bug.