97605 - PAC: PAC execution should not block the UI thread

Status: VERIFIED INVALID

(Core :: Networking, defect)

Description

[Chase Tingley]

Spun off from bug 93924.

PAC execution occurs synchronously in the UI thread, so that appropriate
channels may be opened based on the results.  Unfortunately, PACs sometimes make
synchronous calls to nsDNSService::Resolve -- any PAC containing dnsResolve(),
isInNet(), or isResolvable() may make these calls.  

Synchronous calls to nsDNSService::Resolve() will ignore the DNS cache (bug
97097), but adding a one-element cache directly in PAC (bug 93924) will improve
this somewhat.  The real issue is in the case where the name is not resolvable,
in which case the DNS service may take a while to time-out.  Until it does, the
UI thread is blocked, effectively hanging the browser.

I've discussed the problem with bbaetz a bit, and there doesn't seem to be an
easy solution.

Comment 1

[Chase Tingley]

+arch

Comment 2

[Bradley Baetz (:bbaetz)]

Its not that this is hard, its that it cannot be fixed, I think.

a) necko isn't threadsafe, so PAC will always be called on the UI thread
b) We need PAC to be synchronous, because we a caller which calls 
NewChannelFromURI needs to be able to QI it to an http chanel immediately if we 
are using an http proxy.

Thus, PAC must be run synchronously on the UI channel.

(OK, we could use a wrapper class which then forwards to the real channel once 
its known, after calling pac asynchronously, when a method is called, _or_ when 
a QI is made. I don't see the benefits being worth the complexity, however. Even 
then it probably wouldn't help, because calling AsyncOpen after NewChannel isn't 
that uncommon. We can't proxy the AsyncOpen as well, because it needs to return 
an error)

darin?

Comment 3

[Darin Fisher]

actually, perhaps a much simpler solution would be to push an event queue, which
would still allow UI events, but the PAC download would then become an app modal
operation.

dougt: does this sound possible?

Comment 4

[Doug Turner (:dougt)]

It would be easy enough to try.

Comment 5

[Chase Tingley]

A similar problem will probably be caused by a fix for bug 80918, unless "No
proxy for" on IP addresses continues to not resolve the hostname before doing a
comparison.

Comment 6

[Bradley Baetz (:bbaetz)]

This bug is unfortunately invalid. PAC needs to block the ui thread, because
consumers creating a channel need to be able to QI it at once to an
nsIHttpChannel if appropriate. Creating a channel is not an async operation; we
can't run this off a separate event.

Once bug 97097 is fixed (currently targetted for 0.9.6), then we shouldn't have
any problems with speed, I think.

darin, does that sound right?

Comment 7

[Tom Everingham]

-> future

Comment 8

[benc]

bradley: should this be RESOLVED/INVALID then?

Comment 9

[Bradley Baetz (:bbaetz)]

Unfortunately, yes

Comment 10

[benc]

VERIFIED
(byebye)

Is this any worse than having a manual config that points to a proxy hostname
that is unresolvable?