819902 - Slow performance when proxy PAC file is used, which is retrieved using "System settings"

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Frank Walburg]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121209 Firefox/20.0
Build ID: 20121209030817

Steps to reproduce:

Since some days, the performance of loading a website is very slow and sometimes aborts when using a proxy PAC file (lading to a http proxy) which is retrieved using "Use System Proxy Settings"
When I retrieve the same PAC file using the option "Automatic proxy configuration URL" within Firefox, loading of websites is fast.

Comment 1

[Patrick McManus [:mcmanus]]

Frank-how long has this been going on? About 1 week or several weeks?

What OS are you using?

Can you please provide the URL you are using? The details may matter. Feel free to substitute any hostname with www.example.com

Comment 2

[Patrick McManus [:mcmanus]]

Frank - also confirm you are talking about firefox 20(nightly).Thanks.

Comment 3

[Frank Walburg]

I'm using Windows 7 64bit
The problem exists since some days, so probably 1 week
I'm using firefox 20 (nightly)

Comment 4

[Frank Walburg]

it's independant of the hostname I'm using. Every external hostname (outside my company's intranet) shows this effect.

Comment 5

[Patrick McManus [:mcmanus]]

(In reply to Frank Walburg from comment #4)
> it's independant of the hostname I'm using. Every external hostname (outside
> my company's intranet) shows this effect.

Not just the hostname, sorry for the confusion. I need the whole URL to see if it is something that would undergone url normalization. which is what the patch that was landed about a week ago imapcted.

if you'd rather not put it in the bug can you email it to me [e-mail]

Comment 6

[Frank Walburg]

the problem occurs for example when using the following URLs:
http://html5test.com/index.html or
http://winfuture.de/

I will send you the PAC file via email. This PAC file is retrieved using the option "Use System settings". The loading of these URLs either abort with an error "Server not found, Firefox can't find the server at html5test.com." or takes more than 30 sec to load.
When using the same PAC file with the option "Automatic proxy configuration URL" the URLs are loading immediately.

Comment 7

[Patrick McManus [:mcmanus]]

I was unclear in what I needed and have sent frank a followup mail. The interesting bit of info is the PAC URL itself in the system settings. The code that changed a week ago dealt with what happens when that URL is updated and dealt with normalization of the PAC url (not the destination url).

Comment 8

[Frank Walburg]

I've sent a mail to Patrick with the PAC URL

Comment 9

[Patrick McManus [:mcmanus]]

Frank.. I can't reproduce anything yet.. 

I'm going to ask 2 favors of you.. 1 can you get me an http log?

https://developer.mozilla.org/en-US/docs/HTTP_Logging

But this is IMPORTANT - I need you to include proxy information too - so setup the log like so (include the proxy bit):
set NSPR_LOG_MODULES=timestamp,nsHttp:5,nsSocketTransport:5,nsHostResolver:5;proxy:5

I'm also going to make a custom nightly for you that disables 1 aspect of that patch that is about a week old. I'd like it if you could run that (with logging) too and let me know if identifies the problem.

I'll update the bug when the build has been made.

Comment 10

[Patrick McManus [:mcmanus]]

(In reply to Patrick McManus [:mcmanus] from comment #9)

> 
> I'll update the bug when the build has been made.

Frank - the windows build will be here - 

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9d71bc93be7e

(it will take an hour or three to appear there)

Thanks!

Comment 11

[Frank Walburg]

(In reply to Patrick McManus [:mcmanus] from comment #9)
> Frank.. I can't reproduce anything yet.. 
> 
> I'm going to ask 2 favors of you.. 1 can you get me an http log?
> 
> https://developer.mozilla.org/en-US/docs/HTTP_Logging
> 
> But this is IMPORTANT - I need you to include proxy information too - so
> setup the log like so (include the proxy bit):
> set
> NSPR_LOG_MODULES=timestamp,nsHttp:5,nsSocketTransport:5,nsHostResolver:5;
> proxy:5
> 


I sent the logfile generated with the above mentioned settings to Patrick.

Comment 12

[Frank Walburg]

(In reply to Patrick McManus [:mcmanus] from comment #10)
> (In reply to Patrick McManus [:mcmanus] from comment #9)
> 
> > 
> > I'll update the bug when the build has been made.
> 
> Frank - the windows build will be here - 
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mcmanus@ducksong.
> com-9d71bc93be7e
> 
> (it will take an hour or three to appear there)
> 
> Thanks!

I tested with this version. The performance problem does not exist.

Comment 13

[Patrick McManus [:mcmanus]]

Frank's log suggests the issue is with a 302 redirect of the system proxy pac file.

I think the issue is that we update the uri on the redirect and when poll the system proxy settings to see if there had been a change notice they don't match.

this isn't a problem when you use the firefox pac option (instead of system settings) because changes there are detected via events instead of polling.

Comment 14

[Patrick McManus [:mcmanus]]

Attachment: patch 0

815783 caused this problem by consolidating the two uris that had existed in nsPACMan.cpp (one as a nsIURI and one as a nsCString) into one string while getting rid of the nsIURI because of thread issues.

They really represented different things.. the string was the configured PAC uri (used to detect when the configuration changed) and the nsIURI was used to greenlight transfer of the PAC file without a proxy.

Normally these are the same uri, but when following redirect the configured one stays constant while the active-uri one was updated to allow chasing the redirected-to uri.

This patch breaks it back into two values and tries to make it more explicit what is going on.

Comment 15

[Patrick McManus [:mcmanus]]

Attachment: patch 1

update it for a minor performance tweak (don't convert nsiuris to strings for comparison when not using pac)

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 692719 [details] [diff] [review]
patch 1

Review of attachment 692719 [details] [diff] [review]:
-----------------------------------------------------------------

sorry for the delay!

::: netwerk/base/src/nsPACMan.cpp
@@ +658,5 @@
>    nsresult rv = NS_OK;
>    nsCOMPtr<nsIURI> pacURI;
>    if (NS_FAILED((rv = newChannel->GetURI(getter_AddRefs(pacURI)))))
>        return rv;
> +  

no trailing whitespace, please

@@ +668,5 @@
> +       mPACURISpec.get(), mPACURIRedirectSpec.get()));
> +
> +  // do not update mPACURISpec - that needs to stay as the
> +  // configured URI so that we can determine when the config changes.
> +  // However do track the most recent URI in the redirect chnage

chnage -> change

::: netwerk/base/src/nsPACMan.h
@@ +131,5 @@
> +   * URI it has been redirected to. In the case of a chain of redirections
> +   * only the current one being followed and the original are considered
> +   * becuase this information is used, respectively, to determine if we
> +   * should bypass the proxy (to fetch the pac file) or if the pac
> +   * configuration has changed (and we shouild reload the pac file)

shouild -> should

Comment 17

[Patrick McManus [:mcmanus]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/19296526f23d

Comment 18

[Patrick McManus [:mcmanus]]

Comment on attachment 692719 [details] [diff] [review]
patch 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 815783 .. broken on 19 should be fixed on 19
User impact if declined: proxy pac files retrieved via HTTP redirects will fail to work.. probably leaving the user without internet connectivity.
Testing completed (on m-c, etc.): on mc
Risk to taking this patch (and alternatives if risky): small patch with average risk
String or UUID changes made by this patch: none

Comment 19

[bhavana bajaj [:bajaj]]

(In reply to Patrick McManus [:mcmanus] from comment #18)
> Comment on attachment 692719 [details] [diff] [review]
> patch 1
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 815783 .. broken on 19 should be
> fixed on 19
> User impact if declined: proxy pac files retrieved via HTTP redirects will
> fail to work.. probably leaving the user without internet connectivity.
> Testing completed (on m-c, etc.): on mc
> Risk to taking this patch (and alternatives if risky): small patch with
> average risk
> String or UUID changes made by this patch: none

815783, does not seem to have landed on aurora yet.Do we still this on aurora ?

Comment 20

[Patrick McManus [:mcmanus]]

(In reply to bhavana bajaj [:bajaj] from comment #19)

> 815783, does not seem to have landed on aurora yet.Do we still this on
> aurora ?

the patch in question is on aurora-19.

https://bugzilla.mozilla.org/show_bug.cgi?id=815783#c35

Comment 21

[bhavana bajaj [:bajaj]]

Thanks for the confirmation, Guessing I was looking at a wrong bug ,when questioning that.Sorry about that!

Please perform any needed local-testing for this, to react to regressions/backout earlier as suggests comment# 19 its average risk & this area is not explicitly covered in our qa testing.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19296526f23d

Comment 23

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b339d18e2238

Comment 24

[Frank Walburg]

loading of web pages with proxy configures via PAC file using system settings now works fine with nightly 20.0a
Thank you for quickly resolving this issue