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

VERIFIED FIXED in mozilla0.9.3

Status

()

Core
Networking
--
major
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: ruth@innocent.com, Assigned: serge (gone))

Tracking

({relnote})

Trunk
mozilla0.9.3
relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [approved for 0.9.3]r=bbaetz sr=darin)

Attachments

(6 attachments)

(Reporter)

Description

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

17 years ago
*** This bug has been confirmed by popular vote. ***
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

17 years ago
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.
Depends on: 58030
OS: Linux → All
Hardware: PC → All
Summary: PAC: dnsResolve() needed for some Proxy Auto Config features → PAC: dnsResolve()

Comment 3

17 years ago
(corrected depends to 53080)
Depends on: 53080
No longer depends on: 58030

Updated

17 years ago
Target Milestone: --- → mozilla0.9.3

Updated

17 years ago
No longer depends on: 53080

Comment 4

17 years ago
qa to me.
QA Contact: tever → benc

Comment 5

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

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

17 years ago
-->PAC bugs to Jussi
Assignee: neeti → jpm

Comment 8

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

Comment 9

17 years ago
Created attachment 34902 [details]
Proxy.Pac File - Broken by dnsResolve
(Reporter)

Comment 10

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

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

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

Comment 13

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

Summary: PAC: dnsResolve() → PAC: dnsResolve() missing, breaks isinNet() and isResolvable()

Comment 14

17 years ago
Tracking.  This is a nasty one.  (Breaks my employer's PAC as well!)
Blocks: 79893

Comment 15

17 years ago
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.
Blocks: 85656, 87272
Severity: normal → major
(Reporter)

Comment 16

17 years ago
tingley@sundell.net, 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

17 years ago
ruth@innocent.com, 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

17 years ago
I've been messing around with a workaround along the lines of what
ruth@innocent.com 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

17 years ago
Created attachment 40092 [details] [diff] [review]
patch -- something along these lines

Updated

17 years ago
QA Contact: benc → pacqa

Comment 20

17 years ago
Serge, can you take a look at these PAC issues? Thanks - Jussi-Pekka
Assignee: jpm → serge

Comment 21

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

17 years ago
Created attachment 40379 [details] [diff] [review]
slightly improved try #2

Comment 23

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

17 years ago
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.
(Reporter)

Comment 25

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

17 years ago
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...
(Reporter)

Comment 27

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

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

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

Comment 30

17 years ago
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.
Depends on: 84798

Comment 31

17 years ago
Created attachment 43560 [details] [diff] [review]
fix, split off from bug 84798

Comment 32

17 years ago
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.
No longer depends on: 84798
Keywords: patch, review
Whiteboard: looking for sr=
r=bbaetz

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

Comment 34

17 years ago
Created attachment 43562 [details] [diff] [review]
Previous patch had a typo; please use this one!
r=bbaetz with the variable name change...

Comment 36

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

17 years ago
Created attachment 43836 [details] [diff] [review]
new version following checkin of bug 80363

Comment 38

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

Comment 39

17 years ago
sr=darin

Updated

17 years ago
Whiteboard: looking for sr= → r=bbaetz sr=darin
a=dbaron (on behalf of drivers) for 0.9.3 checkin
Whiteboard: r=bbaetz sr=darin → [approved for 0.9.3]r=bbaetz sr=darin
(Assignee)

Comment 41

17 years ago
tingley@sundell.net 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
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 42

17 years ago
RELNOTE (NS 6.1)

isinNet(), isResolvable() and dnsResolve() are not supported in this release.
Keywords: relnote
*** Bug 95327 has been marked as a duplicate of this bug. ***

Comment 44

17 years ago
ruth@innocent.com: could you verify with a newer version of mozilla like
mozilla0.9.3 ?
(Reporter)

Comment 45

16 years ago
Sorry to take so long, this is working fine at our site. Let's see if I can mark
verified...
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.