Closed Bug 111384 Opened 23 years ago Closed 19 years ago

Support OCSP requests through a proxy

Categories

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

Other Branch
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: miikkak, Assigned: KaiE)

References

Details

(Keywords: fixed1.8.1, Whiteboard: [asaP2] [kerh-coa])

Attachments

(10 files, 13 obsolete files)

3.76 KB, text/plain
Details
10.49 KB, patch
Details | Diff | Splinter Review
116.07 KB, application/pdf
Details
26.28 KB, text/plain
Details
25.66 KB, text/plain
Details
5.75 KB, text/plain
Details
7.27 KB, text/plain
Details
127.73 KB, patch
KaiE
: review+
Details | Diff | Splinter Review
3.81 KB, patch
KaiE
: review+
Details | Diff | Splinter Review
169.34 KB, patch
Details | Diff | Splinter Review
Is it possible to enhance OCSP support in that way it would work also behind firewall via HTTP proxy?
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.
Priority: -- → P2
Target Milestone: --- → Future
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Depends on: 152426
Blocks: 157555
*** Bug 157955 has been marked as a duplicate of this bug. ***
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.
Keywords: nsbeta1
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.
Depends on: 163717
*** Bug 156580 has been marked as a duplicate of this bug. ***
Blocks: 151271
*** Bug 171152 has been marked as a duplicate of this bug. ***
*** Bug 203755 has been marked as a duplicate of this bug. ***
*** Bug 208362 has been marked as a duplicate of this bug. ***
*** Bug 213028 has been marked as a duplicate of this bug. ***
*** Bug 219890 has been marked as a duplicate of this bug. ***
*** Bug 200121 has been marked as a duplicate of this bug. ***
Blocks: 220974
*** Bug 228852 has been marked as a duplicate of this bug. ***
No longer blocks: 151271
No longer blocks: 220974
Mass reassign ssaux bugs to nobody
Assignee: ssaux → nobody
*** Bug 236072 has been marked as a duplicate of this bug. ***
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.
Target Milestone: Future → ---
*** Bug 244560 has been marked as a duplicate of this bug. ***
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.
*** Bug 255291 has been marked as a duplicate of this bug. ***
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?
*** Bug 256123 has been marked as a duplicate of this bug. ***
*** Bug 259812 has been marked as a duplicate of this bug. ***
*** Bug 235715 has been marked as a duplicate of this bug. ***
*** Bug 256804 has been marked as a duplicate of this bug. ***
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.
Blocks: 273470
No longer blocks: 273470
*** Bug 273470 has been marked as a duplicate of this bug. ***
Product: PSM → Core
Flags: blocking-aviary1.1?
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).
But this is so 1337!
(In reply to comment #28) > But this is so 1337! What do you mean ? (Is that a spamming comment ?)
Whiteboard: [asaP2]
*** Bug 295451 has been marked as a duplicate of this bug. ***
*** Bug 296026 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Wan-Teh, what are thoughts about enabling NSS to work with a proxy configured in the client software?
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. :-) )
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.
No longer depends on: 163717
Summary: OCSP with proxy? → Support OCSP requests through a proxy
*** Bug 307638 has been marked as a duplicate of this bug. ***
Is there a current status on this bug? This enhancement is critical for DoD applications utilizing the NSS component.
Whiteboard: [asaP2] → [asaP2] [kerh-coa]
Attached patch Patch v1 (obsolete) — Splinter Review
Experimental patch. This patch has a dependency. It requires a change in NSS, see bug 152426 for a proposed patch and discussion.
Assignee: nobody → kengert
Status: NEW → ASSIGNED
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 on attachment 204058 [details] [diff] [review] Patch v1 Hi Darin, I'm very interested in your feedback. Sorry, it's a large patch. Thanks!
Attachment #204058 - Flags: review?
Attachment #204058 - Flags: review? → review?(darin)
Comment on attachment 204058 [details] [diff] [review] Patch v1 Discussed this patch with Wan-Teh, needs more work.
Attachment #204058 - Flags: review?(darin)
Attachment #204058 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
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 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.
Attachment #208974 - Attachment is obsolete: true
Attached patch Patch v3 (obsolete) — Splinter Review
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 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.
Attachment #209405 - Attachment is obsolete: true
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.
Attached patch Patch v4 (obsolete) — Splinter Review
This patch requires the latest patch from bug 152426. I'm ready to request reviews.
I've written this document that explains the background and the ideas used in 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 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.
Attachment #209707 - Flags: review?(darin)
Flags: blocking1.8.1?
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 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.
Attachment #209707 - Attachment is obsolete: true
Attachment #209707 - Flags: review?(darin)
Attached patch Patch v5 (obsolete) — Splinter Review
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.
Attachment #210433 - Flags: review?(darin)
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.
Attached patch Patch v6 (obsolete) — Splinter Review
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.
Attachment #210433 - Attachment is obsolete: true
Attachment #211823 - Flags: superreview?(darin)
Attachment #211823 - Flags: review?(wtchang)
Attachment #210433 - Flags: review?(darin)
Attached patch Patch v7 (obsolete) — Splinter Review
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.
Attachment #211823 - Attachment is obsolete: true
Attachment #211835 - Flags: superreview?(darin)
Attachment #211835 - Flags: review?(wtchang)
Attachment #211823 - Flags: superreview?(darin)
Attachment #211823 - Flags: review?(wtchang)
My apologies for not getting to this code review yet. I hope to get a slice of time to look at it shortly.
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/
Patch does partly fix bug 101406 and is the groundwork to provide a full fix later.
Blocks: 101406
A document that explains the new HTTP delegation interface leveraged by this patch: http://developer.mozilla.org/en/docs/HTTP_Delegation
Attached patch Patch v8 (obsolete) — Splinter Review
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.
Attachment #211835 - Attachment is obsolete: true
Attachment #212604 - Flags: ui-review?(darin)
Attachment #212604 - Flags: review?(wtchang)
Attachment #211835 - Flags: superreview?(darin)
Attachment #211835 - Flags: review?(wtchang)
Attachment #212604 - Flags: ui-review?(darin) → superreview?(darin)
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.
Attached patch Patch v9 (obsolete) — Splinter Review
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.
Attachment #212604 - Attachment is obsolete: true
Attachment #212901 - Flags: ui-review?(darin)
Attachment #212901 - Flags: review?(wtchang)
Attachment #212604 - Flags: superreview?(darin)
Attachment #212604 - Flags: review?(wtchang)
Attachment #212901 - Flags: ui-review?(darin) → superreview?(darin)
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 #################################################################
Attached file smoke test report
Blocks: 329990
Attached patch Patch v10 (obsolete) — Splinter Review
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.
Attachment #212901 - Attachment is obsolete: true
Attachment #214630 - Flags: superreview?(darin)
Attachment #214630 - Flags: review?(wtchang)
Attachment #212901 - Flags: superreview?(darin)
Attachment #212901 - Flags: review?(wtchang)
Some incomplete review comments from me. Sorry it took so long for me to get these to you!
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.
Attached patch Patch v11 (obsolete) — Splinter Review
> 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!
Attachment #214630 - Attachment is obsolete: true
Attachment #214979 - Flags: superreview?(dougt)
Attachment #214979 - Flags: review?(darin)
Attachment #214630 - Flags: superreview?(darin)
Attachment #214630 - Flags: review?(wtchang)
For XXX-LATER items, I recommend filing bugs :) I'll try to review the rest shortly.
> For XXX-LATER items, I recommend filing bugs Files bug 330503
Blocks: 330503
Attachment #214979 - Flags: review?(darin) → review-
Attached patch Patch v12 (obsolete) — Splinter Review
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!
Attachment #214979 - Attachment is obsolete: true
Attachment #215971 - Flags: superreview?(dougt)
Attachment #215971 - Flags: review?(darin)
Attachment #214979 - Flags: superreview?(dougt)
I read through your responses, and it all sounds good. I'll try to complete the code review tomorrow. Cross your fingers :)
Comment on attachment 215971 [details] [diff] [review] Patch v12 r=darin for everything but the SSL thread part.
Attachment #215971 - Flags: review?(darin) → review+
Comment on attachment 215971 [details] [diff] [review] Patch v12 Bob, could you please review nsSSLThread.h and nsSSLThread.cpp?
Attachment #215971 - Flags: review?(rrelyea)
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.
Attachment #215971 - Flags: review?(rrelyea) → review+
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.
Blocks: 332387
Attached patch Patch v13 (obsolete) — Splinter Review
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.
Attachment #215971 - Attachment is obsolete: true
Attachment #216854 - Flags: review+
Attachment #215971 - Flags: superreview?(dougt)
Attached patch Patch v14Splinter Review
> > 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.
Attachment #216854 - Attachment is obsolete: true
Attachment #216905 - Flags: review+
Depends on: 332607
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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attached patch Patch v14 addonSplinter Review
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
Attachment #217141 - Flags: review+
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)
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.
So is the plan to merge the two interfaces later on trunk? Or just to leave them separate?
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.
Attachment #217427 - Flags: approval-branch-1.8.1?(shaver)
Comment on attachment 217427 [details] [diff] [review] 1.8 branch version of patch v14+addon + biesi's fix branch=shaver
Attachment #217427 - Flags: approval-branch-1.8.1?(shaver) → approval-branch-1.8.1+
Keywords: fixed1.8.1
Fixed on 1.8.1, clearing blocking.
Flags: blocking1.8.1?
Blocks: 140247
Depends on: 342646
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: