Closed Bug 93924 Opened 23 years ago Closed 23 years ago

PAC - dnsResolve should have one-element cache

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla0.9.4

People

(Reporter: eilya497, Assigned: darin.moz)

References

Details

(Keywords: perf, Whiteboard: r=bbaetz, sr=darin, a=roc+moz)

Attachments

(3 files)

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.
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).
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?
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.
QA Contact: tever → benc
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.
-> 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?
Component: Networking: HTTP → Networking
QA Contact: benc → pacqa
Summary: Mozilla 9.3 freezes temporarily during network operations → PAC - Mozilla 9.3 freezes temporarily during network operations
> 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.
Attached file My proxy configuration file. (deleted) —
I've attached my proxy.pac It indeed contains IsInNet.
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
Assignee: neeti → tingley
Status: UNCONFIRMED → NEW
Depends on: 97097
Ever confirmed: true
Summary: PAC - Mozilla 9.3 freezes temporarily during network operations → PAC - dnsResolve should have one-element cache
Attached patch patch (deleted) — Splinter Review
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...
Keywords: perf
Whiteboard: want for 0.9.4, r=bbaetz, sr=?
Attached patch trivially updated patch (deleted) — Splinter Review
Added a pointer in the comments. Got an ok from bbaetz on irc to stick with case-sensitive comparisons for now.
OS: Linux → All
Hardware: PC → All
r=bbaetz on the new version [irc]
sr=darin
Whiteboard: want for 0.9.4, r=bbaetz, sr=? → want for 0.9.4, r=bbaetz, sr=darin
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.
Status: NEW → ASSIGNED
Whiteboard: want for 0.9.4, r=bbaetz, sr=darin → r=bbaetz, sr=darin
Target Milestone: --- → mozilla0.9.5
Adding nsenterprise. The fix is ready to go: let's get it in.
Keywords: nsenterprise
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.
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.
Assignee: tingley → darin
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.5 → mozilla0.9.4
Whiteboard: r=bbaetz, sr=darin → r=bbaetz, sr=darin, a=roc+moz
fixed-on-trunk
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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?)
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).
The new bug is bug 97605.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: