817533 - proxy system detection (environment) broken

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

Between Firefox (and TB) 17 and 18b2 the proxy detection broke for the case of autodetection.
I haven't fully debugged it yet but what happens is that
https://mxr.mozilla.org/mozilla-beta/source/toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp#494 can return NS_ERROR_FAILURE what it does in my usecase where there is no GConf,GSettings and nothing is set in environment variables.

Apparently https://mxr.mozilla.org/mozilla-beta/source/netwerk/base/src/nsProtocolProxyService.cpp#1462 was changed in a way that
https://mxr.mozilla.org/mozilla-beta/source/netwerk/base/src/nsProtocolProxyService.cpp#1552
is still reached and something happens within that evaluation.

When I compare with 17 I find:
https://mxr.mozilla.org/mozilla-release/source/netwerk/base/src/nsProtocolProxyService.cpp#1258

So in my use case if no system proxy was found we left Resolve_Internal via NS_OK immediately instead of proceeding to everything below.

This breaks Necko for me on systems not running with Gnome (no GConf/GSettings) and having default settings for using system proxy settings.

Comment 1

[Patrick McManus [:mcmanus]]

Wolfgang, thanks for the bug report and the diagnosis.

Can you be more clear on what the behavior was in ff17 and how that changed in 18? Thanks. (i.e. you got an error before and now it connects without a proxy? or vice versa?)

The code has changed substantially to improve jank, so we need to focus on behavior rather than lines of code. Thanks.

Comment 2

[Wolfgang Rosenauer [:wolfiR]]

(In reply to Patrick McManus [:mcmanus] from comment #1)
> Can you be more clear on what the behavior was in ff17 and how that changed
> in 18? Thanks. (i.e. you got an error before and now it connects without a
> proxy? or vice versa?)

Ok, sorry for being unclear ;-)

In my test case (and real usage case) I have no proxy at all. I do not have GConf/GSettings so that it falls back to check environment variables.
With FF17 that just worked with proxy.type=5 (the default). With Firefox 18 (actually I noticed the changed behaviour with Thunderbird 18b1) I'm not able to access any HTTP site at all. I only get the error page that my proxy server cannot be accessed.
If GConf is available and used (even if no proxy is set up) everything works fine -> no proxy used at all

Comment 3

[Patrick McManus [:mcmanus]]

Wolfgang, thanks - I still can't reproduce.

here's what I did.. I disable gconf and gsettings like this

 nsresult
 nsUnixSystemProxySettings::Init()
 {
   mSchemeProxySettings.Init(5);
+  return NS_OK;
+  
   mGConf = do_GetService(NS_GCONFSERVICE_CONTRACTID);
   mGSettings = do_GetService(NS_GSETTINGSSERVICE_CONTRACTID);

and then I set http_proxy environment var to a real proxy and loaded ipchicken.com to confirm the env var code was being used. And it was.

I then deleted the env var and reran firefox.

I confirmed that nsUnixSystemProxySettings::GetProxyForURI() now returned an NS_ERROR_FAILURE error as you described. But that didn't cause a problem - firefox connected without a proxy just fine - which is the behavior you say ff17 has (and sounds correct to me).

what are we doing differently?

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

Ok, some more details of testing I did.
First I was thinking it might be related to a patch I'm using in FF and TB which is that one
http://www.rosenauer.org/hg/mozilla/file/31f273919032/mozilla-nongnome-proxies.patch
This is used to make sure that GConf is not used when running under KDE or other non-Gnome Windowmanagers but it looked innocent to me.

To make sure there is nothing in my own compilation I reproduced like this:
(sorry, didn't try FF that way yet)
- install thunderbird 18b1 as provided by mozilla.org
- remove libmozgnome.so from components/binary.manifest
- rename libmozgnome.so to aaalibmozgnome.sobbb in components
By removing libmozgnome both GConf and GSettings is not available in Thunderbird.
This is basically the same what happens when that component cannot be loaded because of deps issues.
Starting up TB and I see immediately a "The proxy server is refusing connections" error.

I'll now try to reproduce with FF18b2 the same way.

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

Ok, the same steps fail for Firefox.
So actually I was thinking it might be a core Necko issue when it might be some TB specific usage of necko.
Now I do not have an idea what could cause that difference between Firefox and Thunderbird.

Comment 6

[Patrick McManus [:mcmanus]]

I want to figure out if this impacts a platform we support. That obviously excludes distributions where files get deleted or have custom patches :)

I'm leaning towards "not supported" right now - but I'd love to see the case where it is supported.

If the answer is "supported" then I want to hurry in a fix and get it backported to remove regressions.

If the answer is "not supported" it isn't necessarily WONTFIX but we certainly won't set the tracking flags, etc..

Comment 7

[Patrick McManus [:mcmanus]]

what are your proxy environment variables set to (if anything)?

Comment 8

[Wolfgang Rosenauer [:wolfiR]]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> I want to figure out if this impacts a platform we support. That obviously
> excludes distributions where files get deleted or have custom patches :)

