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.

BTW, my SOCKS 5 proxy is an SSH tunnel.

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.

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

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.

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

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

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.

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.

(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!

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).

I'll look at it in the morning.

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.

troycauble - can you verify a fix using http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mcmanus@ducksong.com-070f87c5f537/try-macosx64/

(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)

(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.

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

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.

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

(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.

troycauble, can you give http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/mcmanus@ducksong.com-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.

troycauble : comment 21

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 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"

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

> can you give > http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/ > mcmanus@ducksong.com-5aa33a614843/try-macosx64/ a try? Works for me.

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

(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?

(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..)

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 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!

(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.

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

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

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.

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

(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 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).

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

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 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 on attachment 693902 [details] [diff] [review] /workaround for 18 v1 [Approval Request Comment] see comments 36, 37, 38

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.

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

(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)?

(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.)

(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.

(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.