79057 - PAC: dnsResolve() missing, breaks isinNet() and isResolvable()

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[[e-mail]]

The current "sandbox" used by PAC doesn't seem to provide any mechanism
for getting at XP components like the DNS resolver.
Unfortunately this has the side effect that many common PAC fragments will not
work. Worse, Mozilla is currently incapable of reporting the nature of this
problem except via an Exception sent to stderr

Examples of code fragments which fail for our local PAC

isInNet(host, "152.78.0.0", "255.255.0.0")
fails because to discover whether www.mozilla.org is in 152.78.0.0/16 it
needs to resolve the address

isResolvable(host)
fails directly for obvious reasons

The exceptions look like:

[Exception... "[JavaScript Error: "dnsResolve is not defined" {file:
"http://www.ecs.soton.ac.uk/proxy.pac" line: 19}]
[nsIProxyAutoConfig::ProxyForURL]" nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "<unknown>" data: yes]

Whoever gets this on their plate should check out the mother of all PAC bugs,
bug #53080 to see how we got here.

Comment 1

[David Alger]

*** This bug has been confirmed by popular vote. ***

Comment 2

[benc]

qa to me
marking dependent on 58030.
changed summary to make it consistent w/ related bugs.

I'll be confirming this doesn't work if I can verify basic PAC works in 58030.

Comment 3

[benc]

(corrected depends to 53080)

Comment 4

[benc]

qa to me.

Comment 5

[Cesc Noguera]

Is it just me or not being able to use my pac configuration causes 
mozilla to crash before loading completely if its of any use my local pac file 
is http://www-cache.soton.ac.uk/proxy.pac 
after changing to manual proxy settings I imported the profile to moz and it 
worked fine then.

Comment 6

[Cesc Noguera]

Sorry should have made clearer comments :-o

Build: 20010515
OS: Windows 98
I use a proxy server. If I import a configuration form netscape 4.x with the
proxy to be setup using a pac file http://www-cache.soton.ac.uk/proxy.pac a
crash occurs suring loading whilst the splash screen is up. Not possible to
start mozilla. 
If I change the netscape 4.x settings to use a manual proxy configuration and
then import that profile aka a profile with manual configuration for the proxies
mozilla loads up fine.

I assume that trying to use an automatic proxy configuration causes mozilla to
crash. I automatic proxy configuration is not installed yet, but before it
didn't crash on a pac config.

Comment 7

[neeti]

-->PAC bugs to Jussi

Comment 8

[benc]

Cesc, please file a new bug, unless this has something to do with the
dnsResolve() function.

Comment 9

[David Alger]

Attachment: Proxy.Pac File - Broken by dnsResolve

Comment 10

[[e-mail]]

It is quite possible that Cesc's problem is this bug, he is at the same site as me (submitter) although he didn't realise that.
At our site Mozilla PAC fails as shown in my original submission (see topic of bug)
If the problem (imported NS4x PAC prefs break Mozilla) happens for people at sites which do not need dnsResolve() THEN you have a new bug IMHO.

Comment 11

[Mattias Holmlund]

This is still happening in build 2001061204 on WinNT. It makes
proxy-autoconfiguration useless for me and forces me to configure the proxies
manually instead. Is there a workaround or is noone using proxy autoconfiguration?

Comment 12

[benc]

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

Comment 13

[benc]

Migration sending you to deafness is bug 54804, but realistically it should
either be a known bug, or a new bug.

Comment 14

[Chase Tingley]

