Closed Bug 435558 Opened 13 years ago Closed 13 years ago

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


(Core :: Security: PSM, defect)

Not set





(Reporter: mayhemer, Assigned: mayhemer)




(1 file, 2 obsolete files)

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 '', from that moment all problems disappears. Just a note this also applies to FindProxyForURL autoconfig.

The dialog is shown from here

because socketInfo->GetExternalErrorReporting fails because of missing mCallbacks in socketInfo. See 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 ( 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.
Attached patch Fix, v1 (obsolete) — Splinter Review
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)
Duplicate of this bug: 439902
Duplicate of this bug: 442126
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.1?
Duplicate of this bug: 439028
Duplicate of this bug: 438008
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.
Attached patch Fix, v2 (obsolete) — Splinter Review
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+
Attachment #336642 - Flags: superreview?(bzbarsky)
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+
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)?
(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]

with additional
Attachment #341289 - Attachment description: Fix, v2.1 → Fix, v2.1 [Checkin: Comment 15]
Closed: 13 years ago
Flags: in-testsuite?
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
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 and not on ::1 even IPv6 support is enabled on the machine. localhost name is resolved to "::1" and "" 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-
Flags: wanted1.9.1?
You need to log in before you can comment on or make changes to this bug.