SSL connections do not work correctly when not managed by the socket transport service (including especially when the socket is not non-blocking)

RESOLVED FIXED in Firefox 12

Status

()

Core
Security: PSM
P1
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox10-, firefox12 verified)

Details

(Whiteboard: [inbound])

Attachments

(6 attachments, 11 obsolete attachments)

3.04 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
36.97 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
1.04 KB, text/plain
Details
4.77 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
15.25 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
14.47 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #704984 +++

(In reply to Honza Bambas (:mayhemer) from Bug #704984 comment #54)
> - PSMRunnable (SyncRunnableBase) allowed to be dispatched also from the main
> thread (calls its operation method directly)
> - NS_IsMainThread() special handling in PK11PasswordPrompt no longer
> necessary

OK.

> - SSLServerCertVerificationJob remembers the thread it has to dispatch the
> result to (= the thread where the socket is being called on)
> - nsNSSSocketInfo::GetPreviousCert can now be called on the main thread too
> - when send operation on a blocking socket returns would-block then if we
> are waiting for cert verify, block by looping the event queue and then retry
> the send again

There is a better fix. When the current thread is not the socket transport thread, then do not use async. certificate path validation. That is, instead of dispatching a SSLServerCertVerificationJob and returning SECWouldBlock, just call AuthCertificate directly. This will work for blocking sockets too. Async. cert validation is only needed for the socket transport thread.

Yesterday (last night), I also investigated the technique you propose in this patch, having the certificate validation job remembering the thread it is dispatched from, and then returning to it. The problem is that there is another assumption that the async cert validation code makes: it assumes that it can interrupt any PR_Poll (any blocking socket I/O operation) by dispatching an event to that thread. It isn't clear to me that this is (always) going to work for anything except the socket transport thread.

> The true fix should be made in NSS (not for today): when the socket is set
> blocking, we must never return would-block since it will be handled as a
> fatal error by the calling layer.  No idea how to achieve this  This
> presumption might be wrong for the ssl socket, though..

The cert auth hook must never return SECWouldBlock for a blocking socket.
No longer depends on: 704984
Keywords: crash, regressionwindow-wanted
I will submit two patches:

Patch #1 will make SSL connections work correctly in these cases, but cert overrides won't work.
Patch #2 will make the cert overrides work.

Patch #1 will be ready shortly. Patch #2 will take a little more time.
Hardware: x86 → All
Summary: SSL connections do not work correctly when I/O is done off of the main thread or when the socket is not non-blocking → SSL connections do not work correctly when not managed by the socket transport service (including especially when the socket is not non-blocking)
(In reply to Brian Smith (:bsmith) from comment #0)
> There is a better fix. When the current thread is not the socket transport
> thread, then do not use async. certificate path validation. That is, instead
> of dispatching a SSLServerCertVerificationJob and returning SECWouldBlock,
> just call AuthCertificate directly. This will work for blocking sockets too.
> Async. cert validation is only needed for the socket transport thread.

Are you sure about this?  When you will do this on the main thread, then how will cert verification process OCSP requests that must go though the main thread able to process events?
(In reply to Honza Bambas (:mayhemer) from comment #2)
> (In reply to Brian Smith (:bsmith) from comment #0)
> > Async. cert validation is only needed for the socket transport thread.
> 
> Are you sure about this?  When you will do this on the main thread, then how
> will cert verification process OCSP requests that must go though the main
> thread able to process events?

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCallbacks.cpp?rev=cf0b31ff2b6d#401

I am not sure yet. But, our code for fetching OCSP responses has special support for running on the main thread.
(In reply to Brian Smith (:bsmith) from comment #3)
> http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/
> nsNSSCallbacks.cpp?rev=cf0b31ff2b6d#401
> 
> I am not sure yet. But, our code for fetching OCSP responses has special
> support for running on the main thread.

Nice, but: http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCallbacks.cpp?rev=cf0b31ff2b6d#386
BTW, with my patch from bug 704984:

  >	xul.dll!nsHTTPDownloadEvent::Run()  Line 100	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x003a7277)  Line 625 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x007d7d88, bool mayWait=true)  Line 245 + 0x17 bytes	C++
 	xul.dll!nsThread::Shutdown()  Line 483 + 0xb bytes	C++
 	xul.dll!NS_InvokeByIndex_P(nsISupports * that=0x062eb400, unsigned int methodIndex=6, unsigned int paramCount=0, nsXPTCVariant * params=0x00000000)  Line 103	C++
 	xul.dll!nsProxyObjectCallInfo::Run()  Line 182 + 0x2d bytes	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x003a7363)  Line 625 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x007d7d88, bool mayWait=true)  Line 245 + 0x17 bytes	C++
 	xul.dll!nsNSSSocketInfo::BlockOnCertVerification(int bytesWritten=-1)  Line 1028 + 0xd bytes	C++
 	xul.dll!PSMSend(PRFileDesc * fd=0x008337e0, const void * buf=0x0b2b34bc, int amount=43, int flags=0, unsigned int timeout=10000)  Line 2267 + 0xc bytes	C++
 	nspr4.dll!PR_Send(PRFileDesc * fd=0x008337e0, const void * buf=0x0b2b34bc, int amount=43, int flags=0, unsigned int timeout=10000)  Line 226 + 0x1e bytes	C
 	nsldappr32v60.dll!prldap_write(int s=1, const void * buf=0x0b2b34bc, int len=43, lextiof_socket_private * socketarg=0x0b27bb88)  Line 218 + 0x1a bytes	C
 	nsldap32v60.dll!ber_flush(sockbuf * sb=0x0b1b8a48, berelement * ber=0x0b2b3390, int freeit=0)  Line 439 + 0x26 bytes	C
 	nsldap32v60.dll!nsldapi_send_ber_message(ldap * ld=0x0b1b8858, sockbuf * sb=0x0b1b8a48, berelement * ber=0x0b2b3390, int freeit=0, int epipe_handler=1)  Line 473 + 0x11 bytes	C
 	nsldap32v60.dll!nsldapi_send_server_request(ldap * ld=0x0b1b8858, berelement * ber=0x0b2b3390, int msgid=1, ldapreq * parentreq=0x00000000, ldap_server * srvlist=0x00000000, ldap_conn * lc=0x0b1cf098, char * bindreqdn=0x0b2c0778, int bind=0)  Line 342 + 0x17 bytes	C
 	nsldap32v60.dll!nsldapi_send_initial_request(ldap * ld=0x0b1b8858, int msgid=1, unsigned long msgtype=96, char * dn=0x0b1b0890, berelement * ber=0x0b2b3390)  Line 148 + 0x2b bytes	C
 	nsldap32v60.dll!simple_bind_nolock(ldap * ld=0x0b1b8858, const char * dn=0x0b1b0890, const char * passwd=0x0b1c99d0, int unlock_permitted=1)  Line 153 + 0x17 bytes	C
 	nsldap32v60.dll!ldap_simple_bind(ldap * ld=0x0b1b8858, const char * dn=0x0b1b0890, const char * passwd=0x0b1c99d0)  Line 82 + 0x13 bytes	C
 	xul.dll!nsLDAPOperation::SimpleBind(const nsACString_internal & passwd={...})  Line 322 + 0x30 bytes	C++
 	xul.dll!nsAbLDAPListenerBase::OnLDAPInit(nsILDAPConnection * aConn=0x0b1b0b70, unsigned int aStatus=0)  Line 302 + 0x35 bytes	C++
 	xul.dll!nsLDAPConnection::OnLookupComplete(nsICancelable * aRequest=0x0b1b7814, nsIDNSRecord * aRecord=0x0b0d4450, unsigned int aStatus=0)  Line 663	C++
(In reply to Honza Bambas (:mayhemer) from comment #4)
> Nice, but:
> http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/
> nsNSSCallbacks.cpp?rev=cf0b31ff2b6d#386

Yes, that will queue an event on the main thread. Then, later, if we are on the main thread, we will run the event loop and then process that event (among others), right?
Depends on: 706955
Created attachment 583723 [details] [diff] [review]
Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread
Created attachment 583724 [details] [diff] [review]
Part 2: Fix certificate verification for blocking sockets and sockets not managed by the socket transport service
Attachment #583724 - Flags: review?(honzab.moz)
Created attachment 583725 [details] [diff] [review]
Part 3: Move cert override logic to SSLServerCertVerification.cpp, r?honzab

This patch simply moves the cert override handling from nsNSSIOLayer.cpp to SSLServerCertVerification.cpp. I did this because I thought I was going to need to make many changes to it. However, I realized later that I hardly had to change anything with the cert override handling. But this was a change that Honza had previously recommended anyway, so I left it in. So, if you want, I can redo the next patch so that it doesn't depend on this one.
Attachment #583725 - Flags: review?(honzab.moz)
Created attachment 583726 [details] [diff] [review]
Part 4: Fix cert error overrides for blocking sockets and sockets not managed by the socket transport service
Attachment #583726 - Flags: review?(honzab.moz)
Note that Part 1 is just the SyncRunnableBase changes from Honza's patch in bug 704984 attachment 583184 [details] [diff] [review].
Attachment #583723 - Flags: superreview?(honzab.moz)
Attachment #583723 - Flags: review+
tested this in the following horrible way:

Horrible Test #1
----------------
This tests verifies that SSL server cert verification, including OCSP fetching, works correctly on the main thread.

1. I configured a new LDAP account with www.paypal.com as the server, on port 443, using SSL.
2. I set breakpoints in a couple of places in internal_send_receive_attempt.
3. I configured Thunderbird to use this "LDAP" server for autocompletion in Options -> Addressing -> Directory Server.
4. I started composing a new email.
5. I started typing an email address in the "To:" field.
6. My breakpoints in internal_send_receive_attempt were hit.
7. I stepped through the code to verify that everything was working as I expected.
8. Of course, later the bind fails, since www.paypal.com isn't really an LDAP server.

Horrible Test #2
----------------
1. I reconfigured the LDAP settings to use addressbook.mozilla.com with correct settings, using SSL.
2. I started composing a new email
3. I started typing an email address in the "To:" field.
4. I verified that address looks were working correctly.

Tomorrow, I will try to create some xpcshell tests that verify this in a less hacky, and automated way. It won't test LDAP, but it will test some other protocol on the main thread that uses OCSP.
Created attachment 583748 [details] [diff] [review]
Part 5: Move SSLServerCertVerificationResult to SSLServerCertVerification.cpp

This should have been part of Part 3. Once the cert override logic is moved to SSLServerCertVerification.cpp, SSLServerCertVerificationResult doesn't need to be exposed outside of that file anymore.
Attachment #583748 - Flags: review?(honzab.moz)
Created attachment 583751 [details] [diff] [review]
Part 5: Move SSLServerCertVerificationResult to SSLServerCertVerification.cpp [v2]
Attachment #583748 - Attachment is obsolete: true
Attachment #583748 - Flags: review?(honzab.moz)
Attachment #583751 - Flags: review?(honzab.moz)
Brian, p2 patch could not be applied to comm-cetral/mozilla.  Please rebase the patches to what the current client.py from comm-cetral checks out.  I want to test the patches with my _working_ LDAPS OCSP'ed server.
Comment on attachment 583723 [details] [diff] [review]
Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread

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

I can't superreview my own work.
Attachment #583723 - Flags: superreview?(honzab.moz) → superreview?(kaie)
Created attachment 583955 [details] [diff] [review]
Part 2: Fix certificate verification for blocking sockets and sockets not managed by the socket transport service
Attachment #583724 - Attachment is obsolete: true
Attachment #583724 - Flags: review?(honzab.moz)
Attachment #583955 - Flags: review?(honzab.moz)
Created attachment 583957 [details] [diff] [review]
Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread
Attachment #583723 - Attachment is obsolete: true
Attachment #583723 - Flags: superreview?(kaie)
Attachment #583957 - Flags: review?(honzab.moz)
Patches now apply and fixes the LDAP issue for me.
Comment on attachment 583957 [details] [diff] [review]
Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread

Honza, I originally asked you for SR for this patch just because I wanted you to look over the things I omitted from your patch. I don't think an SR is needed for this.
Attachment #583957 - Flags: review?(honzab.moz)
Attachment #583957 - Flags: review+
Attachment #583957 - Flags: feedback?(honzab.moz)
You are missing the changes to CertErrorRunnable::Run().  When the LDAP server has a bad certificate, you don't set the error on the socket, so you silently add an exception for all bad certificates when the cert verification doesn't happen on the socket thread, quit bad.  The reason is missing call to SSLServerCertVerificationResult::Run() that sets the result of verification on the socket (and cancels it when cert is bad).

Just be aware that you should prevent call to SSL_RestartHandshakeAfterAuthCertificate synchronously from the AuthCertificateHook as you could renter the locks, but you know this better then me.

During the review (not yet published) I proposed to have a static function that verifies and if bad, handles the bad cert since you, more or less, duplicate the code of SSLServerCertVerificationJob::Run() in AuthCertificateHook.  Not good since it may introduce bugs like described above.

Setting the verification result on the socket could be done in that method so you may leave CertErrorRunnable simpler.


Comm-beta displays the bad cert info dialog and fails the load.  Comm-central w/o the patches obviously deadlocks.
Attachment #583957 - Flags: feedback?(honzab.moz) → feedback-
Comment on attachment 583955 [details] [diff] [review]
Part 2: Fix certificate verification for blocking sockets and sockets not managed by the socket transport service

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

Dropping r flag, this is just a pre-review anyway.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +455,5 @@
>  {
> +  // Runs on a certificate verification thread if the verifying a certificate 
> +  // for a connection on the socket transport thread; otherwise, runs on the
> +  // thread doing the network I/O, which may (but shouldn't) be the main
> +  // thread.

This comment needs a wording check.

The "but shouldn't" seems redundant to me here.  This patch is fixing exactly this issue.  Now any thread can call on cert verification.

@@ +734,4 @@
>  
>    nsNSSSocketInfo *socketInfo = static_cast<nsNSSSocketInfo*>(arg);
> +
> +  bool onSTSThread;

onNetworkThread ?

@@ +747,5 @@
> +    PR_SetError(PR_UNKNOWN_ERROR, 0);
> +    return SECFailure;
> +  }
> +  
> +  SECStatus rv;

s/rv/status/ ?

@@ +761,5 @@
> +    // thread doing the network I/O may not interrupt its network I/O on receipt
> +    // of our SSLServerCertVerificationResult event, and/or it might not even be
> +    // a non-blocking socket.
> +    rv = AuthCertificate(socketInfo, serverCert);
> +    // XXX: cert overrides are not handled!

You should add to this comment that we also block the read/write op until the cert verification is done regardless the socket is blocking or non-blocking.
Attachment #583955 - Flags: review?(honzab.moz)
Comment on attachment 583726 [details] [diff] [review]
Part 4: Fix cert error overrides for blocking sockets and sockets not managed by the socket transport service

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

Same here.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +281,5 @@
>  // SSLServerCertVerificationResult with the error code from PR_GetError().
>  SECStatus
>  HandleBadCertificate(PRErrorCode defaultErrorCodeToReport,
> +                    nsNSSSocketInfo * socketInfo, CERTCertificate * cert,
> +                    const void * fdForLogging, bool isAsync)

Indention

@@ +456,2 @@
>    }
>    if (NS_FAILED(nrv)) {

Blank line before this if () { line please.

@@ +1251,5 @@
> +      if (rv != SECSuccess && PR_GetError() == 0) {
> +        NS_ERROR("AuthCertificate or HandleBadCertificate did not set error code");
> +        PR_SetError(PR_UNKNOWN_ERROR, 0);
> +      }
> +    }

This code now duplicates in SSLServerCertVerificationJob::Run().  

You should create a static method containing this code that SSLServerCertVerificationJob::Run() would be using.
Attachment #583726 - Flags: review?(honzab.moz)
Comment on attachment 583725 [details] [diff] [review]
Part 3: Move cert override logic to SSLServerCertVerification.cpp, r?honzab

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

I hate checking these patches.. :)

r=honzab.
Attachment #583725 - Flags: review?(honzab.moz) → review+
Comment on attachment 583751 [details] [diff] [review]
Part 5: Move SSLServerCertVerificationResult to SSLServerCertVerification.cpp [v2]

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

r=honzab
Attachment #583751 - Flags: review?(honzab.moz) → review+
Comment on attachment 583957 [details] [diff] [review]
Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread

The final version of this patch will need to land on beta channel as well.  See bug 708813 comment 25.
Blocks: 708813
Are we close to landing this?

I'm especially concerned about part 1 landing on the beta channel in time for release - as otherwise some people will have broken LDAP in Thundebird. I'm not sure what the effects could be for Firefox users (if any).
tracking-firefox10: --- → ?
Ping
Created attachment 590132 [details] [diff] [review]
Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread [v2]

This is the same patch as the first version but with a whitespace change. I understand that there needs to be a change to the way CertErrorRunnable::Run works for the non-STS case, but in other patches in this series I have changed how CertErrorRunnable works for other reasons.
Attachment #583957 - Attachment is obsolete: true
Attachment #590132 - Flags: review?(honzab.moz)
Comment on attachment 590132 [details] [diff] [review]
Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread [v2]

I'm afraid I cannot review this until I also see the remaining changes to be made and have a complete set of patches to test.
(In reply to Honza Bambas (:mayhemer) from comment #30)
> Comment on attachment 590132 [details] [diff] [review]
> Part 1: Make SyncRunnableBase work without deadlocking when run on the main
> thread [v2]
> 
> I'm afraid I cannot review this until I also see the remaining changes to be
> made and have a complete set of patches to test.

Doesn't bug 708813 just require this patch though?
(In reply to Mark Banner (:standard8) from comment #31)
> (In reply to Honza Bambas (:mayhemer) from comment #30)
> > Comment on attachment 590132 [details] [diff] [review]
> > Part 1: Make SyncRunnableBase work without deadlocking when run on the main
> Doesn't bug 708813 just require this patch though?

Yes, that's right.  This patch in particular should land only and only on beta.

I'll copy the patch to that bug and r+ it.

But I still don't think it is sufficient for this bug until the remaining patches get updated and I can test them as a complete set.
Created attachment 590524 [details] [diff] [review]
Part 2: Move CertErrorRunnable, SSLServerCertVerificationResult, and HandleBadCertificate to SSLServerCertVerification.cpp

Honza, I wrote a script to help you review this, which I will attach next.
Attachment #583725 - Attachment is obsolete: true
Attachment #583751 - Attachment is obsolete: true
Attachment #590524 - Flags: review?(honzab.moz)
Created attachment 590525 [details]
[NOT FOR CHECKIN] Script to help verify that there were no semantic changes in Part 2

Here is the script to help review Part 2. That is a big patch but it is just moving code and fiddling with #includes and minor declarations that support the moved code.
Created attachment 590526 [details] [diff] [review]
Part 3: Correct the documentation of how threading works
Attachment #590526 - Flags: review?(honzab.moz)
Attachment #590526 - Attachment description: Part 3: Correct the documentation of how → Part 3: Correct the documentation of how threading works
Created attachment 590528 [details] [diff] [review]
Part 4: Split the creation of CertErrorRunnable from the dispatching of it
Attachment #590528 - Flags: review?(honzab.moz)
Created attachment 590529 [details] [diff] [review]
Part 4: Split the creation of CertErrorRunnable from the dispatching of it
Attachment #590528 - Attachment is obsolete: true
Attachment #590528 - Flags: review?(honzab.moz)
Created attachment 590530 [details] [diff] [review]
Part 5: Part 5: Add back synchronous cert validation for Thunderbird
Attachment #583726 - Attachment is obsolete: true
Attachment #583955 - Attachment is obsolete: true
Attachment #590524 - Attachment description: Move CertErrorRunnable, SSLServerCertVerificationResult, and HandleBadCertificate to SSLServerCertVerification.cpp → Part 2: Move CertErrorRunnable, SSLServerCertVerificationResult, and HandleBadCertificate to SSLServerCertVerification.cpp
Tryserver for Firefox seems to be broken right now.

Thunderbird builds with these patches are currently building on cc-try:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=8f9793ec4860
Attachment #590525 - Attachment is patch: false
Comment on attachment 590132 [details] [diff] [review]
Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread [v2]

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

r=honzab for landing on m-c and r/m-a and actually as well as r/m-b (marked then in a different bug).
Attachment #590132 - Flags: review?(honzab.moz) → review+
Comment on attachment 590524 [details] [diff] [review]
Part 2: Move CertErrorRunnable, SSLServerCertVerificationResult, and HandleBadCertificate to SSLServerCertVerification.cpp

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

Thanks for the script, Brian!  Very helpful.
r=honzab
Attachment #590524 - Flags: review?(honzab.moz) → review+
Comment on attachment 590526 [details] [diff] [review]
Part 3: Correct the documentation of how threading works

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

r=honzab
Attachment #590526 - Flags: review?(honzab.moz) → review+
Comment on attachment 590529 [details] [diff] [review]
Part 4: Split the creation of CertErrorRunnable from the dispatching of it

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

This really is nice.

r=honzab

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +358,3 @@
>  }
>  
> +// Returns null with the error code set if it does not create the

"with the PR error code" ?

@@ +508,5 @@
>    }
>  
>    socketInfo->SetStatusErrorBits(*nssCert, collected_errors);
> +  nsSSLIOLayerHelpers::mHostsWithCertErrors->RememberCertHasError(
> +    socketInfo, socketInfo->SSLStatus(), SECFailure);

You forgot to remove this call from SetStatusErrorBits if you still believe it is a good idea to call this separately.

@@ +526,5 @@
> +// which may call nsHttpConnection::GetInterface() through
> +// nsNSSSocketInfo::mCallbacks. nsHttpConnection::GetInterface must always
> +// execute on the main thread, with the socket transport service thread
> +// blocked.
> +class CertErrorRunnableRunnable : public nsRunnable

Interesting solution.

@@ +540,5 @@
> +    nsresult rv = mCertErrorRunnable->DispatchToMainThreadAndWait();
> +    // The result must run on the socket transport thread, which we are already
> +    // on, so we can just run it directly, instead of dispatching it.
> +    if (NS_SUCCEEDED(rv)) {
> +      rv = mCertErrorRunnable->mResult->Run();

Maybe worth to also check on presence of mResult?  Regardless of the MOZ_ASSERT in RunOnTargetThread.

You have a similar check in AuthCertificateHook in Part 5 patch.

Actually, there used to be a code to always create a result.  But it was just a corner case insurance..
Attachment #590529 - Flags: review+
Comment on attachment 590530 [details] [diff] [review]
Part 5: Part 5: Add back synchronous cert validation for Thunderbird

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

r=honzab

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ -1119,5 @@
>    // doing verification without checking signatures.
> -  NS_ASSERTION(checkSig, "AuthCertificateHook: checkSig unexpectedly false");
> -
> -  // PSM never causes libssl to call this function with PR_TRUE for isServer,
> -  // and many things in PSM assume that we are a client.

Shouldn't be this removal in a different bug?

@@ +1204,5 @@
> +    error = PR_UNKNOWN_ERROR;
> +  }
> +
> +  PR_SetError(error, 0);
> +  return SECFailure;

Again, this is a code very much similar to the code in SSLServerCertVerificationJob::Run().  We should have a followup to try to stick this code in one place.

For now, since it is that late, let's go with this, since it works well.
Attachment #590530 - Flags: review+
Created attachment 590608 [details] [diff] [review]
Part 4: Split the creation of CertErrorRunnable from the dispatching of it

Addresses Honza's review comments.
Attachment #590529 - Attachment is obsolete: true
Attachment #590608 - Flags: review+
Created attachment 590610 [details] [diff] [review]
Part 5: Add back synchronous cert validation for Thunderbird

Addresses Honza's review comments.
Attachment #590530 - Attachment is obsolete: true
Attachment #590610 - Flags: review+

Updated

5 years ago
tracking-firefox10: ? → +

Updated

5 years ago
tracking-firefox10: + → -
It seems people are overlooking that this bug also depends on bug 706955 being fixed. There is a patch in bug 706955 awaiting review. I have now just changed the reviewer.
Here are the tryserver results for mozilla-central, for these patches and a bunch more related ones:
https://tbpl.mozilla.org/?tree=Try&rev=b63de8572b37
No longer depends on: 706955
https://hg.mozilla.org/integration/mozilla-inbound/rev/9576aeb57bd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/efae575a79e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/e610972755d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bcaac9fadf1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9eab22ce37a
Whiteboard: [inbound]
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/9576aeb57bd4
https://hg.mozilla.org/mozilla-central/rev/efae575a79e4
https://hg.mozilla.org/mozilla-central/rev/e610972755d9
https://hg.mozilla.org/mozilla-central/rev/0bcaac9fadf1
https://hg.mozilla.org/mozilla-central/rev/d9eab22ce37a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 590132 [details] [diff] [review]
Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread [v2]

[Approval Request Comment]
This has landed on mozilla-central, it narrowly missed landing on mozilla-aurora by a few hours, when we were expecting it to get into mozilla-central.

We could really do with this on mozilla-aurora so that LDAP is no longer broken there, and our testers (that include quite a few LDAP users) can use it again. Doing a one-off branch is not really viable for Thunderbird & SeaMonkey on mozilla-aurora.

Regression caused by (bug #): Various PSM changes
User impact if declined: LDAP over SSL hangs for Thunderbird & SeaMonkey 
Testing completed (on m-c, etc.): Landed on mozilla-central, now in nightlies, it is being released in Thunderbird 11 Beta via a special relbranches for each beta and then for final release. 
Risk to taking this patch (and alternatives if risky): Changes in this patch do affect Firefox, as said previously, this only missed the merge by a few hours. This will hopefully be somewhat mitigated by the extra testing received by the Betas.
String changes made by this patch: None.
Attachment #590132 - Flags: approval-mozilla-aurora?
Attachment #590524 - Flags: approval-mozilla-aurora?
Attachment #590526 - Flags: approval-mozilla-aurora?
Attachment #590608 - Flags: approval-mozilla-aurora?
Attachment #590610 - Flags: approval-mozilla-aurora?
(In reply to Mark Banner (:standard8) from comment #51)
> We could really do with this on mozilla-aurora so that LDAP is no longer
> broken there, and our testers (that include quite a few LDAP users) can use
> it again. Doing a one-off branch is not really viable for Thunderbird &
> SeaMonkey on mozilla-aurora.

I agree. We should land this on mozilla-aurora ASAP.

> Regression caused by (bug #): Various PSM changes

bug 674147 and maybe bug 675221
Target Milestone: mozilla13 → mozilla12
Did not intend to set target milestone.
Target Milestone: mozilla12 → mozilla13
Comment on attachment 590132 [details] [diff] [review]
Part 1: Make SyncRunnableBase work without deadlocking when run on the main thread [v2]

[Triage Comment]
Missed the merge by a few hours - approved for Aurora 12.
Attachment #590132 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #590524 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #590526 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #590608 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #590610 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 704984
Transplanted to aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/0bff753b0529
https://hg.mozilla.org/releases/mozilla-aurora/rev/38c4b6c2a63e
https://hg.mozilla.org/releases/mozilla-aurora/rev/8e27df1d74b4
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd0380ff4264
https://hg.mozilla.org/releases/mozilla-aurora/rev/e844448f2d1d
status-firefox12: --- → fixed
Thanks for fixing it for Firefox 12. I can no longer reproduce the freeze with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a2) Gecko/20120226 Thunderbird/12.0a2 ID:20120226030023
status-firefox12: fixed → verified
Blocks: 705773
You need to log in before you can comment on or make changes to this bug.