83984 - PAC:autoproxy (PAC) should not load at startup

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[basic]

PAC should not load at startup, rather it should only load when it is needed. On
a win32 dialup system, configured for auto dialing, it is rather annoying that
starting mozilla causes a dialup just because it is configured with autoproxy.

Comment 1

[benc]

*** This bug has been marked as a duplicate of 80885 ***

Comment 2

[basic]

How is this bug a duplicate bug 80885? Autoproxy is turned on in this bug. Just
that the browser doesn't need to use it when accessing files on the local hard
drive. In bug 80885, autoproxy seems to be turned on even after it is turned off
in preference.

Comment 3

[benc]

Its the same thing. Whoever did this needs to check for network.proxy.type=2 and
also needs to move the code so it fires at the right time.

Comment 4

[basic]

This bug report was made a duplicate of bug 80885, which was made a duplicate of
bug 85290, which is now fixed (as of build 2001061120 win32 talkback sea
installer). I don't see the PAC:Loading PAC from <URL> but Mozilla still
activates the autodialer on startup.

Reopening

Comment 5

[neeti]

-->PAc

Comment 6

[Jussi-Pekka Mantere]

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

Comment 7

[basic]

I don't see this when I set to direct connection. My ISP have stop using PAC, I
am nolonger capable of testing this bug with a working PAC. This might only be
reproducable the first time PAC is configured or when an invalid PAC is entered.
Note: I load a blank page on my startup, not my homepage or previous page.

Comment 8

[Chase Tingley]

I was taking a look at what would be necessary to implement this last night, and
the tricky bit seems to be that there's a race condition -- the PAC file is
loaded asynchronously.  So if PAC loading was triggered by a call to
nsProtocolProxyService::ExamineForProxy or something, in all likelihood it would
still be loading when the PAC code was actually executed.  In this case,
nsProxyAutoConfig.js will default to "direct".

http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsProxyAutoConfig.js#54

The net effect is that the first url (or possibly more) loaded wouldn't be
proxies under the current implementation, regardless of what the PAC says.

There's also a rather ominous comment about potential deadlocks.  Anyways, this
warrants more study.

On a related note, if this were implemented, should it be a pref?  Some users
may not want this.

Comment 9

[benc]

PAC should only fire as an initialization event of the first network based event
that is proxyable.

I don't know why PAC would always load on startup, before any real network activity.

Comment 10

[tao]

oen side effect of this: see 103979 which block 102958

Comment 11

[basic]

adding back depend.
everybody please be careful of midairs

Comment 12

[serge (gone)]

Attachment: patch v1

Comment 13

[serge (gone)]

Attachment: patch v2

Comment 14

[serge (gone)]

cc darin; seeking for r/sr=

Comment 15

[Chase Tingley]

+  //continue from here means the current URL wouldn't be proxied: *FIX_ME* 

As I understand it, the first URL the user tries to load will instantiate the
nsProtocolProxyService and kick off PAC load.  But since that happens
asynchronously, this first URL will probably not have the benefit of
PAC/proxying.  Is this assessment correct?

Comment 16

[serge (gone)]

yes, this is correct.

Comment 17

[Chase Tingley]

In that case, I'm a little confused by the presence of |allowProxiedProtocols|
and the flags.  The same effect can be achieved by instantiating
nsProtocolProxyService normally, but altering ConfigureFromPAC() to set a
"loadOnNextRequest" flag, then kick off PAC load if the flag is set when a
request comes in.  This takes a good deal less, code, iirc.  Six of one, half
dozen of the other though, I guess.

I had rejected this style of approach because I'm not sure how easy it is to fix
the resulting "first request doesn't proxy" problem without making PAC loading
synchronous -- it seemed like a fix for this bug would be immediately rewritten
to fix the other.  Have you got a good idea where this is going?

Comment 18

[serge (gone)]

>I'm a little confused by the presence of |allowProxiedProtocols| and the flags.
+    if (mMaskOfProxiedProtocolFlags & PROXIED_PROTOCOL_FLAGS)
will increase the performance, there is no needs to call  
mProxyService->ExamineForProxy()
but we do, even there is no proxy specified in user prefs.
>altering ConfigureFromPAC() to set a "loadOnNextRequest" flag, then kick off 
>PAC load if the flag is set when a request comes in.
the thoughts like this have crossed my mind,  but my approach easily allows to 
handle the case when LoadPACFromURL() failed, by setting 
mMaskOfProxiedProtocolFlags
+        ios.allowProxiedProtocols(true); //enable proxy
in /mozilla/netwerk/base/src/nsProxyAutoConfig.js
only after success on  load and evaluation of *.pac file.
>it seemed like a fix for this bug would be immediately rewritten to fix the 
>other.
I did not address "first request doesn't proxy" problem here, which  is a 
different bug, and it exists from beginning of nsProxyAutoConfig.js 
implementation, because of nsHttpChannel::AsyncOpen() call,
What I'm trying to say is I do not introduce "first request doesn't proxy" 
problem in my patch, I just clarify that problem in my comments.

