localHostOrDomainIs() never matches a hostname

VERIFIED FIXED

Status

()

VERIFIED FIXED
15 years ago
14 years ago

People

(Reporter: benc, Assigned: Stefan.Borggraefe)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug] checkkwin, checklinux, URL)

Attachments

(1 attachment)

(Reporter)

Description

15 years ago
From the PAC spec:

The following statement is true (host name match, domain name not specified):

localHostOrDomainIs("www", "www.netscape.com") 

However, I find that we never evaluate this case as true.

I've traced the problem in the localHostOrDomainIs() function to:

"        return (hostdom.search('/^' + host + '/') != -1);\n" +

For some reason, .search works if you hand it a literal, but not if you put a
variable or evaluate something in the ()

I've even gone into venkman to confirm this:

stuff="/www/"
"wwwblahblahblah".search(stuff)" = -1
but
"wwwblahblahblah".search(/www/) = 0
(Reporter)

Comment 1

15 years ago
Neil was helpful in explaining what was wrong here, and has suggested logic that
is much more readable to me:

return host == hostdom || hostdom.lastIndexOf(host + '.', 0) == 0

This is very similar to what is currently used in dnsDomainIs().
(Reporter)

Updated

15 years ago
OS: MacOS X → All
Hardware: Macintosh → All

Updated

15 years ago
Assignee: darin → Stefan.Borggraefe
Keywords: helpwanted
Whiteboard: [good first bug]
(Assignee)

Comment 2

15 years ago
Adding URL of the relevant part of the spec.
Status: NEW → ASSIGNED
(Assignee)

Comment 3

15 years ago
Created attachment 156108 [details] [diff] [review]
Neil's idea in the form of a patch
(Assignee)

Comment 4

15 years ago
Comment on attachment 156108 [details] [diff] [review]
Neil's idea in the form of a patch

Darin, can you review this? Thanks! :-)
Attachment #156108 - Flags: review?(darin)

Comment 5

15 years ago
Comment on attachment 156108 [details] [diff] [review]
Neil's idea in the form of a patch

why lastIndexOf and not indexOf?

shouldn't localHostOrDomainIs("foo", "foo.foo.com") return true?
(Assignee)

Comment 6

15 years ago
(In reply to comment #5)
> (From update of attachment 156108 [details] [diff] [review])
> why lastIndexOf and not indexOf?

indexOf always searches the whole string while lastIndexOf starts at the
beginning of the string and stops when the first character that doesn't match is
seen.

http://lxr.mozilla.org/mozilla/source/js/src/jsstr.c#987
http://lxr.mozilla.org/mozilla/source/js/src/jsstr.c#1051

Besides of this I think indexOf would work, too.

> shouldn't localHostOrDomainIs("foo", "foo.foo.com") return true?

Mhh, it does. "foo.foo.com".lastIndexOf("foo.", 0) returns 0.

Comment 7

15 years ago
Comment on attachment 156108 [details] [diff] [review]
Neil's idea in the form of a patch

OK, that makes sense to me now.  I misunderstood the meaning of the second
parameter.  I see that lastIndexOf(..., 0) is only ever going to do a single
string compare.

r=darin
Attachment #156108 - Flags: review?(darin) → review+
(Assignee)

Updated

15 years ago
Attachment #156108 - Flags: superreview?(bzbarsky)
Comment on attachment 156108 [details] [diff] [review]
Neil's idea in the form of a patch

I'm not likely to get to this review in the next two weeks.
(Assignee)

Updated

15 years ago
Attachment #156108 - Flags: superreview?(bzbarsky) → superreview?(brendan)
(Assignee)

Updated

14 years ago
Attachment #156108 - Flags: superreview?(brendan) → superreview?(bzbarsky)
Comment on attachment 156108 [details] [diff] [review]
Neil's idea in the form of a patch

sr=bzbarsky
Attachment #156108 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 10

14 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Reporter)

Comment 11

14 years ago
V/fixed.
Using the test file:

file:///Users/benc/Public/mozilla-org/html/quality/networking/testing/PAC/byspecs/localHostOrDomainIs.pac

You can see in the js console that all the tests pass.
Status: RESOLVED → VERIFIED
Keywords: helpwanted
Whiteboard: [good first bug] → [good first bug] checkkwin, checklinux
(Reporter)

Comment 12

14 years ago
whoops, references to my local cvs tree are useless...

here's the file:

//Test standards from spec.
function FindProxyForURL(url, host)
{


	if (localHostOrDomainIs("www.netscape.com", "www.netscape.com"))
		alert ("example #1 - PASS");
	if (localHostOrDomainIs("www", "www.netscape.com"))
		alert ("example #2 - PASS");
	if (!localHostOrDomainIs("www.mcom.com", "www.netscape.com"))
    		alert ("example #3 - PASS");
	if (!localHostOrDomainIs("home.netscape.com", "www.netscape.com"))
    		alert ("example #4 - PASS");
alert("done!");
}
(Reporter)

Comment 13

14 years ago
Stefan: can you checkin for the 1.7 branch? (I'm out of date on what the rules
are..)
(Assignee)

Updated

14 years ago
Attachment #156108 - Flags: approval1.7.6?
Comment on attachment 156108 [details] [diff] [review]
Neil's idea in the form of a patch

Too late for 1.7.6.  Also note that a fix to core needs to go into both 1.7 and
aviary branches.  If this fix is still wanted in the future, please make sure
to request approval for both branches.
Attachment #156108 - Flags: approval1.7.6? → approval1.7.6-
(Reporter)

Comment 15

14 years ago
So wait until after 1.7.6 and get it into the branch?
You need to log in before you can comment on or make changes to this bug.