Instead of the bad cert page an alert dialog explaining there is a problem with the certificate is displayed. If you want to add exception for the cert manually (there is no other way than manually to do it) it doesn't work, the button to add exception is still disabled. This happens always when I setup proxy location as 'localhost:someport'. Easy workaround is to replace 'localhost' with '127.0.0.1', from that moment all problems disappears. Just a note this also applies to FindProxyForURL autoconfig. The dialog is shown from here http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSIOLayer.cpp#1329 because socketInfo->GetExternalErrorReporting fails because of missing mCallbacks in socketInfo. See http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSIOLayer.cpp#340. I will find out why this happens. I discovered this while writing test for bug 413909 based on mochitest ssl support from bug 428009. This behavior is present using ssltunnel in http proxy mode or with any other http proxy running on localhost.

I figured out on windows and on mac the 'cause' was installed ipv6 support. After I uninstall it I could not reproduce the problem any more. The cause why mCallbacks is null is because the proxy is not listening on ::1 and first attempt to connect it fails with NSPR error -5981 (PR_CONNECT_REFUSED_ERROR). nsSocketTransport::OnSocketDetached is then called where callbacks are released (set null). nsSocketTransport::RecoverFromError then moves to try another address from the list (127.0.0.1) and continues the connection - the socket transport does not die, but is not any longer join with the http connection. We should release the callbacks after failure of RecoverFromError and not sooner. This bug is cased by fix for bug 412207.

Tested with chatzilla (no more leaks then w/o the patch) and also with test for bug 413909 that is unable to work w/o this patch.

I went from #439028 where i cannot add exception for an ipv6 enabled https website and tried the patch attached. Unfortunatly it fixes anything here, i cannot add exception and thus browse in this website with firefox3.

Not blocking, but wanted. We need to get this patch reviewed and landed on trunk first... (And tests! More tests!)

Can you please use more context next time, and also use the -p flag for the diff? This patch does not make sense. Either it is safe not to null out those two members, then we should never do it. Or it is unsafe, then this is a memory leak. Was comment 1 incorrect? This patch does the release if RecoverFromError succeeded, not when it failed.

Yes, you are right. Sorry, I probably submitted a testing patch. The correct fix is to clean mCallbacks and mEventSink only when we could not recover from error - mCondition fails.

Comment on attachment 336642 [details] [diff] [review] Fix, v2 s/loose/lose/ and sr=bzbarsky

Addressed BZ's comment change.

This needs to land on trunk before landing on the 1.9.0 branch. Additionally, why should we take this in 1.9.0? What direct effect does it have? How risky is it? Is there a testcase for it (automated or manual, but preferably automated)?

(In reply to comment #13) > This needs to land on trunk before landing on the 1.9.0 branch. > I set checkin-needed keyword to land on trunk first. > Additionally, why should we take this in 1.9.0? This bug is present on 1.9.0 too and the bug is marked as wanted 1.9.0.x+. > What direct effect does it have? Sorry, don't exactly understand this question. It just release the mCallbacks and mEventSink members when socket ends its lifetime and not between connection attempts what breaks bond with UI and security manager. > How risky is it? Very low. It is always ensured that those two, now conditionally, released members are threw away when socket (transport) is about to die. mCondition is failure in all cases but the one we recover from error. > Is there a testcase for it (automated or manual, but preferably automated)? No, I have to figure out how to create an automated test for this.

Comment on attachment 341289 [details] [diff] [review] Fix, v2.1 [Checkin: Comment 15] http://hg.mozilla.org/mozilla-central/rev/f4821e598b76 with additional s/RecoveFromError/RecoverFromError/

To have a test for this we can use property of the mochitest/pgo server, the way I originally found this bug. It uses proxy on localhost while that proxy is listening just on 127.0.0.1 and not on ::1 even IPv6 support is enabled on the machine. localhost name is resolved to "::1" and "127.0.0.1" in this order. This will work but we are strongly dependent on the testing platform properties and we have to enable the ipv6 support on all test boxes and rely on OS to provide IPs in demanded order. Other way is to use a direct connection to a host name with several IP addresses and just the last one be accessible. In that case we need to modify hosts file on the machine.

Comment on attachment 341289 [details] [diff] [review] Fix, v2.1 [Checkin: Comment 15] not approving for branch, no test and doesn't seem to meet the branch criteria.

Also minusing for wanted since it doesn't meet the criteria.