Comment 19

[Darin Fisher]

i don't know about this bug... it seems to me that you'd want to load the PAC as
soon as possible, so as to help ensure that the user's homepage loads correctly.
however, i understand that PAC isn't needed unless we are loading a website, so
why bother delaying startup or why require the browser to be online just in
order to startup?  it's a difficult problem.

what we probably really need is to create a PAC dummy channel that wraps the
real channel.  this would only be done, of course, if the corresponding protocol
handler supported proxying.  then AsyncOpen is called on the dummy channel, we
could initiate the PAC download.  when that is complete, we could then redirect
the channel to the real channel.  this would be pretty easy to implement i think.

the solution of patch v2 worries me.  it don't think it is the best solution to
the problem, and it also seems to add more complexity to the IO service and
protocol proxy service than that which is really necessary.

Comment 20

[serge (gone)]

>so why bother delaying startup or why require the browser to be online just in
>order to startup?
that exactly why I moved in mozilla/netwerk/base/src/nsIOService.cpp (r1.115)
    180     rv = nsServiceManager::GetService(kProtocolProxyServiceCID,
    181                                       
NS_GET_IID(nsIProtocolProxyService),
    182                                       getter_AddRefs(mProxyService));
from
    150  nsIOService::Init()
into
     717 nsIOService::NewChannelFromURI(nsIURI *aURI, nsIChannel **result)

BTW: I think it should also fix bug103979
>then AsyncOpen is called on the dummy channel, we
>could initiate the PAC download.  when that is complete, we could then redirect
>the channel to the real channel.
I do not understand how it suppose to work,
how we can block all other proxying protocols and  wait until PACdownload is 
complete? What happen if  *.pac file is located on slow servers? 

Comment 21

[Bradley Baetz (:bbaetz)]

Just from a quick look, I don't really like this.

a) The mask flags need to be in nsIProtocolHandler, however, I don't think that
it is needed:
b) ExamineForProxy has all the proxy info. Thats intentional. Please do not move
knowledge of proxies into the ioservice. (More than there is already, at least)
c) Why did you & the flags from GetProtocolFlags with this mask? This function
is, as its name suggests, meant to return all protocol flags.
d) "prixytype"?
e) The stuff at the end of createChannelFromURI is wrong - you now won't set a
proxyinfo for ftp file, and so proxies will not work.

Also, there is wierd tab/space interaction

darin: When I fixed bug 89500, we agreed that a proxy channel wasn't really
possible. Also, consider socks+pac+irc/mailnews, where the protocolhandlers do
not implement nsIProxiedProtocolHandler (mailnews + my patch, that is). This was
a design decision, because these protocols create socket transports differently.

What about a new proxyType, of PAC_NEEDED, and then have the sockettransport
thread avoid creating the socket until the results come back? We can toss the
"java applet wants to know about this before pac is loaded" (java is the only
other place needing to act of the results of ExamineForProxy) into the too-hard
basket. 

Comment 22

[Bradley Baetz (:bbaetz)]

We need to block proxyable protocols until the pac file is loaded. Otherwise the
results will not be correct. See my other comment.

Comment 23

[Darin Fisher]

bbaetz: you're right... a dummy channel wouldn't work in general :(

the idea of creating using a proxyType would not work completely either i think.
consider what would happen if the PAC says that FTP should be proxied via HTTP
and we don't yet have the PAC.  then we'd incorrectly create a FTP channel. 
perhaps we need both solutions... or perhaps all consumers should implement
protocol handlers?!?  i really think the PAC dummy channel is a clean solution
for real protocol handlers.

Comment 24

[Bradley Baetz (:bbaetz)]

darin: Bleh. How did ns4 handle this?

Comment 25

[serge (gone)]

>b) ExamineForProxy has all the proxy info. Thats intentional.
>knowledge of proxies into the ioservice. (More than there is already, at least)
because of performance:
1) what is the reason to call ExamineForProxy() even when direct connection is 
set in user prefs?
2) what is the reason in ExamineForProxy() to call nsCOMPtr<nsIIOService> ios = 
do_GetService(kIOServiceCID, &rv); when we call it 
from nsIOService::NewChannelFromURI?
maybe it's worth to add additional parameter nsIIOService *ios to 
ExamineForProxy() and do not call do_GetService when ios != 0 ?

