84798 - PAC: Failover (multiple return values for FindProxyforURL)

Status: VERIFIED FIXED

(Core :: Networking, enhancement, P2)

Description

[benc]

Using:

http://www.packetgram.com/pktg/proxy/pac/pacfiles/failover.pac.

Console shows that PAC resolves to two servers, one available, one not, but 
then it doesn't work.

Comment 1

[benc]

Attachment: failvoer pac

Comment 2

[benc]

The first returned proxy server is not available, so presumably it should 
failover.

I need to check the javascript syntax, because it looks weird, but someone told 
me that was correct.

Comment 3

[Bradley Baetz (:bbaetz)]

The proxy APIs only allow for one server, and so the pac stuff just picks the
first, as I understand it.

Comment 4

[neeti]

-->jpm

Comment 5

[benc]

Failover is specified in the PAC definition, and worked in Comm4.
+ lots of keywords.

Comment 6

[benc]

There also is no UI for complete failure, which should ask if you go to direct
connection:
"
If all proxies are down, and there was no DIRECT option specified, the Navigator
will ask if proxies should be temporarily ignored, and direct connections
attempted. The Navigator will
ask if proxies should be retried after 20 minutes has passed (then the next time
40 minutes from the previous question, always adding 20 minutes).
"
http://home.netscape.com/eng/mozilla/2.0/relnotes/demo/proxy-live.html 

Comment 7

[benc]

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

Comment 8

[Chase Tingley]

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

Comment 9

[Travis Freeland]

The failover return which works in other browsers but not in mozilla/netscape
6.1pr1:

        return "PROXY full.its.deakin.edu.au:3128; PROXY
mini.its.deakin.edu.au:3128";

Comment 10

[Jussi-Pekka Mantere]

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

Comment 11

[Chase Tingley]

Attachment: patch

Comment 12

[Chase Tingley]

The patch was co-authored with [e-mail].

Parse the LocalFindProxyForURL output and check dns resolvability of each in
turn.  If nothing satisfiable is found, treat as a direct connection.

Comment 13

[Chase Tingley]

Umm, hold the phone on that one.  It appears I may be able to make a very minor
change and get the fix for bug 79057 much easier than I had expected to.

Comment 14

[Chase Tingley]

Attachment: patch v2, with fix for 79057

Comment 15

[Chase Tingley]

Take 2.  A week ago I could have sworn getting a wrapper around dns.resolve() to
work inside the PAC sandbox was very tough, but apparently I was wrong.  A one
line addition allows a fix for bug 79057 as well.

While I was at it I tweaked isInNet so it doesn't behave badly if dnsResolve
can't get an ip address for the hostname.

cc'ing gagan because he seems to have created the pac code, and mstoltz because
we were talking about security issues.
As usual, co-authored by basic.

Comment 16

[Jesus Cea]

What about if the DNS resolves fine, but the proxy elected by PAC is down?

Comment 17

[Chase Tingley]

You're right -- this is only a partial solution.  Getting it to fall back
properly when it can't connect to its first-choice proxy would require chasing
this all over the netwerk code, and the scope of it seems beyond my present
knowledge.  As bbaetz said: "The proxy APIs only allow for one server, and so
the pac stuff just picks the first, as I understand it."  This is still the
case, but now the PAC stuff picks the first that is not obviously bogus.

Comment 18

[basic]

Changing Platform/OS

the patch tingley attach is more of a workaround than a fix, but it is much
better than the current behavior. A real fix would involve rewriting the proxy
api, which I doubt will happen before bug 89928 is fixed.

Comment 19

[Chase Tingley]

Still hoping for an r/sr for this.  The odds of making it before the 0.9.3
freeze are zero and dropping, but since I'll be away from July 30 - August 10,
if possible I'd like to get some sort of feedback on this this week.  In truth,
I'm more eager to get bug 79057 fixed than this one (as it's more broken, is a
more complete fix, and has more votes to boot); I'd be happy to split this patch
in two if it will speed things up any.

Comment 20

[Bradley Baetz (:bbaetz)]

You realise that this isn't actually fallover - full fallover means that you
need to try the second proxy if the connection is refused, not just if the host
is not existing. (And, arguably, for some proxy errors, I guess) That would
require api changes, though.

A dnsresolve call on each call is expensive, but we cache it, so it really isn't
that important. I just don't think that it solves the problem.

r=bbaetz on the 79057 part. Is FindProxyForURL reentrant? if so, won't the
dnsresolve call serialise everything? If not, r=bbaetz on that part too.

Comment 21

[Chase Tingley]

As I've said before, this is completely right -- I didn't fully realize that
this bug was *not* filed simply against the particular shortcoming of the PAC
code until the patch was already posted.

