111384 - Support OCSP requests through a proxy

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, enhancement, P2)

Description

[Miikka Karhuluoma]

Is it possible to enhance OCSP support in that way it would work also behind
firewall via HTTP proxy?

Comment 1

[Stephane Saux]

cc Julien for NSS comments.
This would require support from NSS.  When NSS provides the application with the
callback that allows it to decide how to connect to the OCSP server, proxy will
certainly be possible.

Comment 2

[Julien Pierre]

There are many implications with that feature request for NSS, aspects of which
were discussed last week. I have opened an NSS bug on this, 152426.

Comment 3

[Kai Engert (:KaiE:)]

*** Bug 157955 has been marked as a duplicate of this bug. ***

Comment 4

[Alon Altman]

BTW, from bug 157955 - the error message printed is "Error establishing an
encrypted connection to [sitename] error
code -5933". This error should at least be more descriptive, as a temporary fix,
and maybe add a comment to the release notes.

Comment 5

[Kai Engert (:KaiE:)]

I think the real solution would be if NSS offered to register a callback to
handle OCSP fetching requests on the behalf of NSS.

By doing so, we could use the standard application mechanism to fetch URLs that
NSS requires.

By doing so, we would solve all proxy issues, and all issues where we appear to
hang, because NSS is executing some blocking code internally.

Comment 6

[John Unruh]

*** Bug 156580 has been marked as a duplicate of this bug. ***

Comment 7

[Torben]

*** Bug 171152 has been marked as a duplicate of this bug. ***

Comment 8

[Andrea Schafferhans]

*** Bug 203755 has been marked as a duplicate of this bug. ***

Comment 9

[Matthias Versen [:Matti]]

*** Bug 208362 has been marked as a duplicate of this bug. ***

Comment 10

[Matthias Versen [:Matti]]

*** Bug 213028 has been marked as a duplicate of this bug. ***

Comment 11

[Matthias Versen [:Matti]]

*** Bug 219890 has been marked as a duplicate of this bug. ***

Comment 12

[Matthias Versen [:Matti]]

*** Bug 200121 has been marked as a duplicate of this bug. ***

Comment 13

[Matthias Versen [:Matti]]

*** Bug 228852 has been marked as a duplicate of this bug. ***

Comment 14

[John G. Myers]

Mass reassign ssaux bugs to nobody

Comment 15

[Jesiah S]

*** Bug 236072 has been marked as a duplicate of this bug. ***

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Mass change "Future" target milestone to "--" on bugs that now are assigned to
nobody.  Those targets reflected the prioritization of past PSM management.
Many of these should be marked invalid or wontfix, I think.

Comment 17

[Matthias Versen [:Matti]]

*** Bug 244560 has been marked as a duplicate of this bug. ***

Comment 18

[Phil Randal]

If we're not going to support OCSP via http proxy, can code be added to Firefox
to  ensure that any "Use OCSP ..." options are ignored if an http proxy is in
use?  If it isn't going to work anyhow (and it doesn't with Firefox 0.9.1)
there's no point in trying.

Comment 19

[Josh Birnbaum]

*** Bug 255291 has been marked as a duplicate of this bug. ***

Comment 20

[zimon]

Oh geez, filed a Bug 255291, but it seems this is an old issue (2001-11-21).
The text in the error dialog box could be more verbose, or as Phil wrote just
tell there this won't be fixed and OCSP should be disabled if proxies has to be
used.

It seems (with ethereal) just normal "POST HTTP/1.0" request with "HTTP/1.0 200
OK" reply, so would it break anything if it would go now through a proxy anyway
although RFC of OCSP doesn't tell one MAY use proxies?

Comment 21

[Matthias Versen [:Matti]]

*** Bug 256123 has been marked as a duplicate of this bug. ***

Comment 22

[Daniel Veditz [:dveditz]]

*** Bug 259812 has been marked as a duplicate of this bug. ***

Comment 23

[Josh Birnbaum]

*** Bug 235715 has been marked as a duplicate of this bug. ***

Comment 24

[Josh Birnbaum]

*** Bug 256804 has been marked as a duplicate of this bug. ***

Comment 25

[simon annear]

With IE, you get a warning that the cert could not be verified - but you have
the option of continuing anyway.  I'd like to suggest that this could be
implemented.

