Certificate error page not displayed while using proxy on 'localhost'

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
Security: PSM
--
major
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
mozilla1.9.1b2
Points:
---
Bug Flags:
blocking1.9.0.2 -
wanted1.9.0.x -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

Comment 2

9 years ago
Created attachment 322795 [details] [diff] [review]
Fix, v1

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.
Attachment #322795 - Flags: review?(cbiesinger)

Updated

9 years ago
Duplicate of this bug: 439902
Duplicate of this bug: 442126
(Assignee)

Updated

9 years ago
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.1?
(Assignee)

Updated

9 years ago
Duplicate of this bug: 439028

Updated

9 years ago
Duplicate of this bug: 438008

Comment 7

9 years ago
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.
Flags: blocking1.9.0.1? → blocking1.9.0.2?
Not blocking, but wanted. We need to get this patch reviewed and landed on trunk first... (And tests! More tests!)
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.2?
Flags: blocking1.9.0.2-
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.
(Assignee)

Comment 10

9 years ago
Created attachment 336642 [details] [diff] [review]
Fix, v2

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.
Attachment #322795 - Attachment is obsolete: true
Attachment #336642 - Flags: review?(cbiesinger)
Attachment #322795 - Flags: review?(cbiesinger)
Attachment #336642 - Flags: review?(cbiesinger) → review+
(Assignee)

Updated

9 years ago
Attachment #336642 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1?
Comment on attachment 336642 [details] [diff] [review]
Fix, v2

s/loose/lose/ and sr=bzbarsky
Attachment #336642 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 12

9 years ago
Created attachment 341289 [details] [diff] [review]
Fix, v2.1
[Checkin: Comment 15]

Addressed BZ's comment change.
Attachment #336642 - Attachment is obsolete: true
Attachment #341289 - Flags: approval1.9.0.4?
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)?
(Assignee)

Comment 14

9 years ago
(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.
Keywords: checkin-needed
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/
Attachment #341289 - Attachment description: Fix, v2.1 → Fix, v2.1 [Checkin: Comment 15]
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite?
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
(Assignee)

Comment 16

9 years ago
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.
Attachment #341289 - Flags: approval1.9.0.4? → approval1.9.0.4-
Also minusing for wanted since it doesn't meet the criteria.
Flags: wanted1.9.0.x+ → wanted1.9.0.x-
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1?
Duplicate of this bug: 471379
You need to log in before you can comment on or make changes to this bug.