I'd love to fix the real failover problem, and maybe I'll try, but it's a more
ambitious effort and I need to spend more time with the code and probably
inspire a bit more trust with the network crew first.  I have half a mind at
this point to postpone the non-79057 bit but whatever, it does a little bit of
good at least.

I'll look into the reentrancy issue.

Comment 22

[Chase Tingley]

*sigh*

Upon further consideration and discussion I'm icing this patch until I or
someone else produces something monstrous to fix the underlying issue.  In the
short term it adds little (as it is much for likely for the proxy to be
unreachable rather than unresolvable), and in the long run it is of dubious
value and would quite possibly just be rewritten.

I'm splitting off the bug 79057 stuff and posting it there.  Sorry for all the
noise.

Comment 23

[serge (gone)]

working on this

Comment 24

[serge (gone)]

Attachment: the first patch proposal

Comment 25

[Chase Tingley]

Woah.  This appears to also fix bug 91630 and a number bugs on feedback in error
cases.  It also includes the alert() code from bug 86846.

I don't know enough about the thread architecture yet to comment on the event
stuff just by looking at it.  I'll try the patch out to get more feedback.  In
the mean time, a couple things:

* Is there something better than an array for keeping track of the servers
(gNodeArray)?  This code is begging for some sort of hashing.

* What's up with this:

