Support OCSP requests through a proxy

RESOLVED FIXED

Status

enhancement
P2
normal
RESOLVED FIXED
18 years ago
3 years ago

People

(Reporter: miikkak, Assigned: kaie)

Tracking

(Blocks 2 bugs, {fixed1.8.1})

Dependency tree / graph
Bug Flags:
blocking-aviary1.5 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [asaP2] [kerh-coa])

Attachments

(10 attachments, 13 obsolete attachments)

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
(Reporter)

Description

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

Comment 1

18 years ago
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

Updated

18 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

17 years ago
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
(Assignee)

Updated

17 years ago
Blocks: 157555
(Assignee)

Comment 3

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

Comment 4

17 years ago
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.
(Assignee)

Updated

17 years ago
Keywords: nsbeta1
(Assignee)

Comment 5

17 years ago
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.
(Assignee)

Updated

17 years ago
Depends on: 163717

Comment 6

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

Updated

17 years ago
Blocks: 151271

Comment 7

17 years ago
*** 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. ***
(Assignee)

Updated

16 years ago
Blocks: 220974
*** Bug 228852 has been marked as a duplicate of this bug. ***
No longer blocks: 151271
No longer blocks: 220974

Comment 14

15 years ago
Mass reassign ssaux bugs to nobody
Assignee: ssaux → nobody

Comment 15

15 years ago
*** 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. ***

Comment 18

15 years ago
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

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

Comment 20

15 years ago
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. ***

Comment 23

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

Comment 24

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

Comment 25

15 years ago
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.

Updated

15 years ago
Blocks: 273470
*** Bug 273470 has been marked as a duplicate of this bug. ***

Updated

14 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

14 years ago
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).

Comment 28

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

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

Updated

14 years ago
Whiteboard: [asaP2]
*** Bug 295451 has been marked as a duplicate of this bug. ***

Comment 31

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

Updated

14 years ago
Flags: blocking-aviary1.1? → blocking-aviary1.1-
(Assignee)

Comment 32

14 years ago
Wan-Teh, what are thoughts about enabling NSS to work with a proxy configured in
the client software?
(Assignee)

Comment 33

14 years ago
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

14 years ago
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

Comment 35

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

Comment 36

14 years ago
Is there a current status on this bug?  This enhancement is critical for DoD applications utilizing the NSS component.
(Assignee)

Updated

14 years ago
Whiteboard: [asaP2] → [asaP2] [kerh-coa]
(Assignee)

Comment 37

14 years ago
Posted 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
(Assignee)

Comment 38

14 years ago
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).

(Assignee)

Comment 39

14 years ago
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?
(Assignee)

Updated

14 years ago
Attachment #204058 - Flags: review? → review?(darin)
(Assignee)

Comment 40

14 years ago
Comment on attachment 204058 [details] [diff] [review]
Patch v1

Discussed this patch with Wan-Teh, needs more work.
Attachment #204058 - Flags: review?(darin)
(Assignee)

Updated

14 years ago
Attachment #204058 - Attachment is obsolete: true
(Assignee)

Comment 41

13 years ago
Posted 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.
(Assignee)

Comment 42

13 years ago
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
(Assignee)

Comment 43

13 years ago
Posted 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.
(Assignee)

Comment 44

13 years ago
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
(Assignee)

Comment 45

13 years ago
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.
(Assignee)

Comment 46

13 years ago
Posted patch Patch v4 (obsolete) — Splinter Review
This patch requires the latest patch from bug 152426.
I'm ready to request reviews.
(Assignee)

Comment 47

13 years ago
I've written this document that explains the background and the ideas used in the patch.
(Assignee)

Comment 48

13 years ago
This "guide to the patch" text document is meant to provide assistance for everybody who wants to read and review the patch.
(Assignee)

Comment 49

13 years ago
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)
(Assignee)

Updated

13 years ago
Flags: blocking1.8.1?
(Assignee)

Comment 50

13 years ago
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.
(Assignee)

Comment 51

13 years ago
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)
(Assignee)

Comment 52

13 years ago
Posted 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)
(Assignee)

Comment 53

13 years ago
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.
(Assignee)

Comment 54

13 years ago
Posted 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)
(Assignee)

Comment 55

13 years ago
Posted 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)

Comment 56

13 years ago
My apologies for not getting to this code review yet.  I hope to get a slice of time to look at it shortly.
(Assignee)

Comment 57

13 years ago
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/
(Assignee)

Comment 58

13 years ago
Patch does partly fix bug 101406 and is the groundwork to provide a full fix later.
Blocks: 101406
(Assignee)

Comment 59

13 years ago
A document that explains the new HTTP delegation interface leveraged by this patch: http://developer.mozilla.org/en/docs/HTTP_Delegation
(Assignee)

Comment 60

13 years ago
Posted 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)
(Assignee)

Updated

13 years ago
Attachment #212604 - Flags: ui-review?(darin) → superreview?(darin)

Comment 61

13 years ago
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.
(Assignee)

Comment 62

13 years ago
Posted 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)
(Assignee)

Updated

13 years ago
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 
#################################################################

(Assignee)

Updated

13 years ago
Blocks: 329990
(Assignee)

Comment 65

13 years ago
Posted 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)

Comment 66

13 years ago
Some incomplete review comments from me.  Sorry it took so long for me to get these to you!
(Assignee)

Comment 67

13 years ago
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.
(Assignee)

Comment 68

13 years ago
Posted 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)

Comment 69

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

Comment 70

13 years ago
> For XXX-LATER items, I recommend filing bugs

Files bug 330503
(Assignee)

Updated

13 years ago
Blocks: 330503

Updated

13 years ago
Attachment #214979 - Flags: review?(darin) → review-
(Assignee)

Comment 73

13 years ago
Posted 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)

Comment 74

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

Comment 75

13 years ago
Comment on attachment 215971 [details] [diff] [review]
Patch v12

r=darin for everything but the SSL thread part.
Attachment #215971 - Flags: review?(darin) → review+
(Assignee)

Comment 76

13 years ago
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 77

13 years ago
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+
(Assignee)

Comment 78

13 years ago
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.
(Assignee)

Updated

13 years ago
Blocks: 332387
(Assignee)

Comment 79

13 years ago
Posted 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)
(Assignee)

Comment 80

13 years ago
Posted 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+
(Assignee)

Updated

13 years ago
Depends on: 332607
(Assignee)

Comment 81

13 years ago
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
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 82

13 years ago
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)
(Assignee)

Comment 84

13 years ago
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?
(Assignee)

Comment 87

13 years ago
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.
(Assignee)

Updated

13 years ago
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+
(Assignee)

Updated

13 years ago
Keywords: fixed1.8.1
Fixed on 1.8.1, clearing blocking.
Flags: blocking1.8.1?
(Assignee)

Updated

13 years ago
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.