Tracking.  This is a nasty one.  (Breaks my employer's PAC as well!)

Comment 15

[Chase Tingley]

Upgrading severity to major; it should be at least this high because unlike
other broken PAC functions like isPlainHostName() that merely behave
incorrectly, this will actually stop *all* PAC functionality by causing an
exception to be thrown by the JS sandbox.

Comment 16

[[e-mail]]

[e-mail], you wrote this stops all PAC functionality.
Can you clarify your build ID and symptoms please?

Older builds used to just spit exceptions to console as I originally reported,
which meant that the browser was effectively disabled.
Modern builds seem to log a JS error in the JS console and treat the result as
"DIRECT". This is IMHO much less serious, although obviously many users will be
unable to use the browser in this state.

Since the engineer assigned hasn't written a patch or suggested a line of attack
yet I'm going to suggest a workaround that would reduce severity to normal or
minor IMHO, I do not know if it is feasible.

1. Before passing URL into the PAC sandbox copy the Host portion
2. Look up the host portion in DNS for an IP e.g. 172.16.17.18
3. Pass the Host and IP (or null if lookup failed) into the sandbox
as additional parameters/ properties of some built-in.

4. Implement dnsResolve() which ONLY works for the Host portion of the URL
by checking for an exact string match, and then returning the IP

As far as I know 99.9% of dnsResolve() executions are looking up the host of the
URL being contemplated by PAC. The remaining 0.1% can return failure, and I bet
that pretty much every PAC user will get satisfactory results.

How about it - is this an acceptable work-around?

Comment 17

[Chase Tingley]

[e-mail], I think you're right -- it will default to DIRECT.  (I'm
running nightlies/cvs on  Linux.)  So it doesn't stop connectivity, but it
prevents the user from using a proxy.

Comment 18

[Chase Tingley]

I've been messing around with a workaround along the lines of what
[e-mail] proposed.  This appears to work, at least for the simple test
PAC I've been playing with (using isInNet()).

This approach requires wrapping the call to the PAC FindProxyForURL call in
order to pass in the host IP address.

Comment 19

[Chase Tingley]

Attachment: patch -- something along these lines

Comment 20

[Jussi-Pekka Mantere]

Serge, can you take a look at these PAC issues? Thanks - Jussi-Pekka

Comment 21

[benc]

If understand this workaround, you want to DNS lookup for for every URL sent to
PAC? That could send out a large number of useless DNS requests (not all PAC
files would call DNS, and some proxy admins are deliberatly delegating DNS
downstream to the server...), so I don't think it is a long term solution.

However, PAC is not on track to be usable for RTM or Mozilla 1.0, so if this
will fix a large number of submitted PAC files (I see three in the block list),
then lets try it.

Comment 22

[Chase Tingley]

Attachment: slightly improved try #2

Comment 23

[Chase Tingley]

Ben, you're right, this could generate a lot of unneeded lookups if the PAC
doesn't use any dns-related functions.  Although if a PAC file included
isResolvable or isInNet, we're not doing many more queries than "real" DNS
support would.  (And if the PAC included more than one DNS call, we may be doing
fewer, due to the natural "caching" of this dnsResolve() implementation, heh heh.)

Anyways, a couple improvements to handle hosts that don't resolve a little better.

Comment 24

[benc]

But nobody was using those calls, at least noboody with a brain was. Just try
it, create a PAC that uses either function call once, on every URL. That means
you have to time out DNS to go anywhere. The performance is horrible.

Ari, one of the Proxy architects, said once he wishes they had never told anyone
that was a feature of PAC, be cause it had almost no real-world application.
When I did support for PAC, we basically told people to never use it.

Comment 25

[[e-mail]]

Bens comments apparently assumes that hosts using PAC do not have working DNS. I
have no idea where this assumption came from, but I am glad that I never had to
deal with his "support" of the PAC feature.

FWIW the 10,000+ hosts affected which caused me to raise this bug do have
working DNS, and are using three or four IsInNet() lines routinely from mostly
WinIE and some NS4x hosts over a large distributed /16 network. The performance
is fine, no better or worse than it was before proxies were mandatory.

Hosts without DNS _should_ be configured to fail host lookup cleanly. Do we have
good evidence that this is not the case, and that they will just hang until the
DNS lookup times out? Sites with partial DNS should definitely fail cleanly,
we've used a set-up like that happily on intranet dial-up.

Comment 26

[benc]

hmph! I actually got paid to do that support, and it wasn't my problem, it was
the hostmaster's problem, if you must know... :)