+            if (pac == '' || pac.search("<TITLE>Not Found</TITLE>") != -1) {
+                //httpd can return <TITLE>Not Found</TITLE><H1>Not Found</H1>

Are the status/errorMsg params not enough?

* Rather than doing this to catch the error,

+            if (pac.search("FindProxyForURL") == -1) {

why not put a try block around the assignment
             LocalFindProxyForURL=ProxySandBox.FindProxyForURL;

iirc, this will currently throw an exception if FindProxyForURL does not exist
or is not well-formed.)

and nits (not counting obvious debugging code, etc)...
- the regexp pattern "/\s* \s*/" should be "/\s+/"
- the if(!canDoFailover) block contains the typo "rerurn"
- the PACF_TYPE_SOCKS{,4,5} constants should be "socks{,4,5}" rather than
  "sock{,4,5}" (iirc -- nsProtocolProxyService.cpp appears to return "socks" in
  a similar situation)
- s/Cer/Cr/("CereateAndPostMainThreadEvent")
- var PACF_* should be const PACF_*

cool.  i'm very eager to try this out.

Comment 26

[benc]

This feature is pretty important, so lets ask any heavy hitters we need for
help, like brendan.

Also, is there any reason why this patch is addressing several bugs?

Comment 27

[serge (gone)]

tingley, thanks for the comments,
I would like to notice the patch is *FIRST DRAFT PROPOSAL I posted mostly for
backup reason.

>* What's up with this:
>+            if (pac == '' || pac.search("<TITLE>Not Found</TITLE>") != -1) {
>+                //httpd can return <TITLE>Not Found</TITLE><H1>Not Found</H1>
as I said in comment client can get valid content, for example 
"the requested object does not exist on this server." from httpd and no
status/errorMsg is set. 
 
>why not put a try block around the assignment
> LocalFindProxyForURL=ProxySandBox.FindProxyForURL;
what is the reason to create new sandbox object from invalid source? 

Comment 28

[grega]

fix for spring release.  marking nsenterprise-.

Comment 29

[serge (gone)]

Attachment: the patch #2

Comment 30

[serge (gone)]

the patch#2 is actually workable patch, 
[e-mail] could you try to take a look on this, please?

Comment 31

[serge (gone)]

Attachment: addition to patch#2 proxies.properties

Comment 32

[serge (gone)]

Attachment: this has all files in, disregard all previous ones

Comment 33

[Darin Fisher]

some comments:

1- please add descriptive comments to each IDL method.

2- please put the nsHttpChannel code in a member function.
about the design:

is there any chance that the code for PAC failover can be handled at the socket
transport level completely instead of at the protocol level?  you may be able to
modify nsIProxyInfo to convey any necessary info down to the socket transport. 
why wouldn't such an approach be viable?

Comment 34

[serge (gone)]

1&2 will be done.
I like the idea to handle  pac failover at socket transport level, but I'm not 
sure if it's possible to handle all JS dialogs on non ui thread? Any examples?

Comment 35

[Darin Fisher]

PSM launches dialogs from the socket thread... it probably involves
synchronously proxying the dialog invocation over the ui thread.

Comment 36

[Chase Tingley]

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

Comment 37

[serge (gone)]

>is there any chance that the code for PAC failover can be handled at the socket
>transport level completely instead of at the protocol level?
intuitively it feels like the socket transport level is an appropriate place to 
do so, and this is true for statically build pac failover list, but pac spec 
allows to change that list dynamically.
Here is one example how: 
function FindProxyForURL(url, host) {
 if (timerange(0, 0, 0, 0, 0, 30)) {//between midnight and 30 sec past midnight.
   return "PROXY proxy_0; PROXY proxy_1";
 }
 else if (timerange(0, 0, 0, 0, 1,0)) {//between midnight and 1 min past
   return "PROXY proxy_2; PROXY proxy_3";
 }                    
 else if (timerange(0, 0, 0, 0, 2,0)) { //between midnight and 2 min past
   return "PROXY proxy_4; PROXY proxy_5";
 }
...
for the same arguments FindProxyForURL() returns different proxies depends on 
the current time, it means from socket transport level we should be able to make 
js call FindProxyForURL(url, host) to get new proxy info, but we know nothing 
about what url we are processing at this point.
So it looks to me the protocol level is the right palace to handle pac failover. 
Darin, what do you think, am I wrong?

Comment 38

[Darin Fisher]

right, plus you don't want to have to deal with the fact that the socket
transport is running on a background thread... sync proxying to the UI thread in
order to call JS code should be avoided.

Comment 39

[Bradley Baetz (:bbaetz)]

darin, serge: The proxyinfo object passed down to the socket transport stuff was
designed to be extensible. All you have to do is add a next attribute to it, and
extend the internal PAC interfaces to return a list of values, and then you have
all the data you need in a linked list - no calls to js needed. I don't know how
to do an out array param in idl, though.

Comment 40

[Darin Fisher]

bbaetz: but serge was saying that he wanted to query the PAC file each time...
am i missing something?

Comment 41

[Bradley Baetz (:bbaetz)]

AIUI, serge was saying that he needs to query PAC each time.we make a
connection. I'm claiming that whilst pac needs to be queried, it doesn't have to
be the socket transport thread which does the querying.

Note that the http proxy code would need to be tweaked, especially the "should
we reuse this connection" logic.

Then theres also the SOCKS fallback to http logic (or the other way arround),
where we run into the same problems wrt people QIing for an http channel if
we're doing ftp.

Comment 42

[serge (gone)]

bbaetz: timerange() function from pac spec allows to build proxy list 
dynamically (se my example above),
so we have to call  FindProxyForURL() to get the current proxy list...

Comment 43

[Bradley Baetz (:bbaetz)]

PAC is called (when enabled) every time a proxyinfo object is created. This
happens in NewChannelFromURI (usually). The proxy info is passed into the
sockettransport service. These proxyinfo structures are not cached by anyone.

Or are you saying that if we start connection at 29 seconds past midnight, and
fail at 31 seconds past midnight, we need call pac again to see if anything has
changed?

Comment 44

[serge (gone)]

That is exactly what I'm trying to say,
well, of course in real life this is very rare situation,
but why we cannot assume that same ISP wouldn't do some sort of balance loading 
or shuts down their set of proxy servers and configure user's pac to handle 
failover depends on particular time?

Comment 45

[Bradley Baetz (:bbaetz)]

Then tough. We get a list at the time we call PAC, and we need to try every
entry in that line. Thats why there are multiple entries - we should not call
PAC again; its just too confusing. I doubt ns4 did that, and I really don't see
the point. Its a _lot_ of additional complexity for 0 gain.

Think about it - if all proxy servers are down, would you just keep calling pac
forever?

I do agree that it will need code in at least the http protocol. Doing socks<->
http is going to be impossible though, w/o a wrapper channel which we've all
agreed (in other bugs) won't work.

Comment 46

[benc]

I don't think the time-based features was ever intended to create this type of
tight control...

If someone tried to connect right before midnight, then failed over, and talked
to an offline proxy, it would not be a big deal. They would fail to connect.
They would say some choice words, hit reload, and then re-PAC, and get the new
(post-midnight) directives and connect.

A possilbe feature to correct this would be possible in PAC2, which would be
allow a reload() function that could be time based.

Comment 47

[serge (gone)]

Attachment: serge's patch v2

Comment 48

[serge (gone)]

darin, bbaetz: could you take a look on attachment id 56789, thanks.

Comment 49

[Chase Tingley]

+        } else if (pac == '' || pac.search("<TITLE>Not Found</TITLE>") != -1) {

Trying to sniff failure codes out of raw HTML is a bad idea.  Letting the js
parser validate the integrity/correctness of the PAC file (by calling
evalInSandbox()) is more heavyweight, but who cares?  It's an operation we only
have to do once (and if the PAC is valid, one we would have to do anyway), and
it will catch all sorts of things that string searches won't.

Comment 50

[serge (gone)]

just try comment out
        }/* else if (pac == '' || pac.search("<TITLE>Not Found</TITLE>") != -1) 
{
...
        } else  if (pac.search("FindProxyForURL") == -1) { //check if there is 
function FindProxyForURL()
...
        }
        */
and for unknown pac, but valid http://host after evalInSandbox() you will get 
something like:
"Error: syntax error
Source File: .....
Line: 211 Source Code:
<TITLE>Not Found</TITLE><H1>Not Found</H1> The requested object does not exist 
on this server. The link you followed is either outdated, inaccurate, or the 
server has been instructed not to let you have it."
Do you think this is an apropriate message for the end user, rather than 
"ProxyPACNotFound"?

Comment 51

[Bradley Baetz (:bbaetz)]

So you need to check if its http and the http response code starts with a 2.
(Yes, doing it that wasy sucks). For other protocols, just assume success if you
get data back.

You should print a useful error message if the js engine fails to parse the
script, as well.

Don't do pac.search() - let the js engine parse the file, and then check if
(FindProxyForURL in ProxySandBox). I tihnk that that will work.

I haven't had time to look at this patch in detail.

Comment 52

[serge (gone)]

You are right, the new patch is following.

Comment 53

[serge (gone)]

Attachment: patch v3

Comment 54

[Bradley Baetz (:bbaetz)]

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

Comment 55

[Matthew Peter Tippett]

Is there anything holding up the commit of the patch?  Is there anything that
I can do to speed up the commit?

Comment 56

[Xiaobin Lu]

This is a very nice to have feature. We need to get this fixed ASAP.

Comment 57

[benc]

+helpwanted, mozilla1.0, nsbeta1.
serge, is there a reason you didn't nominate this for any of the post 6.2
milestones? Like mozilla 0.9.8, mozilla 0.9.9?

Comment 58

[serge (gone)]

lack of review, and some internal reorg is keeping this bug untouched fro a long 
time.
reassign this to gagan for properly assignment in necko team.

Comment 59

[Gagan]

I believe the new owner for this code is in trudelle's team. Letting him make
the call on that. 

Comment 60

[Peter Trudelle]

->morse

Comment 61

[Peter Trudelle]

nsbeta1- per Nav triage team.

Comment 62

[benc]

+nsbeta again.

If you minus, please provide reason for nsbeta-. 

This is probably the most serious PAC bug.

This is a specified feature, and there is no workaround. If we don't support
this, we have to still test for handling multiple directives correctly, and a
couple other special conditions.

Historically, really large enterprise customers have selected PAC over manual
configs because it offers more reliable connectivity.

Comment 63

[Peter Trudelle]

This is only a problem if the primary server is not present, it works in the
overwhelmingly typical case.  Also, it is only an issue among enterprise
customers who use PAC, and we are not aware of any that consider this a
high-priority problem for MachV (if you know any, please cite them here).  It is
getting very late in the release cycle to be considering changes that have low
impact (severity X number of users), and a correct fix for this seems risky too.
 Finally, we have shipped a few times with this defect already.  Do you have
strong reasons why fixing this is critical for MachV?

Comment 64

[benc]

This is a "chicken and egg" bug (some people think ALL bugs are this way, but
this one really is...)

The really big IT shops really like this feature. Even if PAC failover is needed
rarely, and is very slow (our Communicator failover was kind of sluggish), it
works. Every call from a down server costs money, esp. if your help desk is
outsourced.

Most really big IT shops will not consider using Netscape 6 unless it has
features like this, so they have not been complaining because they have not be
using it.

If the marketing is to sell to these big shops, I'm telling you now you want
this to work. When I supported PAC and Proxy server, I talked to NationsBank,
CSFB, Citicorp, Chase, JP Morgan, etc. (the names have changed, but you know
what I mean).

I don't see a lot of contributors that work for these organizations, so I've
either proven my point or completely missed the boat.

If we ship without this, expect this to be on the short list of things big
customers demand.

Comment 65

[Peter Trudelle]

nsbeta1-, per Nav triage team. If they were demanding it, we'd plus.  cc
jhooker, will consider plus if he has strong demand from enterprise.

Comment 66

[Gagan]

I agree that this is a valuable bug to fix. Perhaps all that is needed is
someone verifying that Serge's patch is still valid and does work. Any volunteers?

Comment 67

[Stephen P. Morse]

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

Comment 68

[Stephen P. Morse]

cc'ing jhooker on this.  Jeff, please let us know whether or not this is 
considered essential for the enterprise folks.

Comment 69

[Jeff Hooker]

PM chime in - this is considered essential for the enterprise deployments using PAC.

Comment 70

[Stephen P. Morse]

The patch v3 has bit-rotted -- specifically the "-80,52 +140,111" part of the 
patch for nsProxyAutoConfig.js.  Attaching a revised patch for just that one 
file.  Serge, can you check and see if the revised patch is still doing what 
your original patch did?

Also renominating for nsbeta1 based on jhooker's input (comment 69).

Comment 71

[Stephen P. Morse]

Attachment: replacement for nsProxyAutoConfig.js part of patch v3

Comment 72

[Stephen P. Morse]

The patch v3 does not compile.  Specifically, mReloadPACURLCount is undefined.

Serge, can you post a new patch?

Comment 73

[Stephen P. Morse]

OK, reviving serge's patch is going to take much more work than I originally 
thought.  By his estimates, it would take about a week of work.  Therefore I 
don't see any way that this one can make it for Netscape 7.0.

That's unfortuante since the enterprise group has now indicated that this is 
essential. 

Comment 74

[Peter Trudelle]

I just talked with Jeff, and we agreed that this doesn't look like a good bet
for 7.0 at this point.  This will be important for enterprise customers who try
to deploy, but currently there are none, and it is doubtful that a large patch
with little/no immediate benefit will be allowed onto the branch the last week
before zarro boogs anyway.  We do expect this to be on the short list of fixes
that big enterprise customers will demand, and that will no doubt be a major
focus of the point release.  If anyone discovers how this can be fixed with less
work/risk, or finds a large customer who would deploy 7.0 except for this bug,
please renominate.

Comment 75

[Stephen P. Morse]

Attaching much shorter and much safter patch.  It doesn't do everything that 
serge's patch did, such as putting up dialogs.  But it does do the failover, and 
that's what this bug report is all about.  And this patch has a better chance of 
being approved at this late date.

A significant difference between this patch and serge's is that the way I keep 
track of which retry I am up to is by adding a ProxyRetryCount field to the 
ProxyInformation.  Also I've added an mProxyRetryCount member to nsHttpChannel 
for the same reason.  This way I don't have to keep a node structure in 
nsProxyAutoconfig.js to maintain this state.  That was a substantial 
simplification.

Most of the changes in this patch are simply to account for this extra 
ProxyRetryCount member.  The actual changes to handle the failover itself are 
relatively small.

Renominating for nsbeta1 in the hopes that we can still get this in for Mach V.

Comment 76

[Stephen P. Morse]

Attachment: simpler patch that does just failover and nothing more

Comment 77

[Stephen P. Morse]

Comment on attachment 87761 [details] [diff] [review]
simpler patch that does just failover and nothing more

r=caillon (from morse's computer) with a few changes that I had him make. 
darin or someone in necko should probably sr= this.

Comment 78

[Stephen P. Morse]

Attachment: same patch but with minor changes per caillon's review

Comment 79

[Stephen P. Morse]

Comment on attachment 87765 [details] [diff] [review]
same patch but with minor changes per caillon's review

r=caillon, carried forward from previous patch

Comment 80

[Bradley Baetz (:bbaetz)]

Comment on attachment 87765 [details] [diff] [review]
same patch but with minor changes per caillon's review

This code hooks into http. What about fallover involving socks proxies or even
DIRECT, which is often the last thing on the list? How will/can that be
handled?

Fallover from http->direct/socks with an ftp url, using this code, will end up
really confusing things, because the http protocol handler can't deal with
those, unless there is an http proxy involved.

Also, you can't have ProxyForURL requery the PAC js file, because the PAC file
could switch over based on the time of the day, or teh destination IP (think
roundrobin DNS) The entire string needs to be stored in the proxy info as an
array, then your requery code can just iterate through that.

I realise that we want a quick fix, and that those issues won't affect the vast
majority of installations, so long as you file a new bug, the only thing which
needs to be fixed is not doing fallback from an HTTP proxy to SOCKS or none if
the url type is not http/https AND you test fallback from http to none with,
say, and ftp url.

Do file a bug on the rest, though.

Comment 81

[Stephen P. Morse]

Yes, you are correct that we are looking for a quick fix here.  Can you modify 
the existing patch (or post a new one) that also does the HTTP SOCKS failover 
and tests for non http/https.  Without someone posting a better patch quickly, 
we either go with this one now or we get nothing.

jhooker, what are your feelings about the functionality in the existing patch?  
Would this be better than nothing for the existing enterprise customers?

Comment 82

[Stephen P. Morse]

> This code hooks into http. What about fallover involving socks proxies or
> even DIRECT, which is often the last thing on the list? How will/can that
> be handled?

DIRECT is handled by the current patch.

Comment 83

[Bradley Baetz (:bbaetz)]

I'm not really set up to do this ATM, or even to test (uni exams/assignments)

Does the patch handle an ftp url, where the pac file returnes:

PROXY some-unknown-host:1234, DIRECT

? If it does, then thats ok, but I see nsHttpChannel::ProxyPACFailover
explicitly calling NewProxiedChannel on nsHttpHandler::get(), and that won't
work for ftp. You should either a) be calling nsIIOService::getProtocolHandler,
(special casing http), then trying to QI to nsIProxiedProtocolHandler, or b) add
a newChannelWithProxyInfo method to the io service, and call that (factoring the
current NewChannelFromURI stuff out)

b) is cleanest and simplest, it already handles the special cases for you. You'd
change the nsIIOService code like this (uncompiled/untested):

