815783 - PAC file://localhost/User... urls in from system proxy configuration broken

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[troy]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:18.0) Gecko/18.0 Firefox/18.0
Build ID: 20121121075611

Steps to reproduce:

I'm on 18.0 on the beta update channel on OSX.
I use "Use System Proxy Settings" in Firefox with my OSX location proxy set to a local PAC file mapping to a SOCKS5 proxy.  This has worked fine for months.  It also works fine with chrome and safari.

Today I did a Firefox update.


Actual results:

The firefox upgrade hung checking for Add-on compatibility.  After a force quit, no pages would load. Safe-mode was the same. A safe-mode 'reset' (which wiped my add-ons and who knows what else  ](*,) ) was the same.  Then I tried changing my system proxy to a non-SOCKS PAC file and it works.

I've toggled my system proxy a few times and it's consistent.


Expected results:

Should continue to support an OSX system proxy that is a local PAC file that returns SOCKS5 proxies.

Comment 1

[troy]

BTW, my SOCKS 5 proxy is an SSH tunnel.

Comment 2

[Matthias Versen [:Matti]]

What happens if you setup the pac file directly in Firefox ?
It's the question if loading the pac file via system settings broke and or the pac file itself doesn't work anymore.

Comment 3

[Marcia Knous [:marcia]]

Adding Juan in case there were some changes made in 18 to networking.

Comment 4

[Patrick McManus [:mcmanus]]

troycauble, can you provide a (simplified?) PAC file that reproduces your issue. Maybe just test that returning a socks5 proxy all the time makes it happen for you and then include that file?

Thanks.

Comment 5

[troy]

Good idea, Matthias.  It does work to load the local PAC file directly.

Comment 6

[troy]

Attachment: Simple PAC file returning SOCKS5.

Attaching a simple PAC file that returns SOCKS5.  Works loaded directly into Firefox.  Does not work via System Proxy indirection.

Comment 7

[Patrick McManus [:mcmanus]]

but if you return something other than SOCKS it works? That's weird :)

I say that because of "when I tried changing my system proxy to a non-SOCKS PAC file and it works."

If that's true I'll look into it tomorrow. I don't have OSX 10.8, but hopefully any osx will do.

Comment 8

[troy]

Found it!  It seems to have to do with the form of local file URLs.

My OSX system preferences had
    file://localhost/Users/troy/tmp/test.pac

That's what's generated by the OSX finder dialog.

I noticed that when I paste that into Firefox's preferences, it gets rewritten as
    file:///Users/troy/tmp/test.pac