IsInNet is less of an issue, because there are more situations where people use
it without hitting DNS failures. However, my point is that there are lots of
people that do have split DNS horizons, and this hack will not serve them well
in the long run. DNS timeouts take a LONG time, and are generally hard-coded
into the OS. We are talking about seconds per URL, so the home page of
www.news.com, could take almost two minutes to load (for example)...
 
If you want, I can whip up a really good PAC example that you can run under
Communicator 4...

Comment 27

[[e-mail]]

OK, so the case Ben is worried about is the case where some bozo has a "working"
setup in which DNS is mis-configured and times out for (all/most) sites instead
of returning NXDOMAIN but he uses PAC to work-around it with optionally a few
string-match tests to avoid going to the proxy for Intranet stuff.

This is probably much much more common than I want to think it is. Yes, my
suggested work-around will suck very badly for this case. Probably better that
Ben warned me about it before I got buried in hate mail from bozos who can't or
won't configure a working DNS server :)

Three options:
Clean option - Find a Sandbox / JS Components hacker to think hard doing this
correctly for 1.0

Dirty option - Regexp hacks to check for DNS-related code and enable work-around

Less Dirty option - Check for Exception and re-run PAC sandbox with work-around
if the DNS Component exception occurs.

Comment 28

[benc]

ruth:

I still like your proposal. I still think we should do it, as long as we agree
its not the final fix.

I would rather have PAC slow than no PAC at all. Asa says to make no advocacy
comments in bugzilla, but this is an example of what I like to say is "Metcalf's
law is  greater than Moore's Law". It is better to have a product that extends
the number of people that can be connected to the network that disenfranchise
people so a fancier, computational feature can be added. 

PAC is long over-due, and needed to drive the next wave of adoption of
mozilla-based users.

Besides, neither of us works in support anymore. Someone else can argue with the
network admin. I am also behind on my DNS technology (NXDOMAIN was not a widely
used record type when I had these PAC issues). If the local DNS server is
responsive, the performance hit might be minimal as long as it caches NXDOMAIN.

I gotta go order a newer DNS and BIND book now...

Comment 29

[basic]

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

Comment 30

[Chase Tingley]

I just posted a patch to bug 84798 which will also fix (*not* the hack) this one
as well, so I'm marking a dependency.

Comment 31

[Chase Tingley]

Attachment: fix, split off from bug 84798

Comment 32

[Chase Tingley]

I've split this fix off from bug 84798, since that patch won't be happening for
a while.

That bug had "r=bbaetz for the 79057 stuff"; hopefully that still applies here.

Comment 33

[Bradley Baetz (:bbaetz)]

r=bbaetz

You could change the if (test==null) to if (!test) as well

Comment 34

[Chase Tingley]

Attachment: Previous patch had a typo; please use this one!

Comment 35

[Bradley Baetz (:bbaetz)]

r=bbaetz with the variable name change...

Comment 36

[Chase Tingley]

I had made an error splitting this patch off and was assigning the resolved
value to 'test' rather than 'ipaddr' -- bad news since convert_addr(ipaddr) is
called later on.  so attachment 43562 [details] [diff] [review] is the correct one.

Comment 37

[Chase Tingley]

Attachment: new version following checkin of bug 80363

Comment 38

[Chase Tingley]

No functional changes, but the diff wouldn't apply cleanly due to bbaetz's checkin.

Comment 39

[Darin Fisher]

sr=darin

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

a=dbaron (on behalf of drivers) for 0.9.3 checkin

Comment 41

[serge (gone)]

[e-mail] thanks a lot.
It has been checked in.
mozilla/netwerk/base/src/nsProxyAutoConfig.js,v  <-- nsProxyAutoConfig.js
new revision: 1.13; previous revision: 1.12
done

Comment 42

[benc]

RELNOTE (NS 6.1)

isinNet(), isResolvable() and dnsResolve() are not supported in this release.

Comment 43

[now [e-mail]]

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

Comment 44

[basic]

[e-mail]: could you verify with a newer version of mozilla like
mozilla0.9.3 ?

Comment 45

[[e-mail]]

Sorry to take so long, this is working fine at our site. Let's see if I can mark
verified...