NS_IMETHODIMP
nsIOService::NewChannelFromURI(nsIURI *aURI, nsIChannel **result)
{
    nsresult rv;
    NS_ENSURE_ARG_POINTER(aURI);
    NS_TIMELINE_MARK_URI("nsIOService::NewChannelFromURI(%s)", aURI);

    nsCOMPtr<nsIProxyInfo> pi;
    rv = mProxyService->ExamineForProxy(aURI, getter_AddRefs(pi));
    if (NS_FAILED(rv)) return rv;

    return NewProxiedChannelFromURI(aURI,pi,result);
}

NS_IMETHODIMP
nsIOService::NewProxiedChannelFromURI(nsIURI* aURI, nsIProxyInfo* pi, nsIChannel
**result) {
    nsCAutoString scheme;
    rv = aURI->GetScheme(scheme);
    if (NS_FAILED(rv)) return rv;

    nsCOMPtr<nsIProtocolHandler> handler;

    if (pi && !strcmp(pi->Type(),"http")) {
        // we are going to proxy this channel using an http proxy
        rv = GetProtocolHandler("http", getter_AddRefs(handler));
        if (NS_FAILED(rv)) return rv;
    } else {
        rv = GetProtocolHandler(scheme.get(), getter_AddRefs(handler));
        if (NS_FAILED(rv)) return rv;
    }

    nsCOMPtr<nsIProxiedProtocolHandler> pph = do_QueryInterface(handler);

    if (pph)
        rv = pph->NewProxiedChannel(aURI, pi, result);
    else
        rv = handler->NewChannel(aURI, result);

    return rv;
}