>c) Why did you & the flags from GetProtocolFlags with this mask?
well, lets suppose PAC loading failed, in this case I do not want  to 
ExamineForProxy() for any proxied protocols, the easy way to
do so is unset  ALLOW_PROXY flag for all those protocols. 
There are two trivial ways to do so:
implementing SetProtocolFlags() method in nsIProtocolHandler, or mask 
nsIProtocolHandler::protocolFlags where it's needed, I choose the easiest one.
I agree maybe it's not good idea to mask it in nsIOService::GetProtocolFlags().
>e) The stuff at the end of createChannelFromURI
do you mean nsIOService::NewChannelFromURI()
> is wrong - you now won't set a proxyinfo for ftp file, and so proxies will not 
>work.
I do not see any diffs in code for http and ftp or any other protocols, could 
you clarify what is wrong, please?
>What about a new proxyType, of PAC_NEEDED, and then have the sockettransport
>thread avoid creating the socket until the results come back?
I like this idea.

Comment 26

[Bradley Baetz (:bbaetz)]

> 1) what is the reason to call ExamineForProxy() even when direct connection is 
> set in user prefs?

proxy info belongs in the protocol proxy service. That said, passing in an
optional protocolhandler argument, and moving the mUseProxy bailout checks to
the top would be good.

>>c) Why did you & the flags from GetProtocolFlags with this mask?
>well, lets suppose PAC loading failed

We should pop up a large error message in that case, like ns4 does.

> in this case I do not want  to 
>ExamineForProxy() for any proxied protocols, the easy way to
>do so is unset  ALLOW_PROXY flag for all those protocols. 
>There are two trivial ways to do so:
>implementing SetProtocolFlags() method in nsIProtocolHandler, or mask 
>nsIProtocolHandler::protocolFlags where it's needed, I choose the easiest one.
>I agree maybe it's not good idea to mask it in nsIOService::GetProtocolFlags().

No, you shouldn't mask it there. Only mask where needed

>>e) The stuff at the end of createChannelFromURI
>do you mean nsIOService::NewChannelFromURI()

Yes, sorry

> is wrong - you now won't set a proxyinfo for ftp file, and so proxies will not 
>work.
> I do not see any diffs in code for http and ftp or any other protocols, could 
>you clarify what is wrong, please?

Looking at the diff, you appear to only QI to nsIPRoxiedPRotocolHandler for the
http case. That is wrong; socks proxies need to get the proxy info too. I can't
see what that hunk of the patch is trying to do.

I could just be misreading the patch, though.

>>What about a new proxyType, of PAC_NEEDED, and then have the sockettransport
>>thread avoid creating the socket until the results come back?
>I like this idea.

As darin mentioned, this won't work. Lots of places create a new channel, and
expcet to be able to QI it to an nsIHttpChannel immediately. We can't go
changing the type later.

Comment 27

[Bradley Baetz (:bbaetz)]

Actually, what do people think of marking this bug WONTFIX? Provided that we
don't try to fetch the url when pac is disabled, or we're offline, I think that
it would be best to begin loading the url as soon as possible.

This is important because of the lack of proxying while the pac file is being
loaded, but also to improve perceived response time.

Comment 28

[Chase Tingley]

The original complaint -- that launching mozilla triggers a dialup connection,
seems like a reasonable gripe to have.  On the other hand, it seems like most
solutions sucks, and delayed loading would introduce a noticeable latency in
response time of the first page someone tries to load (probably the start page).

There's a comment in bug 97605 that has always puzzled me -- Darin says:
> 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.

I don't entirye understand this proposal (I don't really understand the event
architecture).  It sort of sounds like it would make fixing this bug fairly easy
(and fairly important, since you wouldn't want a modal load in the startup
sequence).  Could we mark a depend and call it a day?

Comment 29

[Bradley Baetz (:bbaetz)]

tingly: right, but I'm proposing that we load this as soon as we go online. We
had another discussion a few months ago about pac listening for these
notifications, so that the myIPAdress stuff was correct.

If you don't want dialup to happen, then start up offline. I assume that we
allow this; 4.x did.

Pushing an event queue won't really help. Thats to do with running the pac
stuff; we just need to evaluate it at startup. Also, that bug is invalid - see
the comments I'm about to make there.

Comment 30

[Chase Tingley]

Tying the load to going online makes a lot of sense.  I still have code in my
tree somewhere to make PAC a listener for this -- it should be easy to modify it
to load the PAC as well.

