HTTP connection needs to hold a ref to security info

VERIFIED FIXED in mozilla0.9.2

Status

()

--
critical
VERIFIED FIXED
18 years ago
16 years ago

People

(Reporter: ccarlen, Assigned: ccarlen)

Tracking

Trunk
mozilla0.9.2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Assignee)

Description

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

Comment 1

18 years ago
Created attachment 37912 [details] [diff] [review]
patch to hold ref
(Assignee)

Comment 2

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

Updated

18 years ago
Blocks: 76168

Comment 3

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

Comment 4

18 years ago
Created attachment 37933 [details] [diff] [review]
update with darin's suggestion
(Assignee)

Comment 5

18 years ago
-> 0.9.2
Doug - can you review it?
Target Milestone: --- → mozilla0.9.2

Comment 6

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

18 years ago
it is correct given our implementation of the socket transport ;-)
(Assignee)

Comment 8

18 years ago
So Doug, given what Darin says, can you give r=?
Status: NEW → ASSIGNED

Comment 9

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

Updated

18 years ago
Blocks: 83989

Comment 10

18 years ago
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)

Comment 11

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

Comment 12

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

Updated

18 years ago
No longer blocks: 83989
(Assignee)

Comment 14

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

Comment 15

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

Comment 16

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

18 years ago
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. 
Whiteboard: Need sr, approval

Updated

18 years ago
Blocks: 82156

Comment 18

18 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

18 years ago
Alright - Thanks.
No longer blocks: 82156

Comment 20

18 years ago
Reopening. Please mark fixed after the patch is checked in.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

18 years ago
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.
Whiteboard: Need sr, approval

Comment 22

18 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 23

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

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

Comment 25

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

Comment 26

16 years ago
Verified. Lock icon behavior has been working correctly for several months now.
Status: RESOLVED → VERIFIED
QA Contact: benc → junruh
You need to log in before you can comment on or make changes to this bug.