Which, apart from getting the scheme after the proxy info, rather than before,
is literally just adding a closing brace and a method declaration.

However, I know darin wanted to move this stuff out of the ioservice a bit, into
the protocolproxyservice, and even had a patch to do so. Not sure if this would
meet with his approval - he may want NewProxiedChannelFromURI there, but thast
just cut and paste in any event.

darin?

Comment 84

[Darin Fisher]

Comment on attachment 87765 [details] [diff] [review]
same patch but with minor changes per caillon's review

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+    // PAC failover stuff (mProxyRetryCount of -1 means no more proxies to try)
>+    if ((status == NS_ERROR_UNKNOWN_HOST || status == NS_ERROR_CONNECTION_REFUSED ||
>+            status == NS_ERROR_NET_TIMEOUT || status == NS_ERROR_NET_RESET) &&
>+            (mProxyRetryCount != -1) && mConnectionInfo->UsingHttpProxy()) {
>+        if (NS_SUCCEEDED(ProxyPACFailover(++mProxyRetryCount))) {
>+            // pac created new channel so don't propogate error status
>+            // to current channel's listener
>+            status = NS_OK;
>+        }
>+    }

code like this actually needs to live in nsHttpChannel::OnStartRequest.  you
need
to check the status of the transaction in OnStartRequest and suppress sending
mListener->OnStartRequest until you have made a successful proxy connection. 
as
is your patch results in multiple OnStart/OnStop request notifications, and
that
will cause all sorts of chaos in docshell land.