Comment 26

[Dave Miller [:justdave]]

*** Bug 273470 has been marked as a duplicate of this bug. ***

Comment 27

[Serge Gautherie (:sgautherie)]

I got this |Error establishing an encrypted connection to client.noos.fr. Error
code: -5990.| error while I had to register my new cable-modem
(<https://client.noos.fr/echange_modem/login.jsp>) in order to gain access to
the Internet:
I had to use _Netscape v4_ (or MsIE v5) instead of Mozilla :-(

That was with MAS v1.76 and v1.8b2-0325.

I fully support what has been said so far:
either change the error message to something more helpful,
or fix the underlying issue if possible (by disabling the feature, or making it
to work).

Comment 28

[Peter Kovář]

But this is so 1337!

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> But this is so 1337!

What do you mean ? (Is that a spamming comment ?)

Comment 30

[Hans van Walderveen]

*** Bug 295451 has been marked as a duplicate of this bug. ***

Comment 31

[Jo Hermans]

*** Bug 296026 has been marked as a duplicate of this bug. ***

Comment 32

[Kai Engert (:KaiE:)]

Wan-Teh, what are thoughts about enabling NSS to work with a proxy configured in
the client software?

Comment 33

[Kai Engert (:KaiE:)]

Please ignore my previous comment, I didn't realize there is already a dependent
NSS bug.

(And for Serge, regarding your question about "1337", that's hacker speech,
means "leet", see the entry for leet in wikipedia if you're interested. :-) )

Comment 34

[Wan-Teh Chang]

Kai, please see bug 299150 comment 3 and bug 299150 comment 4
for our current proposal for how this problem should be solved.
Perhaps bug 299150 should be marked as a duplicate of this bug.

Comment 35

[OstGote!]

*** Bug 307638 has been marked as a duplicate of this bug. ***

Comment 36

[Tim Miller]

Is there a current status on this bug?  This enhancement is critical for DoD applications utilizing the NSS component.

Comment 37

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Experimental patch.

This patch has a dependency. It requires a change in NSS, see bug 152426 for a proposed patch and discussion.

Comment 38

[Kai Engert (:KaiE:)]

This patch, in theory, should work with any kind of proxy that Mozilla is configured to use.

However, it increases the complexity within PSM. 

As of today, whenever Necko asks PSM to do SSL I/O, NSS will be called directly. Necko is blocked while NSS does its work.

Should NSS decide that talking to an OCSP is necessary, Necko will still be blocked. This means, a callback from NSS to PSM/Necko, asking to obtain the OCSP data, does not work.

The idea behind this patch is to "unwind" Necko from PSM.

A new SSL thread is being introduced. Whenever SSL read/write I/O is requested by Necko, the request will be communicated to the separate SSL thread.

PSM immediately gives control back to Necko, with a status of "would block", meaning, no data is available at the moment, but Necko should come back later.

Should NSS or PSM decide to block I/O for a while, either because of showing an UI to the user, or because requesting OCSP data is necessary, this will not affect Necko. Necko will still be able to function with any protocols (that do not require SSL).

This allows us to use Necko's HTTP engine to obtain the required OCSP data, and Necko will use the configured proxy.

In the initial implementation of this patch, there is only one SSL thread. This means, as soon as the first SSL I/O blocks on UI or OCSP, no other SSL I/O will be possible for the duration of the block, and will be delayed until the block goes away (UI will not appear as blocked).

I would like to start with this initial implementation, that makes it work in the basic situation.

At a later time, if required, we can extend the logic to allow more than one SSL thread. This could allow us to fetch OCSP data from another SSL server, or allow the use of one SSL window, while another one is blocked (like on showing a cert mismatch window).

Comment 39

[Kai Engert (:KaiE:)]

Comment on attachment 204058 [details] [diff] [review]
Patch v1

Hi Darin, I'm very interested in your feedback. Sorry, it's a large patch. Thanks!

Comment 40

[Kai Engert (:KaiE:)]

Comment on attachment 204058 [details] [diff] [review]
Patch v1

Discussed this patch with Wan-Teh, needs more work.

Comment 41

[Kai Engert (:KaiE:)]

Attachment: Patch v2

This patch works for me on Linux.
It's not yet the final version:
- the NSS portions of the patch will have to be landed in a NSS release
- the comments in the patch are not up to date with the logic I have actually used

I'm about to test this patch on Mac and Windows.

The patch is complicated. I plan to write a comment that explains how it works, so it is easier to review the patch.

Although there is still work to do, I'm attaching the patch now, so I have a backup in Bugzilla, and you can have an early look, if you would like. Feedback more than welcome.

Comment 42

[Kai Engert (:KaiE:)]

Comment on attachment 208974 [details] [diff] [review]
Patch v2

The patch works on Windows and Mac, too! 

(A fix to nss.def is required.)

I tested using a standard http proxy, activating ocsp to verify certs that specify an ocsp service url, and tested with https://www.wellsfargo.com and https://us.etrade.com

But I found a bug on all platforms, the application runs into a deadlock when I try to open page info and click on the view certificate button.

Also I see a lot of warnings about nsStandardUrl not being threadsafe.

Comment 43

[Kai Engert (:KaiE:)]

Attachment: Patch v3

The deadlock I reported was not created by my patch.
The cause was the fact, that all network protocol calls have to go through the main thread, however, when requesting ocsp information from the main thread, which blocks on the result, the network callback into necko does not succeed.

I introduced another thread, and made the UI request the data asynchronously, with a callback to JS once data is available.

I also fixed the reported assertions. All callback code to Necko is now executed on the main thread, as required.

This patch works for me on Linux.
I will test the patch on Windows and Mac tomorrow, hopefully it works, and then I'm ready to request reviews.

Not this patch also includes the HTTP interface portions that must be added to NSS, this portion has also been attached to the dependent NSS bug, and will be discussed there.

Comment 44

[Kai Engert (:KaiE:)]

Comment on attachment 209405 [details] [diff] [review]
Patch v3

This patch works for me on Windows and Mac, too!

Marking as obsolete, because I need to clean up the code before I request reviews.

Comment 45

[Kai Engert (:KaiE:)]

I plan to produce Firefox, Thunderbird and Seamonkey test builds based on the mozilla 1.8 branch, hopefully by tomorrow.

If you'd like to see OCSP-over-proxy working in official mozilla software as soon as possible, please consider to help testing. Please let me know by private mail which operating system you'd test on, and what kind of proxy you'll use in your environment. If you don't have a proxy environment, you could help by verifying that standard https still works correctly.

Comment 46

[Kai Engert (:KaiE:)]

Attachment: Patch v4

This patch requires the latest patch from bug 152426.
I'm ready to request reviews.

Comment 47

[Kai Engert (:KaiE:)]

Attachment: Motivation and Explanation of the proposed solution.

I've written this document that explains the background and the ideas used in the patch.

Comment 48

[Kai Engert (:KaiE:)]

Attachment: Text document explaining the patch.

This "guide to the patch" text document is meant to provide assistance for everybody who wants to read and review the patch.

Comment 49

[Kai Engert (:KaiE:)]

Comment on attachment 209707 [details] [diff] [review]
Patch v4

Darin, could you please review this patch? It's a large patch, therefore I created text documents that are meant to ease the reviewing process. I'd appreciate your comments very much. Thanks in advance.

Comment 50

[Kai Engert (:KaiE:)]

I had produced test builds based on Patch v3, and Timothy Miller was able to use them successfully with OCSP enabled in an environment that requires proxies. Thanks a lot for testing them.

I have now produced various test builds of Firefox 1.5+, Thunderbird 1.5+ and Seamonkey, all based on the latest MOZILLA_1_8_BRANCH, for Linux, Mac OS X and Windows:
  https://kuix.de/mozilla/ocspproxy/

If you can, please download, test and provide feedback. No installation is required, you should be able to just download, extract and run.

I'm particularly interested whether you experience any worsening of performance, when using standard https, without ocsp, without proxies.

Thanks.

Comment 51

[Kai Engert (:KaiE:)]

Comment on attachment 209707 [details] [diff] [review]
Patch v4

The patch requires more work.
I ran into problems when testing the mail/ssl code.
Problem is, during runtime, there is a period of time, where the socket is tricked to be a pollable event.
I initially hoped, the application would only call (poll, read, write) during that period of time.
Unfortunately, due to the multithreaded nature of the application, it occurrs that the mail code calls "getsockname" on the FD while we still have the pollable event in place.
This makes it necessary to introduce an additional FD layer above the pollable event, where we direct some more calls to the real ssl socket, but still have read/write disabled in that layer.

In addition there is a problem at application shutdown time, it must be ensured that all sockets are destroyed before NSS gets shut down.

Comment 52

[Kai Engert (:KaiE:)]

Attachment: Patch v5

New patch, seems to fix the issues I mentioned.
I'll add a comment that explains the changes tomorrow, and will provide new test builds.

Comment 53

[Kai Engert (:KaiE:)]

I've uploaded new test builds, for Linux, Mac, Windows of Firefox, Thunderbird, Seamonkey.

They are based on yesterday's MOZILLA_1_8_BRANCH plus "Patch v5".

http://kuix.de/mozilla/ocspproxy/20060202/

Please test and provide feedback whether you see any regressions in SSL - even if you're not using OCSP, your feedback would be very much appreciated.

Comment 54

[Kai Engert (:KaiE:)]

Attachment: Patch v6

For the first time while running Seamonkey with "Patch v5", I ran into a problem.

I ended up in a busy loop in NSPR, ptio.c, pt_poll_now, with the following stack trace:

#0  pt_Continue (op=0xb5e75354) at ../../.././mozilla/nsprpub/pr/src/pthreads/ptio.c:635
#1  0x02cdee6f in pt_Write (fd=0x9e35c40, buf=0x2ce7acd, amount=1) at ../../.././mozilla/nsprpub/pr/src/pthreads/ptio.c:1347
#2  0x02cc9376 in PR_Write (fd=0x9e35c40, buf=0x2ce7acd, amount=1) at ../../.././mozilla/nsprpub/pr/src/io/priometh.c:146
#3  0x02ccba75 in PR_SetPollableEvent (event=0x9e35c00) at ../../.././mozilla/nsprpub/pr/src/io/prpolevt.c:498
#4  0x04a51e4d in nsSSLThread::Run (this=0x9e36068) at nsSSLThread.cpp:903
#5  0x04a5205d in nsSSLThread::nsSSLThreadRunner (arg=0x9e36068) at nsSSLThread.cpp:49
#6  0x02ce29dd in _pt_root (arg=0x9e36170) at ../../.././mozilla/nsprpub/pr/src/pthreads/ptthread.c:220
#7  0x00c0bb80 in start_thread () from /lib/libpthread.so.0
#8  0x00a7c9ce in clone () from /lib/libc.so.6

The operating system poll returns -1, but errno is neither EINTR nor EAGAIN.
Unfortunately the debugger is unable to print the value or errno.

I have a speculation about the cause.

"Patch v5" calls PR_SetPollableEvent very often, but never calls PR_WaitForPollableEvent.
Necko calls poll on the FD and will wake up.
I looked at the implementation of pollable events on Linux, and noticed that it works using a socket. SetEvent will write a byte to the socket. WaitForEvent consumes a byte. If I understand correctly, poll calls will not consume bytes.

I conclude, the reason for not being able to write additional bytes to the local socket: The network buffer provided by the operating system had filled up and the operating system decided not to allow further writing.

I conclude it is necessary to match calls to PR_SetPollableEvent with calls to PR_WaitForPollableEvent to clear the event.

"Patch v6" adds a new PRBool mPollableEventSet to accomplish that.

Comment 55

[Kai Engert (:KaiE:)]

Attachment: Patch v7

Oops, there was a typo in the previous patch.

Note all patches are against 1.8 branch, but they apply to the trunk, there is just a small conflict due to the fact that on the trunk, we implement additional new interfaces.

Comment 56

[Darin Fisher]

My apologies for not getting to this code review yet.  I hope to get a slice of time to look at it shortly.

Comment 57

[Kai Engert (:KaiE:)]

I uploaded more recent test builds, they have the latest patch, and are either aginst 1_8 branch or the trunk as of Tuesday.
  http://kuix.de/mozilla/ocspproxy/20060214/

Comment 58

[Kai Engert (:KaiE:)]

Patch does partly fix bug 101406 and is the groundwork to provide a full fix later.

Comment 59

[Kai Engert (:KaiE:)]

A document that explains the new HTTP delegation interface leveraged by this patch: http://developer.mozilla.org/en/docs/HTTP_Delegation

Comment 60

[Kai Engert (:KaiE:)]

Attachment: Patch v8

I did another full review of my own patch...

I added more error checking, did some cleanup, fixed an incorrect use of a variable, fixed a memory cleanup issue, renamed variables for clarity, moved duplicated code to a shared function.

Comment 61

[Darin Fisher]

Comment on attachment 212604 [details] [diff] [review]
Patch v8

This is a big patch.  I've skimmed it, and I don't see any major problems with it.  Of course, the devil is in the details (especially when it comes to MT code), so I'll try to find the time to do a thorough review shortly.

Comment 62

[Kai Engert (:KaiE:)]

Attachment: Patch v9

I encountered another situation where the application blocks, because the main thread does a blocking certificate verification request. As explained before, this is not allowed, because we need the main thread to remain responsive, to allow Necko to process the HTTP request.

In order to prevent a deadlock, this new patch revision has a small additional block of code. Look for nsIThread::IsMainThread(). It will refuse to do cert verification when run from the wrong thread. It will display an error message to the user and return failure to NSS. This ensures the application will remain usable.

The scenario that caused this situation was verification of a signed email message, where the signature certificate specifies an OCSP responder. I will work on the additional mail code to unwind the verification in a separate bug/patch.

Comment 63

[Chandrasekar Kannan]

Kai,

per your suggestion, I did run through our mozilla basic security 
smoke tests with these builds. Seems ok to me. 

Will do the actual ocsp over proxy tests soon. 

###################################################################

>>Hi Bob, Chandra,
>>
>>no, I did not think of that possibility!

>>Chandra, if you are able to, it would help me very much if you could do some testing of test builds that contain the OCSP over proxy solution. I uploaded test builds to http://kuix.de/mozilla/ocspproxy/20060214/
>>
>>You don't need to enable OCSP for your testing, I'm concerned about SSL, because the solution changes the way the application communicates using SSL.

>>What I'm mostly interested in:
>>- Do you experience any problems when browsing secure sites?
>>- Do you have a subjective feeling that SSL is slower than usual, or it is still going at the same speed?

>>http://kuix.de/mozilla/ocspproxy/20060214/linux_fedora_core4/firefox-1.5.en-US.linux-i686.tar.gz
>>http://kuix.de/mozilla/ocspproxy/20060214/mac_osx/firefox-1.5.en-US.mac.dmg
>>http://kuix.de/mozilla/ocspproxy/20060214/win32/firefox-1.5.en-US.win32.zip

>>Thanks a lot,
>>Kai 
#################################################################

Comment 64

[Chandrasekar Kannan]

Attachment: smoke test report

Comment 65

[Kai Engert (:KaiE:)]

Attachment: Patch v10

As mentioned in comment 62, additional code is required to make rendering S/Mime messages work, without blocking.

I filed bug 329990 so the required mail changes can be discussed separately.

However, some changes were required to the patch in here. They are:

- Generalize the CertVerificationThread to run arbitrary verification jobs
  This introduced new file nsVerificationJob.h,
  and some code was moved in file nsCertVerificationThread.cpp

- Added new interface nsICMSMessage2 to spawn background verification
  This is just a simple layer around existing code.

Comment 66

[Darin Fisher]

Attachment: darin's initial review comments on Patch v10

Some incomplete review comments from me.  Sorry it took so long for me to get these to you!

Comment 67

[Kai Engert (:KaiE:)]

Attachment: kai's answer to darin's review comments on patch v10

Darin, thanks a lot for your comments!
Here is a text document with my answers.
I have addressed most of your comments, and would like to postpone some interface cleanup. Please see the header in the text document for details.

Comment 68

[Kai Engert (:KaiE:)]

Attachment: Patch v11

> I've stopped reviewing here at the top of nsSSLThread.cpp.
> I can pick up from there later, or if you want, I can wait until you 
> digest my current set of review comments.

You've reviewed most of the patch already, and you found several issues, so it was really helpful.

If you have some more time, can you please look at nsSSLThread.cpp, too?
Because that is the part with the tricky I/O code.
Thanks in advance!

Comment 69

[Darin Fisher]

For XXX-LATER items, I recommend filing bugs :)  I'll try to review the rest shortly.

Comment 70

[Kai Engert (:KaiE:)]

> For XXX-LATER items, I recommend filing bugs

Files bug 330503

Comment 71

[Darin Fisher]

Attachment: more review feedback from darin on patch v11

Comment 72

[Kai Engert (:KaiE:)]

Attachment: kai's answer to darin's review comments on patch v11

Comment 73

[Kai Engert (:KaiE:)]

Attachment: Patch v12

Updated patch, as explained in the text file.

> >+PRInt32 nsSSLThread::requestRead(nsNSSSocketInfo *si, void *buf, PRInt32 amount)
> 
> Stopped here.  Sorry, will try to finish soon, but maybe not until next week.

Looking forward to your review of the final section. Thanks!

Comment 74

[Darin Fisher]

I read through your responses, and it all sounds good.  I'll try to complete the code review tomorrow.  Cross your fingers :)

Comment 75

[Darin Fisher]

Comment on attachment 215971 [details] [diff] [review]
Patch v12

r=darin for everything but the SSL thread part.

Comment 76

[Kai Engert (:KaiE:)]

Comment on attachment 215971 [details] [diff] [review]
Patch v12

Bob, could you please review nsSSLThread.h and nsSSLThread.cpp?

Comment 77

[Robert Relyea]

Comment on attachment 215971 [details] [diff] [review]
Patch v12

r+ = relyea for the nsSSLIOLayer stuff (including the SSLThread code) with the following comments:

This code is sufficient to support necko, but there are a couple of areas that could be address if it is possible for pluggins to call this code:

1) recv and send can easily be "fully" supported using the existing read and write interfaces. SSL only supports the PEEK flag, for recv, so in the non-peek case (the only case not currently supported by this function) we can just call our read function. I'm not sure which flags SSL supports for send, so we may want to pass the flag and call send. SSL maps write to a send with no flags).

2) Pluggins handshaking as servers. There is an existing bug that is trying to add this functionality (it does not yet exist). In order for this to work accept and listen will have to be added. It should be safe to call the SSL functions directly for these as SSL handshakes are only initiated on reads and writes.