(It's probably time to get that patch committed then since it makes a lot of sense for Linux users.)

What I did to reproduce the issue is removing the mozgnome component. This component is intentionally designed as an optional dependency. To my knowledge Gecko is still supported on systems w/o gconf or gsettings with a bit limited feature set.
I just was too lazy to remove my libgconf stuff from my system.
If Thunderbird is a "supported platform" I do not know nowadays. Neither I don't know why apparently only Thunderbird is affected (I'm going to try Seamonkey now).

(In reply to Patrick McManus [:mcmanus] from comment #7)
> what are your proxy environment variables set to (if anything)?

None.
wolfi@Hygiea:~> env | grep -i proxy
wolfi@Hygiea:~>

Comment 9

[Patrick McManus [:mcmanus]]

alex/lsblakk based on comments 4 5 and 6 I would suggest not tracking for 18.

Comment 10

[Wolfgang Rosenauer [:wolfiR]]

Just for completeness: Seamonkey is not affected apparently

Comment 11

[Karl Tomlinson (:karlt)]

We definitely support systems without GConf and gsettings-desktop-schemas (which is GNOME-specific AFAIK).
(I don't know whether or not that is equivalent, for purposes of this bug, to removing libmozgnome.so - libmozgnome should now load on all systems because there are no longer gnomevfs or libnotify dependencies, and GConf is accessed through dlopen.)

(In reply to Wolfgang Rosenauer [:wolfiR] from comment #2)
> With Firefox 18
> (actually I noticed the changed behaviour with Thunderbird 18b1) I'm not
> able to access any HTTP site at all. I only get the error page that my proxy
> server cannot be accessed.

(In reply to Wolfgang Rosenauer [:wolfiR] from comment #4)
> Starting up TB and I see immediately a "The proxy server is refusing
> connections" error.

(In reply to Wolfgang Rosenauer [:wolfiR] from comment #5)
> Ok, the same steps fail for Firefox.
> [...]
> Now I do not have an idea what could cause that difference between Firefox
> and Thunderbird.

What is the difference in behavior between Firefox and Thunderbird?

Comment 12

[Wolfgang Rosenauer [:wolfiR]]

(In reply to Karl Tomlinson (:karlt) from comment #11)
> We definitely support systems without GConf and gsettings-desktop-schemas
> (which is GNOME-specific AFAIK).
> (I don't know whether or not that is equivalent, for purposes of this bug,
> to removing libmozgnome.so - libmozgnome should now load on all systems
> because there are no longer gnomevfs or libnotify dependencies, and GConf is
> accessed through dlopen.)
> 
> (In reply to Wolfgang Rosenauer [:wolfiR] from comment #2)
> > With Firefox 18
> > (actually I noticed the changed behaviour with Thunderbird 18b1) I'm not
> > able to access any HTTP site at all. I only get the error page that my proxy
> > server cannot be accessed.
> 
> (In reply to Wolfgang Rosenauer [:wolfiR] from comment #4)
> > Starting up TB and I see immediately a "The proxy server is refusing
> > connections" error.
> 
> (In reply to Wolfgang Rosenauer [:wolfiR] from comment #5)
> > Ok, the same steps fail for Firefox.
> > [...]
> > Now I do not have an idea what could cause that difference between Firefox
> > and Thunderbird.
> 
> What is the difference in behavior between Firefox and Thunderbird?

If that question was to me:
I screwed up. I never was able to reproduce with Firefox but when I opened the bugreport and the behaviour looked very much like Gecko/Necko itself I wrote initially about Firefox instead of Thunderbird while I still was trying to reproduce.

To make it clear:
I cannot observe any erratic behaviour with Firefox 18b2 but I can reproduce with Thunderbird 18b1 easily. Sorry for the confusion but I really hadn't expected a difference there.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Patrick McManus [:mcmanus] from comment #9)
> alex/lsblakk based on comments 4 5 and 6 I would suggest not tracking for 18.

Thanks Patrick, not tracking.

Comment 14

[Patrick McManus [:mcmanus]]

I bet bug 713802 plays a role here.

Comment 15

[Wolfgang Rosenauer [:wolfiR]]

While that's indeed possible I still do not understand the difference between TB and FF.
I build both with
ac_add_options --disable-gnomevfs
ac_add_options --enable-gio
And I guess that the mozilla.org build options for FF18 and TB18 are the same as well?

Comment 17

[Patrick McManus [:mcmanus]]

Attachment: patch 0

The issue here is that failed system proxy lookups fallback to looking at the manual configs even if we aren't in manual config mode. So manual specific prefs that might be set result in this bug even though we aren't in manual config mode. The fallback should be to direct.

Comment 18

[Patrick McManus [:mcmanus]]

** if m-i is OPEN then do HAPPY DANCE!

   https://hg.mozilla.org/integration/mozilla-inbound/rev/85f1d207f525

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85f1d207f525

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Works fantastic with Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20130412 Firefox/23.0 ID:20130412030828 CSet: 7b8ed29c6bc0

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Patrick, can we get a test for it?

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is a regression started with the landing of the patch on bug 824341 for Firefox 22. Can we please get this backported and fixed in Aurora?

(In reply to Henrik Skupin (:whimboo) from comment #21)
> Patrick, can we get a test for it?

This question still stands. Can we get an automated test for this issue?

Comment 24

[Patrick McManus [:mcmanus]]

> This question still stands. Can we get an automated test for this issue?

patches welcome

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Patrick McManus [:mcmanus] from comment #24)
> > This question still stands. Can we get an automated test for this issue?
> 
> patches welcome

Usually we should provide tests for regressions like those. As far as I have seen there were a couple in the proxy related code lately.

So would this be doable as a browser chrome test?

Comment 26

[Alex Keybl [:akeybl]]

This was originally filed against FF18 - how is this a regression from 824341 in FF22?

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Alex Keybl [:akeybl] from comment #26)
> This was originally filed against FF18 - how is this a regression from
> 824341 in FF22?

All the bugs we have originally filed against Firefox 22 (see e.g. bug 858854) were duped against this bug. So it seems like a change in Firefox 22 made this problem to widely appear. But Patrick might be able to give more details.

Comment 28

[Patrick McManus [:mcmanus]]

(In reply to Henrik Skupin (:whimboo) from comment #27)

> 
> All the bugs we have originally filed against Firefox 22 (see e.g. bug
> 858854) were duped against this bug. So it seems like a change in Firefox 22
> made this problem to widely appear.

Right, a change in the linux system settings proxy implementation uncovered this bug because it used this interface. In addition to using that system settings implementation the user needed some unused stale non-default prefs too. (well, they should have been unused - that was the bug).

The patch here is system independent, but I'm not aware of the issue effecting anything except desktop linux with a dogeared config.

Henrik, you can nom the patch for aurora if you think that's the right thing to do. Do the tracking dance after there is a merged patch is kind of redundant. Either we'll take it or we won't - there isn't much to track at that point.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Patrick McManus [:mcmanus] from comment #28)
> Henrik, you can nom the patch for aurora if you think that's the right thing
> to do. Do the tracking dance after there is a merged patch is kind of
> redundant. Either we'll take it or we won't - there isn't much to track at
> that point.

Patrick, I would propose that you request for it given that you wrote the patch and know each and every detail to write up the right assessment for the flag.

Also please let me know about a framework we could use for the test.

Comment 30

[Alex Keybl [:akeybl]]

(In reply to Henrik Skupin (:whimboo) from comment #29)
> (In reply to Patrick McManus [:mcmanus] from comment #28)
> > Henrik, you can nom the patch for aurora if you think that's the right thing
> > to do. Do the tracking dance after there is a merged patch is kind of
> > redundant. Either we'll take it or we won't - there isn't much to track at
> > that point.
> 
> Patrick, I would propose that you request for it given that you wrote the
> patch and know each and every detail to write up the right assessment for
> the flag.

Patch nominations should definitely come from the bug assignee. If this risk outweighs the corner case described in https://bugzilla.mozilla.org/show_bug.cgi?id=849792#c29, we shouldn't take a fix at all.

Comment 31

[Patrick McManus [:mcmanus]]

Comment on attachment 735214 [details] [diff] [review]
patch 0

[Approval Request Comment] (firefox 22 aurora)
Bug caused by (feature/regressing bug #): 824341

User impact if declined: Linux desktop users who once had configured a manual proxy but are no longer using that configuration may still use the old configuration (probably pointing at a non working proxy) depending on the details of their new proxy settings. (e.g. if their new setting is "use system settings" and the system settings returns an err).

Testing completed (on m-c, etc.): verified on m-c

Risk to taking this patch (and alternatives if risky): 

the patch for this is short and low risk. Its downside is that it touches code that runs on all platforms and all transactions in order to make a fix for a very small population.

an alternative would be to backout 824341 (a linux specific change) from aurora-22. The bug fixed by this patch would remain latent, but without 824341 we don't have any reports of it affecting anything.

String or IDL/UUID changes made by this patch: none.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Patrick, the needinfo flag is still valid given that I wait for a response regarding a possible test framework from you. See comment 25.

Comment 33

[Alex Keybl [:akeybl]]

(In reply to Patrick McManus [:mcmanus] from comment #31)
> User impact if declined: Linux desktop users who once had configured a
> manual proxy but are no longer using that configuration may still use the
> old configuration (probably pointing at a non working proxy) depending on
> the details of their new proxy settings. (e.g. if their new setting is "use
> system settings" and the system settings returns an err).

If this is truly the only affected population and there's no reason to believe there's a FF22 regression, then we won't take the patch. Thanks Patrick.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Patrick, any feedback regarding my question for a test? Thanks.

Comment 36

[Gary Houston]

This is a nasty bug, since Firefox just fails without any error message after upgrading to version 22. I was affected myself and it took a while to diagnose. One thing puzzles me: why does google.com still work when a broken manual proxy is configured?

Comment 37

[Patrick McManus [:mcmanus]]

(In reply to Gary Houston from comment #36)
> One thing puzzles me: why does google.com still work when a broken
> manual proxy is configured?

probly using ssl and that either takes a slightly different code path or your old stale proxy config didn't cover that.

Comment 38

[Gary Houston]

Yeah, ssl, and my proxy was http only. Thanks.

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Patrick, need-info is still valid. You didn't answer my question from comment 35. I hope you will do it at some point! Thanks.

Comment 40

[Mamoru TASAKA]

Step to reproduce:

* Launch with firefox -ProfileManager , create new profile, and launch
  firefox with that (new) profile
* Go Preferences -> Advanced -> Network -> Connection -> Settings
* Once choose "Manual proxy configuration", set random http proxy, change port
  (like HTTP Proxy: aaaaaa Port: 8080).
* Then choose *again* "Use system proxy settings"
* Click OK
* Now open some http:// page like http://www.google.com/

Comment 43

[Mamoru TASAKA]

(In reply to Mamoru TASAKA from comment #40)
> Step to reproduce:
> 
> * Launch with firefox -ProfileManager , create new profile, and launch
>   firefox with that (new) profile
> * Go Preferences -> Advanced -> Network -> Connection -> Settings
> * Once choose "Manual proxy configuration", set random http proxy, change
> port
>   (like HTTP Proxy: aaaaaa Port: 8080).
> * Then choose *again* "Use system proxy settings"
> * Click OK
> * Now open some http:// page like http://www.google.com/

on Linux (Fedora 19 i686)

Comment 45

[Patrick McManus [:mcmanus]]

(In reply to Henrik Skupin (:whimboo) from comment #25)
>
> So would this be doable as a browser chrome test?

you can probably write an xpcshell test that sets some http proxy address along with a PROXYCONFIG_MANUAL setting and then test the results of a nsIProtocolProxyService.asyncResolve invocation to make sure the pi.type is not http.