i have some ideas about how to solve this bug, and i think we need to have a
design meeting to come up with the best solution.  hacking HTTP like this will
work for HTTP proxies, but it doesn't help other proxy consumers.  i think we
should spend some time and explore other solutions.  we still have time...
let's
not rush in a partial solution, ok?

morse: can we meet this monday to discuss this bug?

Comment 85

[Jeff Hooker]

PM chime in - per steve's comment (Attaching much shorter and much safter patch.
 It doesn't do everything that serge's patch did, such as putting up dialogs. 
But it does do the failover, and that's what this bug report is all about.  And
this patch has a better chance of being approved at this late date.) it sounds
as if this patch would be of value to the customer base. If a particular feature
does not work as expected it makes it difficult to get past the evaluation stage
to be even considered for deployment. Again, the summary (does do the failover)
sounds like a valuable resolution.

Comment 86

[Paul Wyskoczka]

Adding nsbeta1+ and [adt2 rtm] per nav triage team.

Comment 87

[Stephen P. Morse]

reassigning to darin who said he might have time to do some work on it.

Comment 88

[Darin Fisher]

actually, i'm going on vacation tomorrow and won't be able to look at this until
mid-july.  how critical for RTM is this?

Comment 89

[Bradley Baetz (:bbaetz)]

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