3) Short writes. Currently the code starts a write, but tells the application no data was written. For most well behaved applications, this means the application will retry the original data. It is possible an application may decide not to retry the data, however, in which case there will be and inconsistant state between the application at the server it is talking to. One way to get around this is to return a partial write back to the application, so the application knows that most of the data has already been sent, but it still needs to retry the rest of the buffer.

None of these are issues for Necko's usage. Issue 2 most likely will not affect existing pluggins since that support does not exist, and issue 3 is pretty rare.

Minor nits:

1) The realSSLFD thread code is crying out to be a helper functions. That would reduce most of your nsSSLThread:: fucntions to something like:

PRStatus nsSSLThread::XXXXXRequest(nsNSSScoketInfo *si, params)
{
    PRFileDesc *reaSSLThread = getRealThread(si);
    if (!realSSLThread) 
       return PR_FAILURE;
    return realSSLThread->methods->XXXXXRequest(realSSLThread, params);
}

nsNSSSocketInfo::CloseSocketAndDestroy() is a class function which takes a pointer to an nsNSSSocketInfo *, is there a reason it isn't just a member function that operates on the 'this' pointer?

!nPSMBackgroundThread should assert of mThead is not null and requestExit should set mThread to NULL after the Join(). (just a paranoia operation here).

The nsSSLThread::requestXXXX code (RecvMsgPeek, Read, Write) are examining both an NSPR error and an SSL error. This is redundant. PORT_SetError maps to PR_SetError, so there is only one error code. This is not a bug because the mSSLErrorCode does not seem to be set anyway, just redundant code.

