586908 - Infinite requests when proxy from system settings is redirected

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[John P Baker]

Our IE PAC file used to be served from http://www.bristol.ac.uk/xyz.pac (name changed).
Several years ago it was changed to http://wwwcache.bris.ac.uk/cgi-bin/proxy.pl and a redirect put in place from the previous URL.
As we update Firefox, in the summer break, to a version defaulting to 'use system settings' for the proxy configuration, we have found some that have the old URL configured and, when Firefox is started, they will continually try to fetch the old PAC script (one last week 2 million times over a 7 hour period, one this week with 4.5 million requests) rather than use the new one.

Comment 1

[Bjarne (:bjarne)]

Do I understand you correctly: If I point FF to a URL which redirects to a PAC-file, then it loops forever? Version of FF?

Comment 2

[John P Baker]

(In reply to comment #1)
> Do I understand you correctly: If I point FF to a URL which redirects to a
> PAC-file, then it loops forever? Version of FF?

Not quite; If you set your system setting to a PAC URL which redirects and set Firefox to use system setting then it loops forever. The PC systems team replicated on 3.6.0 and 3.6.8 - we haven't tried trunk yet; IIRC the _default_ setting was switched around 3.6.4 release.

Comment 3

[Bjarne (:bjarne)]

Reproduced. Nominating for blocking.

Comment 4

[Bjarne (:bjarne)]

Attachment: Fix to avoid this problem

The problem here was that the redirected channel was trying to resolve its proxy, picked up the uri from system settings, and thus got itself into an endless loop.

nsPACMan::AsyncOnChannelRedirect() makes sure to update |nsPACMan::mPACURI| appropriately, but the code in nsProtocolProxyService::Resolve_Internal() picks up the original uri from system settings and overwrites it.

Comment 5

[Bjarne (:bjarne)]

Adding timeless to CC-list since he did the work for bug #529773 and may have input here.

Comment 6

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

Comment on attachment 476156 [details] [diff] [review]
Fix to avoid this problem

r=biesi but please add a test

Comment 7

[Daniel Veditz [:dveditz]]

Not blocking 1.9.2.x but we'll approve a patch with a test if you set the appropriate approval flag.

Comment 8

[Bjarne (:bjarne)]

Attachment: Slightly modified version w/ testcase

Changed behaviour to return NS_OK immediately if the loop was discovered. Also added testcase for this particular problem.

Comment 9

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

Comment on attachment 476368 [details] [diff] [review]
Slightly modified version w/ testcase

+++ b/netwerk/base/src/nsProtocolProxyService.cpp	Fri Sep 17 22:37:44 2010 +0200
+        if (mPACMan && mPACMan->IsPACURI(uri)) return NS_OK;

Please put the return on its own line, that makes debugging simpler (when stepping through or when setting a breakpoint)

+// Ensure we're using system-properties

Please indent comments to the same level as the code

+  if (failed) retval = failed;

Should that be "failed" instead of failed?

Comment 10

[Bjarne (:bjarne)]

Attachment: Addressed comments from reviewer

Fixed up the last issues, carrying over the r+

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 476379 [details] [diff] [review]
Addressed comments from reviewer

Approved for 1.9.2.11, a=dveditz for release-drivers

Comment 12

[Bjarne (:bjarne)]

Seems like I forgot to request check-in...  done. I believe the patch also applies to 1.9.2.

Comment 13

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/97346e7d9180

Comment 14

[Daniel Veditz [:dveditz]]

backed out on 1.9.2 due to failure in the new test added with this patch
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ea16213ee93e
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1285745534.1285745969.9046.gz

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 476379 [details] [diff] [review]
Addressed comments from reviewer

We'll need a new 1.9.2 patch that either fixes the test or fixes whatever bug in the backport the test is catching.

Comment 16

[Bjarne (:bjarne)]

I'll take a look. It didn't land on trunk yet?

Comment 17

[Bjarne (:bjarne)]

Attachment: Patch for 1.9.2

The constant nsIProtocolProxyService::PROXYCONFIG_SYSTEM used by the test is undefined in 1.9.2. Test defaulted to DIRECT and failed in the cross-check to verify that redirection actually took place. Should be ok now.

(Carrying over the r+ since this is just a simple change in the test.)

Comment 18

[Bjarne (:bjarne)]

Attachment: Added missing keyword "function" in PAC file...

Fixed syntax-error in PAC-script which seems to have been forgiven by trunk, but just for completeness.  Carrying over the r+.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 479740 [details] [diff] [review]
Patch for 1.9.2

too late for 1.9.2.11, moving to next release.

Comment 20

[Daniel Veditz [:dveditz]]

With the tree in its current phase you'll probably need to actively look for a check-in buddy on IRC (or mail). The keyword alone might not get it into Firefox 4.

Comment 21

[timeless]

sorry, i was traveling on vacation for a month. but it looks like things are mostly in hand, minus the excitement of landing window hunts

Comment 22

[LegNeato]

Comment on attachment 479740 [details] [diff] [review]
Patch for 1.9.2

a=LegNeato for 1.9.2.12. Please land only on the mozilla-1.9.2 default branch, *not* the relbranch.

Comment 23

[Bjarne (:bjarne)]

Anything I need to do to move this one forward...?

Comment 24

[John P Baker]

It looks as if this was landed on trunk yesterday

http://hg.mozilla.org/mozilla-central/rev/290ad164b656

Comment 25

[Bjarne (:bjarne)]

This still needs landing on 1.9.2 ref comment #22. Should it be marked "fixed" anyway?

Comment 26

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b3d441310e5c