Comment 90

[Gagan]

->morse (per darin's comments)

Comment 91

[Stephen P. Morse]

reassigning to darin at his suggestion.

Comment 92

[Darin Fisher]

future (until we find a real PAC owner or until i find time)

Comment 93

[Jo Hermans]

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

Comment 94

[Marek Z. Jeziorek]

batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

Comment 95

[Robert Wall]

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

Comment 96

[Jeff Hooker]

removed jhooker from cc: list

Comment 97

[Jaime Rodriguez, Jr.]

Changing from topembed, to embed, as this is not currently blocking a major
embedding customer.

Comment 98

[Darin Fisher]

-> moz 1.3 beta

Comment 99

[Andrei Boros]

Coming here from bug #79893 :

In a related incident to comment #13:

Our proxy.pac file is loaded as:
http://internal.server/path/proxy.pac (pretty generic)

function FindProxyForURL(url, host)
{
    if (dnsDomainIs(host,"internal")) {
        return "DIRECT";
    } else

     if (isInNet(myIpAddress(),"192.168.0.0","255.255.255.0")) {
        return "PROXY proxy1.internal:3128; PROXY proxy2.internal:3128; DIRECT";
     } else
       return "DIRECT";
}

In this file I respect the specs with host:port.

Mozilla 1.2b behaviour:
always uses DIRECT.

Comment 100

[Matthias Versen [:Matti]]

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

Comment 101

[benc]

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

Comment 102

[benc]

flagged for 1.3b.
Need to know if this is planned for 1.3b so I can start working on testcases.

Deferral to 1.3f is fine from a testing and feature perspective, but waiting for
1.4 is probably too much, because it would miss a lot of 1.3 branch based products.

Comment 103

[Darin Fisher]

no plan to work on this for mozilla 1.3.  maybe during the 1.4 cycle.

Comment 104

[wbayer]

I agree strongly with Comment #102: "Deferral to 1.3f is fine from a testing and
feature perspective, but waiting for 1.4 is probably too much, because it would
miss a lot of 1.3 branch based products."

Please maintain the 1.3f target.

Comment 105

[Jo Hermans]

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

Comment 106

[Matthias Versen [:Matti]]

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

Comment 107

[Darin Fisher]

might still happen for 1.4 ...

Comment 108

[greg baker]

Same here.

I can't speak for the enterprise corporation I work for, other than I'm a
Unix/Network sysadmin for about 3000 hosts (500 users) in a division of said
corporation.

Corporate (central) IT has asked us to use/implement the official proxy.pac
autoconfiguration script since they use it seamlessly on all of their windows/IE
boxes to handle failover between our different sites - and this is their method
  to handle seamless proxy/firewall upgrades for end customers.

Example autoconfiguration script:

http://internal.proxypac.com/proxy.pac

function FindProxyForURL(url, host)
{

// blah blah blah - filtered out but here's the sticker
proxyPath = "PROXY MIDWESTPROXY:74; PROXY WESTCOASTPROXY:74";

return proxyPath;
}

So when upgrades to MIDWESTPROXY happen all windows/IE users bounce
automatically to WESTCOASTPROXY; all unix/mozilla users that I support have to
reset their proxy to MANUAL and choose WESTCOASTPROXY.

I'd really like to see this happen...

Thanks!

Comment 109

[wbayer]

Thanks for the great work! The flurry of 1.4 related activity over the past few
weeks is impressive. 

I see 80918 (Proxy: "No Proxy for:" does not support ip address wildcards) was
checked in yesterday. Can this bug (PAC failover) also be prioritized for 1.4b? 

Either way, given the number of other Mozilla 1.4 fixes and enhancements, we
will likely deploy it (or a Mozilla 1.4 based Netscape release) corporately.

Comment 110

[Darin Fisher]

Attachment: v2 patch

this patch is a work in progress.  i've taken a simpler route i think.	instead
of querying the PAC component multiple times, i query it once and build up a
list of nsIProxyInfo objects.  each has an owning reference to the next.  this
meant moving the PAC string parsing into nsProtocolProxyService.

the HTTP code picks up errors in OnStartRequest and tries to failover to the
next nsIProxyInfo.  if there isn't a next nsIProxyInfo, then i try a direct
connection (note: this will happen even in the case of manual proxy
configuration, which is i think a nice feature).  the HTTP code creates a new
channel and does a "load replace" ... this patch just copies code from the 3xx
redirect code.	the two code paths should overlap to avoid duplicate code.

finally, it might be good to remember failures somewhere... probably
nsProtocolProxyService, but that'll require a method call from the HTTP code. 
i might add a "MarkFailed" method on nsIProxyInfo.  the implementation of that
interface could then add something to a global hash table to remember the
failure.  we could expire these remembered failures after some interval (maybe
10 seconds or 1 minute or something progressive).

anyhow, with a little bit of cleanup, this patch should be good for 1.4 beta.

Comment 111

[Darin Fisher]

Attachment: v2.1 patch

revised patch.	this one is ready for reviews.	changes include:

1- no longer automatically failover to direct connections.  only do this if
explicitly given in the PAC file.  a number of reasons why this is good: a)
compatible with expectations, b) DIRECT could appear between PROXY decls, c)
failover to DIRECT may not be desirable, d) messes up proxy error reporting..
we end up reporting the last error, which would be errors to directly connect. 
enough reasons to not go there right now IMO.  later on we could add a pref to
always failover to DIRECT.

