Last Comment Bug 517655 - use libproxy to get system proxies
: use libproxy to get system proxies
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: ---
Assigned To: Wolfgang Rosenauer [:wolfiR]
:
Mentors:
: 528700 (view as bug list)
Depends on: 621226
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-18 23:32 PDT by Wolfgang Rosenauer [:wolfiR]
Modified: 2011-01-03 14:56 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch #1 (9.70 KB, patch)
2009-10-01 14:33 PDT, Wolfgang Rosenauer [:wolfiR]
doug.turner: review-
Details | Diff | Splinter Review
patch (build changes) (4.30 KB, patch)
2010-07-28 03:49 PDT, Wolfgang Rosenauer [:wolfiR]
khuey: review+
Details | Diff | Splinter Review
patch (5.63 KB, patch)
2010-07-28 03:51 PDT, Wolfgang Rosenauer [:wolfiR]
doug.turner: review-
Details | Diff | Splinter Review
patch #2 (5.74 KB, patch)
2010-08-01 23:47 PDT, Wolfgang Rosenauer [:wolfiR]
doug.turner: review+
Details | Diff | Splinter Review

Description Wolfgang Rosenauer [:wolfiR] 2009-09-18 23:32:36 PDT
The current implementation to use system settings for proxies on Linux is basically relying on Gnome and gconf (there is a fallback to env variables) but Firefox should and can still run on other desktop environments (KDE, Xfce, LXDE...).
libproxy (http://code.google.com/p/libproxy/) could improve that as it provides the desktop specific support through a stable and simple API.
Apparently Gnome itself is using that from 2.18? on. At least openSUSE and Fedora already ship or will ship it with their next versions. So this is about "replacing" the current unixproxy implementation with one using libproxy and new external dependency. Replacing means that the current implementation should at this moment still be available and also be the default but preparing a new optional compile time environment.
I might be able to come up with the patch if time allows.
Comment 1 Jason Duell [:jduell] (needinfo me) 2009-09-22 10:04:38 PDT
Wolfgang,

This looks like a great idea.  We'd love to see a patch from you, as I'm not sure when the rest of us will have the time.
Comment 2 Wolfgang Rosenauer [:wolfiR] 2009-10-01 14:33:06 PDT
Created attachment 404122 [details] [diff] [review]
patch #1

This is a first version of a possible patch.
It's likely not ready for review but for collecting general feedback.

The patch contains a drop in replacement for the current nsUnixSystemProxySettings class and uses the very same class name and XPCOM identifiers. Not sure if that's ok in this case or if it should be a completely separate class and xpcom module. I've tested a bit already but cannot easily test advanced proxy environments.
Comment 3 Wolfgang Rosenauer [:wolfiR] 2009-11-09 23:01:03 PST
Is there any feedback on the patch?

There is (at least) one issue as the above wouldn't work for proxy strings with auth information I think.
Comment 4 Wolfgang Rosenauer [:wolfiR] 2010-04-07 11:55:03 PDT
development builds of Firefox for openSUSE have that patch for quite some time now. There is one bug reported about it:
https://bugzilla.novell.com/show_bug.cgi?id=592165
Comment 5 Doug Turner (:dougt) 2010-07-26 12:03:00 PDT
Comment on attachment 404122 [details] [diff] [review]
patch #1


>+ * The Initial Developer of the Original Code is
>+ * Wolfgang Rosenauer <wr@rosenauer.org>.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.

2010



>+nsresult
>+nsUnixSystemProxySettings::GetProxyForURI(nsIURI* aURI, nsACString& aResult)
>+{
>+  nsresult rv;
>+
>+  if (!mProxyFactory) {
>+    mProxyFactory = px_proxy_factory_new();
>+  }
>+  NS_ENSURE_TRUE(mProxyFactory, NS_ERROR_OUT_OF_MEMORY);

return NS_ERROR_NOT_AVAILABLE for all failure return codes.

>+  if (!mIOService) {
>+    mIOService = do_GetIOService();
>+  }
>+  NS_ENSURE_TRUE(mIOService, NS_ERROR_FAILURE);

don't cache mIOService.  Use nsCOMPtr<nsIIOService> ios = do_GetIOService(&rv);

>+  // PAC string accepts
>+  // http, proxy, socks, socks4, socks5, direct, unknown
>+  // example: "PROXY proxy1.foo.com:8080; PROXY proxy2.foo.com:8080; DIRECT"

please explain a bit more what you are trying to do.

>+  int c = 0;
>+  while (proxyArray[c] != NULL) {

>+    if (c != 0) {
>+      aResult.AppendLiteral("; ");
>+    }

don't you want this only starting after the first proxy element?



Basically looks fine; please fix up my nits and request review again.  Also factor out the builds/config specific changes and mark khuey as reviewer.
Comment 6 Wolfgang Rosenauer [:wolfiR] 2010-07-28 03:49:45 PDT
Created attachment 460831 [details] [diff] [review]
patch (build changes)

Patch carrying only build (configure.in, Makefile.in and system-headers) changes
Comment 7 Wolfgang Rosenauer [:wolfiR] 2010-07-28 03:51:58 PDT
Created attachment 460832 [details] [diff] [review]
patch

addressed nits
Comment 8 Wolfgang Rosenauer [:wolfiR] 2010-07-28 03:54:52 PDT
(In reply to comment #5)
> 2010

The code was actually written in 2009 (but I don't care and changed it).

> >+  // PAC string accepts
> >+  // http, proxy, socks, socks4, socks5, direct, unknown
> >+  // example: "PROXY proxy1.foo.com:8080; PROXY proxy2.foo.com:8080; DIRECT"
> 
> please explain a bit more what you are trying to do.

Hope it's a bit clearer now.
 
> >+  int c = 0;
> >+  while (proxyArray[c] != NULL) {
> 
> >+    if (c != 0) {
> >+      aResult.AppendLiteral("; ");
> >+    }
> 
> don't you want this only starting after the first proxy element?

Yes? The first is c == 0, isn't it? So it only starts appending it in front of the second element.
Comment 9 Doug Turner (:dougt) 2010-07-28 17:12:42 PDT
Comment on attachment 460832 [details] [diff] [review]
patch

>+  nsUnixSystemProxySettings() {}

please null out mProxyFactory in the constructor.



>+  char **proxyArray = nsnull;
>+  proxyArray = px_proxy_factory_get_proxies(mProxyFactory, (char*)(spec.get()));
>+  NS_ENSURE_TRUE(proxyArray, NS_ERROR_NOT_AVAILABLE);

>+    rv = ios->NewURI(nsDependentCString(proxyArray[c]), 
>+                                        nsnull, 
>+                                        nsnull, 
>+                                        getter_AddRefs(proxyURI));
>+    NS_ENSURE_SUCCESS(rv, rv);

if rv is not NS_OK, you will leak the proxy Array.  Might be best to just continue instead of bailing?

>+    proxyURI->GetScheme(schemeString);
>+    if (NS_SUCCEEDED(proxyURI->SchemeIs("http", &isScheme)) && isScheme) {
>+      schemeString.AssignLiteral("proxy");
>+    }
>+    aResult.Append(schemeString);
>+    if (NS_SUCCEEDED(proxyURI->SchemeIs("direct", &isScheme)) && !isScheme) {
>+      // Add the proxy URI only if it's not DIRECT
>+      proxyURI->GetHostPort(hostPortString);
>+      aResult.AppendLiteral(" ");
>+      aResult.Append(hostPortString);
>+    }

I still am having a hard time understanding what the libproxy string looks like.  Is it basically a list of URI's including user/password information?

>+//printf("returned PAC proxy string: %s\n", PromiseFlatCString(aResult).get());

delete or #ifdef


Getting really really close!  Probably one more patch and we are done here.
Comment 10 Wolfgang Rosenauer [:wolfiR] 2010-08-01 23:47:32 PDT
Created attachment 462003 [details] [diff] [review]
patch #2

address open details

That might be it?
Comment 11 Wolfgang Rosenauer [:wolfiR] 2010-08-03 22:12:57 PDT
Comment on attachment 460831 [details] [diff] [review]
patch (build changes)

This is NPOTB at the moment w/o setting the configure flag.
Comment 12 Wolfgang Rosenauer [:wolfiR] 2010-08-03 22:13:30 PDT
Comment on attachment 462003 [details] [diff] [review]
patch #2

This is NPOTB at the moment w/o setting the configure flag.
Comment 14 Wolfgang Rosenauer [:wolfiR] 2010-09-07 21:56:35 PDT
*** Bug 528700 has been marked as a duplicate of this bug. ***
Comment 15 Jason Duell [:jduell] (needinfo me) 2010-09-17 11:27:11 PDT
> This is NPOTB at the moment w/o setting the configure flag.

Do we want to enable libproxy by default?  Anything that needs to happen first?
Comment 16 Doug Turner (:dougt) 2010-09-17 12:35:14 PDT
we can/will for meego eventually.  I am not sure what linux distros want to do.
Comment 17 Wolfgang Rosenauer [:wolfiR] 2010-09-17 13:08:36 PDT
I'm using it since openSUSE 11.3 by default with FF 3.6.
Let's see what linux@distribution.bugs thinks ;-)
Comment 18 Mike Hommey [:glandium] 2010-10-20 06:13:12 PDT
(In reply to comment #17)
> I'm using it since openSUSE 11.3 by default with FF 3.6.
> Let's see what linux@distribution.bugs thinks ;-)

There is a big flaw imho in there: libproxy may be using libmozjs or libwebkit as a backend to interpret the pac file. This leads to major failure when libproxy is built against firefox 3.6's libmozjs and used with firefox 4.0.

While it is nice to attempt to better integrate with various linux desktops, libproxy is unfortunately not suited for that purpose at the moment.

If it were able to just return the proxy settings or .pac url depending on the system configuration, that would be better.
Comment 19 Wolfgang Rosenauer [:wolfiR] 2010-10-20 06:22:35 PDT
Hmm, is it not suited or is it "just" the technical issue about the linked libmozjs.so? I'm only aware of the latter and it should be possible to resolve it somehow. I'll bring that up for the libproxy guys.
Comment 20 Mike Hommey [:glandium] 2010-10-20 06:38:28 PDT
AFAIUI, libproxy returns the proxy that is to be used for a given url. That is, the proxy address to use for a given url. What firefox would need instead is a way to get the various settings (automatic proxy, http proxy, ftp proxy, etc.) from the system preferences.
Comment 21 Wolfgang Rosenauer [:wolfiR] 2010-10-20 06:46:03 PDT
What you mean is already implemented for GConf/Gnome but this ignores every other desktop and even is doing wrong things when running under KDE.
libproxy indeed just returns the exact proxy to use for a given URI but I don't see anything wrong with that. It transfers the logic to figure out the proxy for a given URI into libproxy and therefore a system service which should know what to do. This already works cross desktop and would end up in one instance where to set proxy stuff. Besides of possible technical issues with (probably) mixing mozjs symbols I don't see a design flow in that.
Do we want to implement reading of proxy settings for every Linux desktop out there directly in Firefox?
Comment 22 Mike Hommey [:glandium] 2010-10-20 06:54:04 PDT
(In reply to comment #21)
> What you mean is already implemented for GConf/Gnome but this ignores every
> other desktop and even is doing wrong things when running under KDE.
> libproxy indeed just returns the exact proxy to use for a given URI but I don't
> see anything wrong with that. It transfers the logic to figure out the proxy
> for a given URI into libproxy and therefore a system service which should know
> what to do. This already works cross desktop and would end up in one instance
> where to set proxy stuff. Besides of possible technical issues with (probably)
> mixing mozjs symbols I don't see a design flow in that.

... which is a big mess already. It means you have to build libproxy against all possible versions of mozjs to have it work in all versions of firefox. Which also means you have to rebuild it when newer versions of firefox come out. It also means upstream firefox can never interface correctly with system desktop if it relies on current libproxy.

> Do we want to implement reading of proxy settings for every Linux desktop out
> there directly in Firefox?

I'd prefer if there was a system library for that. And if that can't be libproxy, then maybe it should be something else.
Comment 23 Wolfgang Rosenauer [:wolfiR] 2010-10-20 07:02:43 PDT
I understand the concerns about mozjs but not sure if that's a problem for real.
Obviously libproxy cannot use whatever mozjs is installed at any given time but needs to rely on a stable mozjs. Possibly by either shipping it itself or using one which is independent of Firefox.
Then there "might" be an issue with having two mozjs loaded into one process (I haven't checked how it is implemented yet and if it's really an issue).

The next issue:
> It also means upstream firefox can never interface correctly with system
> desktop if it relies on current libproxy.

I don't understand that. Could you please explain? For the time being libproxy is probably not widespread enough to enable it by default in upstream builds.
What else do you mean?

So I still haven't seen a reason why libproxy (or any other solution) shouldn't return the proxy directly instead of the settings?
Comment 24 Mike Hommey [:glandium] 2010-10-20 07:06:15 PDT
(In reply to comment #23)
> I understand the concerns about mozjs but not sure if that's a problem for
> real.
> Obviously libproxy cannot use whatever mozjs is installed at any given time but
> needs to rely on a stable mozjs. Possibly by either shipping it itself or using
> one which is independent of Firefox.
> Then there "might" be an issue with having two mozjs loaded into one process (I
> haven't checked how it is implemented yet and if it's really an issue).

Unless you use versioned symbols, there's no way this can work. Except by using libwebkit as a backend for .pac, which would be awkward.
 
> The next issue:
> > It also means upstream firefox can never interface correctly with system
> > desktop if it relies on current libproxy.
> 
> I don't understand that. Could you please explain? For the time being libproxy
> is probably not widespread enough to enable it by default in upstream builds.
> What else do you mean?

Imagine libproxy is widespread and uses libmozjs as it is provided by the firefox you are running. This means it needs to know that particular libmozjs, thus any newer firefox... That just doesn't scale.
Comment 25 Mike Hommey [:glandium] 2010-10-20 07:08:24 PDT
(In reply to comment #23)
> I understand the concerns about mozjs but not sure if that's a problem for
> real.

BTW, I figured today this is a problem because I enabled libproxy support in my latest 4.0 beta builds, and it crashes in libproxy calls to mozjs.

Note You need to log in before you can comment on or make changes to this bug.