242325 - localHostOrDomainIs() never matches a hostname

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[benc]

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

Comment 1

[benc]

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().

Comment 2

[Stefan Borggraefe]

Adding URL of the relevant part of the spec.

Comment 3

[Stefan Borggraefe]

Attachment: Neil's idea in the form of a patch

Comment 4

[Stefan Borggraefe]

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

Darin, can you review this? Thanks! :-)

Comment 5

[Darin Fisher]

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?

Comment 6

[Stefan Borggraefe]

(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

[Darin Fisher]

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

Comment 8

[Boris Zbarsky [: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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

sr=bzbarsky

Comment 10

[Stefan Borggraefe]

Fix checked in.

Comment 11

[benc]

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.

Comment 12

[benc]

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!");
}

Comment 13

[benc]

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

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

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.

Comment 15

[benc]

So wait until after 1.7.6 and get it into the branch?