2- don't bother remembering failures.  i think it would be best to get the
basic functionality in place first, and then go back and add this part later.

3- merged ProcessRedirection and ProxyFailover code.  created new helper method
on nsHttpChannel called SetupReplacementChannel.

Comment 112

[Darin Fisher]

Comment on attachment 120546 [details] [diff] [review]
v2.1 patch

bbaetz: can you please review this patch.  i think you'll find it fairly
straightforward.. just please check out my previous comments outlining the
changes.  thx!

Comment 113

[Bradley Baetz (:bbaetz)]

Comment on attachment 120546 [details] [diff] [review]
v2.1 patch

Yeah, don't fall back to DIRECT. Lots of organisations block direct access, and
those which don't will list it at the end.

This looks fine, but I'm really not in a position to test now, and won't be for
the next couple of weeks at least.

Comment 114

[Darin Fisher]

bbaetz: hrm.. i have tested this fairly well.. if you have suggested tests for
this bug, could you please list them here.  i can then run your tests =)

Comment 115

[Alec Flett]

Comment on attachment 120546 [details] [diff] [review]
v2.1 patch

this all looks fine to me. I don't know if I'm really much of an expert here
but it looks like the meat of this is SetupReplacementChannel and 
ProxyFailover, both of which seem fine. My only question would be if  we
already had some kind of code for replacing a channel that we could leverage

oh, one nit:

+    if (httpChannel) {
+	 if (preserveMethod) {
+	     if (mUploadStream) {

...
+		 }
+	     }
+	     // must happen after setting upload stream since SetUploadStream
+	     // may change the request method.
+	    
httpChannel->SetRequestMethod(nsDependentCString(mRequestHead.Method()));
+	 }

can we combine some of that if() logic, to reduce indentation here?

sr=alecf with that.

Comment 116

[Darin Fisher]

alecf: yeah, i think actually i can get rid of maybe two levels of indentation.
 thx!

Comment 117

[Doug Turner (:dougt)]

Comment on attachment 120546 [details] [diff] [review]
v2.1 patch

nit: f you going to add your name AND gagan's name to the
nsIProxyAutoConfig.idl file, you should also add the rest of the contributors.

nit: indent your case's:
+    switch (len) {
+    case 5:

looks fine to me.

Comment 118

[Darin Fisher]

fixed-on-trunk :-)

Comment 119

[benc]

FYI: to resolve this bug, I'm going to do a two-PROXY run.

I'll eventually add tests for greater than two, as well as multiple directive
testing. 

Comment 120

[Aleksey Nogin]

This check-in have added a warning on Brad TBox:

+netwerk/base/src/nsProtocolProxyService.cpp:425
+ `const char*hostEnd' might be used uninitialized in this function

Comment 121

[Darin Fisher]

Aleksey: thanks!  (fortunately, not a real bug.)

Comment 122

[Jo Hermans]

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

Comment 123

[Jo Hermans]

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

Comment 124

[Hristo Grigorov]

Bug does not seem to be fixed. My Mozilla 1.4b refuses to load proxy.pac file 
from remote http server. Reload button does not do it either. Actually it seems 
that Reload button does nothing. Not to say that if I enter incorrect URL and 
click Reload button it just does not warn me about. When I downgrade to Mozilla 
1.3 everything works fine excluding the failover. 

Anybody else with similar problem ? Could it be that I upgraded from 1.3 to 
1.4b ? May be clean install is needed ?

Comment 125

[Darin Fisher]

Hristo: yes, a fresh install is required.  bug 205514 tracks the problem you're
experiencing.

Comment 126

[benc]

VERIFIED: all plats, Mozilla 1.4b

I'm updating the testcases and working through the releated bugs as well.

I've filed bug 207633 for the remaining issues about failover behavior (failover
is not stateful, it doesn't remember that a server was unavailable, so it has to
have a connection error and then failover every time PAC is called).

This bug has gone through two or three design cycles, and has enough drift about
the expected behavior to confuse most people.

If you have a problem post 1.4b, please look for duplicates and file a new bug
if needed.