Comment 78

[Kai Engert (:KaiE:)]

As Bob explained in comment 77, the proposed enhancements should not be required at this time, and I agree. I would like to address them at a later time and filed bug 332387.

However I addressed the nits that Bob mentioned:

> The realSSLFD thread code is crying out to be a helper functions.

Good idea, changed.

> nsNSSSocketInfo::CloseSocketAndDestroy() is a class function which takes a
> pointer to an nsNSSSocketInfo *, is there a reason it isn't just a member
> function that operates on the 'this' pointer?

Looking at the code again, you're right.
changed.

> !nPSMBackgroundThread should assert of mThead is not null 

Assertion added, although we have assertions in the derived classes, too.

> requestExit should set mThread to NULL after the Join()

added

> The nsSSLThread::requestXXXX code (RecvMsgPeek, Read, Write) are examining
> both an NSPR error and an SSL error. This is redundant. PORT_SetError maps 
> to PR_SetError, so there is only one error code. This is not a bug because 
> the mSSLErrorCode does not seem to be set anyway, just redundant code.

You're right, I never assign to mSSLErrorCode, therefore I removed it.

Comment 79

[Kai Engert (:KaiE:)]

Attachment: Patch v13

This patch addresses the nits as explained in the previous comment.

Darin's and Bob's reviews combined means this patch has been reviewed.

