93924 - PAC - dnsResolve should have one-element cache

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Ilya Tsindlekht]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.3) Gecko/20010801
BuildID:    2001080104

It seems that in 9.3 the lack of response from WEB server sometimes causes whole
UI to freeze until server responds. I didn't notice such
thing with 9.1 and other previous versions.

Reproducible: Sometimes
Steps to Reproduce:
Observed occasionnally when using mosilla. Seems to happen at some stage
during start of HTTP transaction when server doesn't respond.
After some time, browser comes alive (obviously after receiving
response from server (or, maybe, timeout)).

Actual Results:  Mozilla freezes (all UI: resizing, animations, all)
CPU is at zero, so it must be a deadlock.


Expected Results:  UI is expected to remain responsive

I did not use HTTP proxy and had HTTP pipelining disabled.

Comment 1

[Ilya Tsindlekht]

I think, at least some of hangups are during DNS lookup. When chosing an URL
from the URL bar history list, and DNS server failing to resolve URL, the whole X
may become frozen (when chosing bookmark, only Mozilla freezes).

Comment 2

[Ilya Tsindlekht]

It could be that I have mislead you and I had authomatic proxy configuration
enabled. Could it be that the hangup occurs when authomatic proxy configuration
code produces DNS query to check host IP?

Comment 3

[benc]

qa to me.

It could. you could elimiante that possiblility by creating a test PAC file that
uses and IP address.

My guess is that this will turn out to be something else.

Comment 4

[Ilya Tsindlekht]

My proxy.pac file checks for some IPs. It causes DNS query probably to convert
the domain name into IP. Until the query is answered, the browser freezes. It
seems to be quite reproducible.

Comment 5

[benc]

-> pac (comp=networking, qa=pacqa, +cc tingley.

Okay, lets get a file and start analysis.
Can you tell us if the PAC file is working better in other browsers you use?

Comment 6

[Chase Tingley]

> My proxy.pac file checks for some IPs. It causes DNS query probably to convert
> the domain name into IP. Until the query is answered, the browser freezes. It
> seems to be quite reproducible.

This is probably the problem -- isInNet() and dnsResolve() didn't work before
0.9.3 (bug 79057).  These are calling synchronously into nsDnsService::Resolve.
 If this is freezing up mozilla, I'd have to guess it's because PAC execution
was taking place in some UI thread instead of a networking one.  Post your PAC
anyways so we can take a look.

cc'ing bbaetz, who unlike me knows what he's talking about.

Comment 7

[Ilya Tsindlekht]

Attachment: My proxy configuration file.

Comment 8

[Ilya Tsindlekht]

I've attached my proxy.pac It indeed contains IsInNet.

Comment 9

[Bradley Baetz (:bbaetz)]

So it turns out that the dns resolving method we use in isInNet doesn't use the
dns cache, and so the hostname was being synchronously looked up for each
isInNet call. I filed bug 97097 on that.

However, the mozilla classic code had a one-element cache for the domain name
(http://lxr.mozilla.org/classic/source/network/main/mkautocf.c#1376) Our
dnsResolve function should do the same, for added performance and correctness.

CONFIRMING, and ->tingley, cc serge

Comment 10

[Chase Tingley]

Attachment: patch

Comment 11

[Bradley Baetz (:bbaetz)]

r=bbaetz if you mention bug 97097 in the comment, and see if you can do a case
insensitive string comparison for the domain name (thats an edge case though,
since this will be usually called on exactly the same host, so don't worry about
it. mozClassic used strcmp, anyway)

Lets try for 0.9.4...

Comment 12

[Chase Tingley]

Attachment: trivially updated patch

Comment 13

[Chase Tingley]

Added a pointer in the comments.  Got an ok from bbaetz on irc to stick with
case-sensitive comparisons for now. 

Comment 14

[Chase Tingley]

r=bbaetz on the new version [irc]

Comment 15

[Darin Fisher]

sr=darin

Comment 16

[Chase Tingley]

rejected by drivers for 0.9.4, but I guess this is no huge surprise.

-> 0.9.5

I'll need to track down someone to check this in for me when the trunk opens up
again.

Comment 17

[Steve Meredith (gone)]

Adding nsenterprise. The fix is ready to go: let's get it in.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a=roc+moz on behalf of drivers

BTW, I don't quite know what the scope of your 1-element DNS cache is, but I
hope you've considered the implications of having a DNS cache that does not
respect the TTLs reported by DNS. In some environments caching DNS entries
longer than the TTL is quite bad.

Comment 19

[Bradley Baetz (:bbaetz)]

Since my computer is packed up, I've asked darin to do this. -> him

Its a one element cache, and identical to 4.x, and used only for PAC.

Comment 20

[Darin Fisher]

fixed-on-trunk

Comment 21

[Ilya Tsindlekht]

The cache may help in some cases, but what if the address is not in cache?
Isn't the real problem that PAC should not be run by the UI thread (as it 
contains potential for blocking?)

Comment 22

[Chase Tingley]

As I understand it, this is correct -- the patch put a band-aid onto a larger
problem.  I'm going to spin that issue off into a new bug (I'll post the # back
here).

Comment 23

[Chase Tingley]

The new bug is bug 97605.

Comment 24

[benc]

V/fixed:

This behavior was eventually removed when Darin did a DNS re-write (about 1.6b),
and the PAC implementation caught up.

We do have other DNS slowness problems, and I confess that I do not fully
understand them at this time.