85211 - HTTP connection needs to hold a ref to security info

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Conrad Carlen (not reading bugmail)]

This was discovered while investigating bug 76168. When going to the given URL,
you get no warning about entering a secure site. The problem is the call to
nsIChannel::GetSecurityInfo returns NULL because its underlying transport has
gone away. The fix is for the connection to keep a ref to the security info
which it got from the transport.

Comment 1

[Conrad Carlen (not reading bugmail)]

Attachment: patch to hold ref

Comment 2

[Conrad Carlen (not reading bugmail)]

The patch fixes it. Darin - could you r= it (or sr= if you can)?

Comment 3

[Darin Fisher]

 LOG(("nsHttpConnection::OnStartRequest [this=%x]\n", this));
+
+    // Keep a ref to the transport's security info. We can get a call to
+    // GetSecurityInfo after the transport is gone.
-    if (mSocketTransport)
+    if (mSocketTransport && ctxt) // only need to do this once
+        mSocketTransport->GetSecurityInfo(getter_AddRefs(mSecurityInfo));
+

and, then sr=darin

Comment 4

[Conrad Carlen (not reading bugmail)]

Attachment: update with darin's suggestion

Comment 5

[Conrad Carlen (not reading bugmail)]

-> 0.9.2
Doug - can you review it?

Comment 6

[Doug Turner (:dougt)]

what if the security information object changes?  This patch makes the 
assumption that there only can be ONE security info object associated with the 
http transport viewable from the the channel clients.  Is this correct?  Is this 
what we want to do?

Comment 7

[Darin Fisher]

it is correct given our implementation of the socket transport ;-)

Comment 8

[Conrad Carlen (not reading bugmail)]

So Doug, given what Darin says, can you give r=?

Comment 9

[Doug Turner (:dougt)]

he winds up, and there's the pitch: r/sr

Comment 10

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 11

[David P. Drinan]

I've applied this patch to my tree and I'm still losing security info on some 
ssl connections. The code that's having the problem is :

nsresult
nsHttpTransaction::GetSecurityInfo(nsISupports **result)
{
    if (mConnection)
        mConnection->GetSecurityInfo(result);
    return NS_OK;
}

On some secure sites (e.g. https://login.yahoo.com), if I set a break point in 
this method, each time it gets called the mConnection is 0. So I cannot get 
security info and the page loads as non-secure i.e. the lock icon is unlocked. 

Should the nsHttpTransaction object to keep a reference to security info also?

Comment 12

[Conrad Carlen (not reading bugmail)]

This patch was made invalid by some changes since it was tested. New patch
coming up soon.

> Should the nsHttpTransaction object to keep a reference to security info also?

Yes, that's what it does.

Comment 13

[Conrad Carlen (not reading bugmail)]

Attachment: 2nd patch

Comment 14

[Conrad Carlen (not reading bugmail)]

Here's the new patch. Can people r=/sr= ?

Comment 15

[David P. Drinan]

I've tested this in my tree and it works. I'm able to get accurate security info 
on every secure site I tried.

This patch will greatly improve reliability on lock icon setting. I also need it 
for a number of other lock icon bugs I'm working on. Getting this patch landed 
asap would be very helpful.

For what it's worth, r=ddrinan.

Comment 16

[Conrad Carlen (not reading bugmail)]

Then can doug or darin sr=?

> for a number of other lock icon bugs I'm working on

:-) It was one of those I was working on, bug 76168, which led me to this. I
guess there's plenty of those bugs to go 'round but we should talk.

Comment 17

[David P. Drinan]

Conrad,

If this patch does not fix your lock icon bug, it might be best to reassign that 
bug me. I'm working on a patch to 
security/manager/ssl/src/nsSecureBrowserUIImpl.cpp to fix a number of problems 
with it's state machine. 

Comment 18

[Darin Fisher]

conrad: i was forced to implement this today b/c my changes for bug 84019 led
to a crash/blocker-for-today (see bug 85822) and in the process of fixing that
crash, i hit a crash trying to access mSecurityInfo from the connection.  so,
in short my patch for bug 85822 had to include this patch as well.

Comment 19

[Conrad Carlen (not reading bugmail)]

Alright - Thanks.

Comment 20

[John Unruh]

Reopening. Please mark fixed after the patch is checked in.

Comment 21

[Conrad Carlen (not reading bugmail)]

John, what causes you to reopen this? The code darin checked in does what this
patch does. I verifed this in the debugger as well as observing the behavior
when going to a secure site.

Comment 22

[John Unruh]

OK. Marking fixed since there does seem to be improvement in the lock icon 
behavior, but leaving bug 82156 open. From this site 
https://junruh.mcom.com/tests.html start at the top left and start going down 
and you will soon see that the lock icon behavior is still unpredictable.

Comment 23

[Conrad Carlen (not reading bugmail)]

OK. BTW, this bug will not fix all psm security notifications. This bug was
preventing nsSecureBrowserUI from being able to work. There are other bugs to be
solved there and fixing this allows that.

Comment 24

[benc]

junruh:
can you give me more info on how to test the improvement?

Comment 25

[David P. Drinan]

*** Bug 82736 has been marked as a duplicate of this bug. ***

Comment 26

[John Unruh]

Verified. Lock icon behavior has been working correctly for several months now.