Thanks a lot for you help!!!

I will land this patch on the trunk shortly, although I consider to wait until I am able to land bug 329990 at the same time.

Comment 80

[Kai Engert (:KaiE:)]

Attachment: Patch v14

> > requestExit should set mThread to NULL after the Join()
> added

After having added that, I found that we don't close all sockets on exit.
Problem was closeSocket checked for mThreadHandle != nsnull.
This is wrong.
While we do not want to allow any I/O calls on the socket, after our thread terminated, we must still allow to close sockets after the thread has gone away.

This new Patch v14 removes the check for mThreadHandle != nsnull in the close function.

Comment 81

[Kai Engert (:KaiE:)]

checked in

Checking in locales/en-US/chrome/pipnss/pipnss.properties;
/cvsroot/mozilla/security/manager/locales/en-US/chrome/pipnss/pipnss.properties,v  <--  pipnss.properties
new revision: 1.10; previous revision: 1.9
done
Checking in pki/resources/content/viewCertDetails.js;
/cvsroot/mozilla/security/manager/pki/resources/content/viewCertDetails.js,v  <--  viewCertDetails.js
new revision: 1.26; previous revision: 1.25
done
Checking in ssl/public/Makefile.in;
/cvsroot/mozilla/security/manager/ssl/public/Makefile.in,v  <--  Makefile.in
new revision: 1.26; previous revision: 1.25
done
RCS file: /cvsroot/mozilla/security/manager/ssl/public/nsICMSMessage2.idl,v
done
Checking in ssl/public/nsICMSMessage2.idl;
/cvsroot/mozilla/security/manager/ssl/public/nsICMSMessage2.idl,v  <--  nsICMSMessage2.idl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/manager/ssl/public/nsIX509Cert3.idl,v
done
Checking in ssl/public/nsIX509Cert3.idl;
/cvsroot/mozilla/security/manager/ssl/public/nsIX509Cert3.idl,v  <--  nsIX509Cert3.idl
initial revision: 1.1
done
Checking in ssl/src/Makefile.in;
/cvsroot/mozilla/security/manager/ssl/src/Makefile.in,v  <--  Makefile.in
new revision: 1.71; previous revision: 1.70
done
Checking in ssl/src/nsCMS.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsCMS.cpp,v  <--  nsCMS.cpp
new revision: 1.22; previous revision: 1.21
done
Checking in ssl/src/nsCMS.h;
/cvsroot/mozilla/security/manager/ssl/src/nsCMS.h,v  <--  nsCMS.h
new revision: 1.9; previous revision: 1.8
done
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsCertVerificationThread.cpp,v
done
Checking in ssl/src/nsCertVerificationThread.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsCertVerificationThread.cpp,v  <--  nsCertVerificationThread.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsCertVerificationThread.h,v
done
Checking in ssl/src/nsCertVerificationThread.h;
/cvsroot/mozilla/security/manager/ssl/src/nsCertVerificationThread.h,v  <--  nsCertVerificationThread.h
initial revision: 1.1
done
Checking in ssl/src/nsNSSCallbacks.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp,v  <--  nsNSSCallbacks.cpp
new revision: 1.40; previous revision: 1.39
done
Checking in ssl/src/nsNSSCallbacks.h;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCallbacks.h,v  <--  nsNSSCallbacks.h
new revision: 1.9; previous revision: 1.8
done
Checking in ssl/src/nsNSSCertificate.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCertificate.cpp,v  <--  nsNSSCertificate.cpp
new revision: 1.117; previous revision: 1.116
done
Checking in ssl/src/nsNSSCertificate.h;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSCertificate.h,v  <--  nsNSSCertificate.h
new revision: 1.31; previous revision: 1.30
done
Checking in ssl/src/nsNSSComponent.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.cpp,v  <--  nsNSSComponent.cpp
new revision: 1.134; previous revision: 1.133
done
Checking in ssl/src/nsNSSComponent.h;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.h,v  <--  nsNSSComponent.h
new revision: 1.40; previous revision: 1.39
done
Checking in ssl/src/nsNSSIOLayer.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp,v  <--  nsNSSIOLayer.cpp
new revision: 1.103; previous revision: 1.102
done
Checking in ssl/src/nsNSSIOLayer.h;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSIOLayer.h,v  <--  nsNSSIOLayer.h
new revision: 1.28; previous revision: 1.27
done
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsPSMBackgroundThread.cpp,v
done
Checking in ssl/src/nsPSMBackgroundThread.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsPSMBackgroundThread.cpp,v  <--  nsPSMBackgroundThread.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsPSMBackgroundThread.h,v
done
Checking in ssl/src/nsPSMBackgroundThread.h;
/cvsroot/mozilla/security/manager/ssl/src/nsPSMBackgroundThread.h,v  <--  nsPSMBackgroundThread.h
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsSSLThread.cpp,v
done
Checking in ssl/src/nsSSLThread.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsSSLThread.cpp,v  <--  nsSSLThread.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsSSLThread.h,v
done
Checking in ssl/src/nsSSLThread.h;
/cvsroot/mozilla/security/manager/ssl/src/nsSSLThread.h,v  <--  nsSSLThread.h
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsVerificationJob.h,v
done
Checking in ssl/src/nsVerificationJob.h;
/cvsroot/mozilla/security/manager/ssl/src/nsVerificationJob.h,v  <--  nsVerificationJob.h
initial revision: 1.1
done