(That's what I see when I open the dialog again.)

I changed the System Preferences to the file:/// form and Firefox works with it.
So firefox needs it that way.

This is probably not related to SOCKS at all, just local PAC files.

I suggest whatever reads the System Pref Auto Proxy Url
rewrite the url like the Firefox pref dialog does.  Or the
file://localhost/ form could be handled generally.

Comment 9

[troy]

(In reply to Patrick McManus [:mcmanus] from comment #7)
> but if you return something other than SOCKS it works? That's weird :)
> 
> I say that because of "when I tried changing my system proxy to a non-SOCKS
> PAC file and it works."

My non-SOCKS PAC file was via an http URL!

Comment 10

[Matthias Versen [:Matti]]

I tested using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/17.0 Firefox/17.0

I set a local pac file in OSX via filepicker.
file://localhost/Users/matti/test.pac
The Firefox setting is to use the system proxy configuration

Firefox used the pac file (Proxy refused connection).

Comment 11

[Patrick McManus [:mcmanus]]

I'll look at it in the morning.

Comment 12

[Patrick McManus [:mcmanus]]

Ah. The issue is that nsIURI normalizes file://localhost/U into file:///U (you saw this when you pasted the localhost version directly into the firefox prefs).

But the system proxy setting is checked at runtime to determine if there has been a change.. it works with strings and not uris because it is on the pac thread and nsIURI is not safe off the main thread. So a string compare is done instead and it is compared to a version that has been through the nsIURI normalization.. the result is that the pac thread thinks the system pac uri is constantly changing.

Versions < 18 did not have a separate pac thread so used the uri comparisons directly.

It should be easy enough to make sure that comparison gets done against the original value returned from the system proxy setting instead of the normalized version.

Comment 13

[Patrick McManus [:mcmanus]]

troycauble - can you verify a fix using http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-070f87c5f537/try-macosx64/

Comment 14

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 15

[troy]

(In reply to Patrick McManus [:mcmanus] from comment #13)
> troycauble - can you verify a fix using
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mcmanus@ducksong.
> com-070f87c5f537/try-macosx64/

firefox-20.0a1.en-US.mac.dmg from that directory did NOT work with file://localhost/U...
and did with file:///U...
(OSX 10.8.2)

Comment 16

[Patrick McManus [:mcmanus]]

(In reply to troycauble from comment #15)
> (In reply to Patrick McManus [:mcmanus] from comment #13)
> > troycauble - can you verify a fix using
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mcmanus@ducksong.
> > com-070f87c5f537/try-macosx64/
> 
> firefox-20.0a1.en-US.mac.dmg from that directory did NOT work with
> file://localhost/U...
> and did with file:///U...
> (OSX 10.8.2)


specifically as the system PAC file?

cause that exact thing worked fine for me.

Comment 17

[Patrick McManus [:mcmanus]]

can you run it from the command line and paste in the console output?

Comment 18

[Patrick McManus [:mcmanus]]

oh and be sure you're really running the test build by shutting down any other firefox and checking the about firefox information (should report version 20). Apologies if this is obvious, but its easy to end up with multiple windows of the old browser even if you invoke the new applications.

Comment 19

[troy]

Yes I was working with the system proxy and the right browser.  New results:

The provided nightly build doesn't like CHANGES in the system proxy settings.
I can start it with either local URL form successfully, but when I change the system proxy for testing,
pages fail to load and the command line output speeds up (spin?).

In all cases (working or not), output similar to that below repeated.  But it spewed much faster after
a change to the system proxy and the two URLs on the INFO line were different (as shown below).

objc[89260]: Object 0x143098270 of class __NSCFData autoreleased with no pool in place - just leaking - break on objc_autoreleaseNoPool() to debug
objc[89260]: Object 0x143098430 of class __NSCFData autoreleased with no pool in place - just leaking - break on objc_autoreleaseNoPool() to debug
INFO 0 0 file://localhost/Users/troy/tmp/tunnel.pac file:///Users/troy/tmp/tunnel.pac
objc[89260]: Object 0x1430982e0 of class __NSCFData autoreleased with no pool in place - just leaking - break on objc_autoreleaseNoPool() to debug
objc[89260]: Object 0x143098510 of class __NSCFData autoreleased with no pool in place - just leaking - break on objc_autoreleaseNoPool() to debug
INFO 0 0 file://localhost/Users/troy/tmp/tunnel.pac file:///Users/troy/tmp/tunnel.pac

Comment 20

[Patrick McManus [:mcmanus]]

(In reply to troycauble from comment #19)
> Yes I was working with the system proxy and the right browser.  New results:
> 
> The provided nightly build doesn't like CHANGES in the system proxy settings.

ok. thanks.

Comment 21

[Patrick McManus [:mcmanus]]

troycauble,

can you give http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-5aa33a614843/try-macosx64/ a try?

I apologize that this one is only very briefly tested but I wanted to post the link before I went offline for a bit.

Comment 22

[Patrick McManus [:mcmanus]]

troycauble : comment 21

Comment 23

[Patrick McManus [:mcmanus]]

Attachment: patch 1 [landed on 19, 20]

The core issue here is we were evaluating the pacURI sometimes as an nsURI (normalized) and sometimes as a string (directly from the system config). That was driven by the nsIURI requiring the main thread.

I think we're ok just using the string version consistently, which is what this patch does.

Comment 24

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

Comment on attachment 687826 [details] [diff] [review]
patch 1 [landed on 19, 20]

+      nsCOMPtr<nsIURI> pacURI;
+      NS_NewURI(getter_AddRefs(pacURI), mPACURISpec);

perhaps add some sort of logging if this fails?

   nsCString                    mPACURISpec; // for use off main thread

perhaps clarify that comment - "Not an nsIURI for use off main thread"

Comment 25

[Patrick McManus [:mcmanus]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7b79667bc3

Comment 26

[troy]

> can you give
> http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/
> [e-mail]-5aa33a614843/try-macosx64/ a try?

Works for me.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd7b79667bc3

Comment 28

[Patrick McManus [:mcmanus]]

Comment on attachment 687826 [details] [diff] [review]
patch 1 [landed on 19, 20]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 769764
User impact if declined: system proxy pac files on the local disk using localhost syntax will make firefox non operational. If you specify a system wide PAC file on OS X the file picker writes it out using this syntax.
Testing completed (on m-c, etc.): on m-c for a week
Risk to taking this patch (and alternatives if risky): Its not risk free - the change impacts all platforms using system proxy configurations as it is in a generic place. But it has been problem free for a week and is not very complicated. Backing out 769764 which is a headline feature isn't a good idea.. wontfixing it on 18 is a possibility but the failure mode for an existing (if unusual) configuration is pretty awful. (firefox just won't work. Safe mode won't even help you.. you would have to know to clear you system proxy settings.) I think the best path is to take the patch.
String or UUID changes made by this patch:  none

Comment 29

[Alex Keybl [:akeybl]]

(In reply to Patrick McManus [:mcmanus] from comment #28)
> Risk to taking this patch (and alternatives if risky): Its not risk free -
> the change impacts all platforms using system proxy configurations as it is
> in a generic place. But it has been problem free for a week and is not very
> complicated. Backing out 769764 which is a headline feature isn't a good
> idea.. wontfixing it on 18 is a possibility but the failure mode for an
> existing (if unusual) configuration is pretty awful. (firefox just won't
> work. Safe mode won't even help you.. you would have to know to clear you
> system proxy settings.) I think the best path is to take the patch.

Is there a lower risk solution that we can use to mitigate rather than completely fix this issue? I'd hate to regress >.001% of users for .001% of users.

If we end up taking this fix, can we limit its impact to OS X?

Comment 30

[Patrick McManus [:mcmanus]]

(In reply to Alex Keybl [:akeybl] from comment #29)

> Is there a lower risk solution that we can use to mitigate rather than
> completely fix this issue? I'd hate to regress >.001% of users for .001% of
> users.

I racked my brain for something - but what is there is probably best in that way. The good news is that the code will be run by a lot of windows users as soon as it is promoted to a busier channel.
If that proved problematic, as a contingency, I would say we would wontfix this on 18 - the failure mode is indeed bad but the number of users should be very small.

> If we end up taking this fix, can we limit its impact to OS X?

not effectively, it would require two parallel sets of code in a number of different callsites.. that patch is as scary as this one :) (which isn't really that scary.).. and this is a cross platform bug - its just that I don't know of anything other than the OS X filepicker that generates the problematic URL scheme (but I didn't know about the filepicker until this bug was filed..)

Comment 31

[troy]

Maybe this would be a good time to look at

Bug 134105 - SOCKS5: DNS lookups (host resolving) should occur on proxy, not client side. 
Bug 536093 - DNS leakage 

:)

Comment 32

[Alex Keybl [:akeybl]]

Comment on attachment 687826 [details] [diff] [review]
patch 1 [landed on 19, 20]

Since we believe this code will be exercised heavily and the user impact for those affected are great, let's land on branches. Please land asap today. Thanks Patrick!

Comment 33

[Patrick McManus [:mcmanus]]

(RyanVM please note :))

I'm not going to land this quite yet on beta/aurora because 819902 just got new comments moments ago that I want to look into (it might be a report about this patch on moz-central.) first.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

(In reply to Patrick McManus [:mcmanus] from comment #33)
> (RyanVM please note :))

Noted :)

Comment 35

[Patrick McManus [:mcmanus]]

819902 is WFM but with an active reporter.. no other signs of trouble so I'm going to promote this to aurora to broaden the audience and increase confidence while I work that.

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

Comment 36

[Patrick McManus [:mcmanus]]

bug 819902 turns out to be a small, but legit problem, with this patch dealing with redirects.

Given the impending holiday season, I know testing and opportunities for action on beta 18 are going to be reduced and we should therefore be more conservative. So I've spent a bunch of time figuring out how we can mitigate this problem (815783) on 18 without porting the existing patch plus the pacth for redirects (819902) to the beta branch. Backing out the original feature would be incredibly difficult both because of the high value of the feature in terms of jank reduction and the fact that it changes a bunch of interfaces that addons have been dutifully updating to.

I suggest we take 815783 and 819902 for m-c-20 and aurora-19 (when the patch is reviewed); but we apply to beta a workaround I've been able to come up with for this file://localhost/ issue.

The workaround addresses the reported problem (I've made and OS build and verified) but is specific to that - it just normalizes file://localhost/foo into file:///foo on the string level. It is very small and very safe and fixes the bug as reported - but it remains possible that there is some other incantation of URL that would have the same problem. (that would be fixed by the riskier 815783 819902 combo). Nonetheless at this point the existing code has had quite a bit of testing exposure and file://localhost/ is the only thing that has come up - so I've come to believe that is the right thing to do.

Comment 37

[Patrick McManus [:mcmanus]]

Attachment: workaround for 18 v0

I'm suggesting this just a precision workaround for gecko 18 because of the risk/reward issues involved and the current timeline.

Comment 38

[Alex Keybl [:akeybl]]

(In reply to Patrick McManus [:mcmanus] from comment #37)
> Created attachment 692772 [details] [diff] [review]
> workaround for 18 v0
> 
> I'm suggesting this just a precision workaround for gecko 18 because of the
> risk/reward issues involved and the current timeline.

Is there any way that we can expedite this review or reassign the review request as necessary? This is going to miss beta 5 (going to build today), so we'll really need to weigh risk/reward and ensure proper testing before approving for beta again.

Comment 39

[Patrick McManus [:mcmanus]]

Comment on attachment 692772 [details] [diff] [review]
workaround for 18 v0

jason can you get this trivial review in asap - see comment 38 we'd like to get this into beta (and only 18).

Comment 41

[Patrick McManus [:mcmanus]]

file://C:/proxy.pac also gets normalized to file:///proxy.pac

Comment 42

[Patrick McManus [:mcmanus]]

Attachment: /workaround for 18 v1

This just adds file://C:/foo to the list of things to fixup in the workaround for 18. It doesn't really add risk to the patch. I believe the "real" patches on 20/19 deal with this fine.

Comment 43

[Honza Bambas (:mayhemer)]

Comment on attachment 693902 [details] [diff] [review]
/workaround for 18 v1

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

r=honzab (stolen on Patrick's request)

Comment 44

[Patrick McManus [:mcmanus]]

Comment on attachment 693902 [details] [diff] [review]
/workaround for 18 v1

[Approval Request Comment]
see comments 36, 37, 38

Comment 45

[Alex Keybl [:akeybl]]

Comment on attachment 693902 [details] [diff] [review]
/workaround for 18 v1

While the affected population will be small, the affected population will be greatly impacted (comment 0). Please land no later than 12/26 to make Firefox 18.

QA also doesn't have much experience regression testing proxy issues of this nature. Please make sure to perform your own local testing, around any possible areas of regression.

Comment 46

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/4d94221c82df

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4d94221c82df

Comment 48

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

(In reply to Patrick McManus [:mcmanus] from comment #41)
> file://C:/proxy.pac also gets normalized to file:///proxy.pac

wait, what? why is that correct (on windows)?

Comment 49

[Masatoshi Kimura [:emk]]

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #48)
> (In reply to Patrick McManus [:mcmanus] from comment #41)
> > file://C:/proxy.pac also gets normalized to file:///proxy.pac
> 
> wait, what? why is that correct (on windows)?

Obviously incorrect. file://C:/proxy.pac should be normalized to file:///C:/proxy.pac (Be care with the slash count.)

Comment 50

[Patrick McManus [:mcmanus]]

(In reply to Masatoshi Kimura [:emk] from comment #49)
> (In reply to Christian :Biesinger (don't email me, ping me on IRC) from
> comment #48)
> > (In reply to Patrick McManus [:mcmanus] from comment #41)
> > > file://C:/proxy.pac also gets normalized to file:///proxy.pac
> > 
> > wait, what? why is that correct (on windows)?
> 
> Obviously incorrect. file://C:/proxy.pac should be normalized to
> file:///C:/proxy.pac (Be care with the slash count.)

I'm on it.

I just repeated the test I had originally done to see what the normalization was and this time it matches what you say (which is what makes sense, I agree). I _swear_ the same test gave the other result the last time which is why I used it. I'll see if I can figure out the discrepancy but maybe I just screwed it up somehow.

either way we'll update it.

Comment 51

[Patrick McManus [:mcmanus]]

(In reply to Patrick McManus [:mcmanus] from comment #50)
> 
> I'm on it.
> 

I looked into this but forgot to update the bug. sorry about that.

The situation on 18, given that it has had its last beta, is ok to live with even with the bogus normalization. 19 gets rid of the bogus code - so there isn't much to do.

The fact that a normalization is introduced stops the loop caused by "new uri, load it, new uri, load it, " etc.. in this case it reduces it to two steps "original uri, load it (ok), normalized uri, load it (fail - fall back to old data), unchanged uri stop loop". So the real damage here would be in requiring a restart to pickup a rare change in system settings between 2 double slash variants. Not a big deal given how rarely that would change.

Relatedly I did find one case where a bogus uri still does cause the original loop to happen.. simply "C:\proxy.pac". That's unfortunate, but its basically the risk we assented to in comment 36. That string does not work in either Firefox 17 or in IE (it needs to be a file URL) though the failure mode in 18 is clearly worse (its already fixed in 19). Given that nobody has reported it in all the betas and the fact that it isn't a working configuration anyhow I don't expect it to be a significant problem.