396745 broke nsLDAPChannel.cpp

RESOLVED FIXED

Status

Directory
LDAP XPCOM SDK
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Howard Chu, Assigned: Howard Chu)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.29 KB, patch
standard8
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
The patch for bug #396745 missed an invocation of nsILDAPConnection:Init...
A fix is attached here.
(Assignee)

Comment 1

11 years ago
Created attachment 284105 [details] [diff] [review]
Fix for nsLDAPChannel.cpp
Comment on attachment 284105 [details] [diff] [review]
Fix for nsLDAPChannel.cpp

Guess I missed this as its in the experimental code.

With this change, you need to make some additional changes:

We no longer need to get options out of the mLDAPURL so that code/variable can be dropped.

 534     if (port == -1)
 535         port = LDAP_PORT;

Here you need to make an additional change to set LDAP_PORT in the mURI for the -1 case. Otherwise this won't be picked up by mLDAPURL.

Additionally the

rv = NS_CheckPortSafety(port, "ldap");

call can just be changed to 

rv = NS_CheckPortSafety(mURI);

Although its not really required for this patch, I think it will make the code a bit cleaner here.
Attachment #284105 - Flags: review-
(In reply to comment #2)
> (From update of attachment 284105 [details] [diff] [review])
>  534     if (port == -1)
>  535         port = LDAP_PORT;
> Here you need to make an additional change to set LDAP_PORT in the mURI for the
> -1 case. Otherwise this won't be picked up by mLDAPURL.

Actually, nsLDAPConnection already does the port == -1 check:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/directory/xpcom/base/src/nsLDAPConnection.cpp&rev=1.65&mark=940-941#940

So drop that from nsLDAPChannel.cpp, and change the NS_CheckPortSafety to the mURI option like I suggested before (which also handles the -1 case), and then we don't need to get the port or the options in nsLDAPChannel.cpp.
Keywords: regression
(Assignee)

Comment 4

11 years ago
Created attachment 284124 [details] [diff] [review]
cleaned up patch

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 284105 [details] [diff] [review] [details])
> >  534     if (port == -1)
> >  535         port = LDAP_PORT;
> > Here you need to make an additional change to set LDAP_PORT in the mURI for the
> > -1 case. Otherwise this won't be picked up by mLDAPURL.
> 
> Actually, nsLDAPConnection already does the port == -1 check:

Yeah, just noticed that. OK, cleaned up. Also moved the empty host check up, no point in doing the other setup if the URL is invalid.
Attachment #284105 - Attachment is obsolete: true
Attachment #284124 - Flags: review?
Assignee: dmose → hyc
Comment on attachment 284124 [details] [diff] [review]
cleaned up patch

That's better, r=me. I'm requesting sr for you, not sure if its really necessary as its NPOTB code, but we may as well do it.
Attachment #284124 - Flags: superreview?(bienvenu)
Attachment #284124 - Flags: review?
Attachment #284124 - Flags: review+
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED

Updated

11 years ago
Attachment #284124 - Flags: superreview?(bienvenu) → superreview+
Howard are you able to check this in, or do you want me to?
(Assignee)

Comment 7

11 years ago
(In reply to comment #6)
> Howard are you able to check this in, or do you want me to?
> 
Please go ahead, I don't have commit access. Thanks.
I just checked this in. Marking as fixed. Thanks for the patch Howard.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.