Comment 31

[Darin Fisher]

how about if ExamineForProxy could return an error that said something like
WOULD_BLOCK... then we could have an extra function call like
AsyncExamineForProxy that callers could invoke to get the proxy info when it
becomes available.

this would allow the IO service to implement a dummy channel on behalf of
proxied protocol handlers, and it would also allow mailnews a facility for
asynchronously getting proxy info, without requiring any code changes to the
socket transport service.

Comment 32

[Bradley Baetz (:bbaetz)]

darin: No, that can't work. Consider someone typing ftp://ftp.mozilla.org/ into
the urlbar. Someone calls NewChannelForURI, and we need to return a channel. If
(and only if) we're using an http proxy, then that channel must be QIable to
nsIHttpChannel immediately.

Comment 33

[Darin Fisher]

bbaetz: why must the dummy channel be QI'able to nsIHttpChannel?

Comment 34

[Bradley Baetz (:bbaetz)]

Because you told me so, a few months ago? :)

Here's one random example, from nsDocShell::DoURILoad (line 4506) :

   rv = NS_OpenURI(getter_AddRefs(channel),
                    aURI,
                    nsnull,
                    loadGroup,
                    NS_STATIC_CAST(nsIInterfaceRequestor *, this));
    if (NS_FAILED(rv))
        return rv;

    channel->SetOriginalURI(aURI);

    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(channel));
    //
    // If this is a HTTP channel, then set up the HTTP specific information
    // (ie. POST data, referer, ...)
    //
    if (httpChannel) {

Now, we could argue that it doesn't matter for ftp-via-proxy, but you'd have to
audit all the callers. Handling expiry times would matter, though. And you would
need this for http/https over proxy. That could be done, possibly, I guess, but
it would be really really ugly.

Again, whats wrong with loading this at startup if we're unline? It is not
unreasonable to assume that a person loading a web browser will want to access
the web, in almost all cases.

Comment 35

[Darin Fisher]

did i say that? ;-)  </sigh>

your example mentions two things commonly added to a HTTP channel: POST data and
referrer header, but neither of these would apply to the first HTTP page
loaded... except maybe if editor starts supporting HTTP PUT for publishing
documents... then, the dummy channel would need to support nsIUploadChannel, and
then there'd probably be some other interface that would need to be supported. 
ok... perhaps this is the wrong way to go :(

Comment 36

[Bradley Baetz (:bbaetz)]

And don't forget ssl via connect.

I'm actually not sure how ftp upload via a proxy works - is it just a POST?

This is what we thought of originally for the socks stuff, but it was just too
complicated.

Comment 37

[Darin Fisher]

i don't recall thinking of it in exactly the same way, but yeah... makes sense.

Comment 38

[serge (gone)]

to default owner

Comment 39

[benc]

qa to me.

Can anyone comment as to the validity of this bug in recent milestones (i.e.,
what is our situation going into mozilla 1.0)?

Comment 40

[benc]

Temporarily "futuring" all PAC&SOCKS bugs to clear new-networking queue.

I will review later. (I promise)

If you object, and can make a case for a mozilla 1.0 fix, please reset milestone
to  "--" or email me.

Comment 41

[Jeff Hooker]

removed jhooker from cc: list

Comment 42

[benc]

+helpwanted, -futured.

possible cause of the "I need to reload PAC" bugs, it loads while people are not
on the network they expected to be on.

Comment 43

[Ray Charbonneau]

Bug #100022, bug #180811, and bug #188006 all appear to be dupes or related to
bug #83984.  Since #100022 has the most votes, and best matches what I'm seeing,
I'll add my vote there in the hopes that it will do some good.  This bug will
force my corporation to go to IE if it's not fixed soon!

Comment 44

[Dan Mellem]

This is more visible if Quick Launch is enabled. The browser may try to load the
PAC file before networking is available and then won't try again when the
browser is first used. It’s been a minor support issue for our wireless users
(who must authenticate before they can see the PAC server).

Comment 45

[benc]

Related problem: bug 243277.

Comment 46

[perillonpe]

hum.

it would be nice if we can force fx to load pac file. With fx 3.RC1 auto proxy do not work in my corporate network and fx forget anything about my proxy conf. 

So i have to set it manualy every time it start up my browser. Trust me, it's so boring i can stop using fx (even if i'm from firebird generation).

Comment 47

[Patrick McManus [:mcmanus]]

in the name of snappy, we really want to do things like this ahead of time if possible and the concern over dialup is significantly reduced these days. Most of the significant work of it happens off the main thread anyhow. So I'll wont fix this.