Comment 82

[Kai Engert (:KaiE:)]

Attachment: Patch v14 addon

There was a tinderbox test failure after checking in Patch v14.
Thanks a lot to Jag for his help in identifying the issue.

If the thread runner C function calls the pure virtual Run too early,
before the constructor finished, it will crash.
This patch delays thread creation and virtual function call to a
separate startThread call.

Thanks a lot to Jag for his help in finding the problem!

received r=jag on IRC for this incremental bustage patch

Comment 83

[Christian :Biesinger (don't email me, ping me on IRC)]

Please don't add unfrozen interfaces to SDK_XPIDLSRC (nsIX509Cert3.idl in this case). I fixed this on trunk now. (although I'm not sure why this interfaces exists, instead of just adding this on nsIX509Cert2, which isn't frozen)

Comment 84

[Kai Engert (:KaiE:)]

Biesi, thanks for this fix. 

> I'm not sure why this interfaces
> exists, instead of just adding this on nsIX509Cert2, which isn't frozen

We can't add the new code that was added with nsIX509Cert2 on 1.8 branch.

However we'd like to land the nsIX509Cert3 code on 1.8 branch.

Comment 85

[Kai Engert (:KaiE:)]

Attachment: 1.8 branch version of patch v14+addon + biesi's fix

Comment 86

[Boris Zbarsky [:bzbarsky]]

So is the plan to merge the two interfaces later on trunk?  Or just to leave them separate?

Comment 87

[Kai Engert (:KaiE:)]

I don't have a specific plan yet. I'm fine to leave them separate, and if it doesn't cause any problems, we can merge them. Please note, for IDL cleanup we already have bug 330503.

Comment 88

[Mike Shaver (:shaver emeritus)]

Comment on attachment 217427 [details] [diff] [review]
1.8 branch version of patch v14+addon + biesi's fix

branch=shaver

Comment 89

[Mike Shaver (:shaver emeritus)]

Fixed on 1.8.1, clearing blocking.