Closed Bug 66057 Opened 24 years ago Closed 17 years ago

Proxy: $http_proxy should influence proxy settings

Categories

(Core :: Networking, enhancement)

x86
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: robbe, Assigned: ventnor.bugzilla)

References

Details

Attachments

(7 files, 8 obsolete files)

37.69 KB, patch
Details | Diff | Splinter Review
4.43 KB, patch
Details | Diff | Splinter Review
39.96 KB, patch
Details | Diff | Splinter Review
57.39 KB, patch
Details | Diff | Splinter Review
63.61 KB, patch
diane
: review+
Biesinger
: superreview-
Details | Diff | Splinter Review
1.43 KB, patch
beltzner
: ui-review+
beltzner
: approval1.9+
Details | Diff | Splinter Review
39.67 KB, patch
damons
: approval1.9+
Details | Diff | Splinter Review
On Unix, the environment variables http_proxy, ftp_proxy, etc. will by
convention often hold URLs pointing to appropriate proxies.

Mozilla should use these settings as the default for its own preferences.

E.g. a setting of http_proxy="http://proxy.example.com:8079/" would result in
default values like this:
user_pref("network.proxy.http", "proxy.example.com");
user_pref("network.proxy.http_port", 8079);
is it enough for the installer/first run of a profile to absorb these settings? 
or do you want us to always check them?

personally i would prefer for people to use PAC files [once we have our support 
working] as they are more flexible than an environment variable.
Severity: minor → enhancement
IMO these environment variables should be treated the same as Internet Config 
settings on Mac OS, or Internet control panel settings on Windows. That is, 
Mozilla reads from (and writes to?) them, but only if the `Use my system 
settings' checkbox (or whatever) is checked in that particular prefs panel.

It's been too long since I used Unix. Where in a user's directory are environment 
variables stored? Is it shell-dependent? If so, is there still an API for writing 
persistent changes to them without having to know where the file is?
env vars are relatively transient, a user can set them at will, and your 
average logout script does not persist them.  mozilla has no easy way of 
storing proxy settings for the correct shell(s) and if we tried people would 
yell and scream.

my logic:
as such, if the user has a proxy var setting, and then makes a change in 
mozilla, and then runs mozilla again. suppose we always read from the env, 
we'll dump the user's var and they'll complain.
I agree with mpt that a "Use system defaults" option is probably the best. This
should be checked by default. Most users will be comfortable with leaving it at
that.

For me as an admin, this makes it easy to change settings: I just edit
/etc/profile et al to load $http_proxy with a different value, and (hopefully)
all browsers will heed the change (unless the user chose to override it). PAC
files don't give me that until a lot more browsers support them.

That's also why doing it just on profile creation is not enough: sure, it would
help /now/; but as soon as the proxy parameters change, I'd be faced with either
sed'ding through all user's prefs.js, or having someone manually apply these
changes in the UI. Not pretty.
Confirming (Status NEW) based on the discussion.
Status: UNCONFIRMED → NEW
Ever confirmed: true
If the "use os settings" option is selected, then mozilla should always read the
settings. Most system administrators set then in global shell config files, such
as /etc/bashrc, for the use of other networking apps. On unix, an external
program can't change the value of an environment variable in another running
program, so these would only hav eto be read on startup, but they should be read
on every startup.

With ns4, if you started mozilla for the first time (ns4 for unix didn't have
profiles) and had those environment variables set, then the default settings
were for manual proxies rather than direct, and the proxies were prefilled for
http, ftp and gopher, as well as no_proxy_for. I don't know if socks was prefilled.

For mozilla, I propose that new profiles always (for all OSs which have this
implemented) default to "use system settings", and prefill the manual proxy
settings if possible, even those this option would not be selected by default.
(Other os's may just have a function which is given a url, and returns proxy
info, in which case this wouldn't be possible. For example, windows can had a
PAC file set up instead of individual manual proxy settings)

Does that sound reasonable? I may consider doing this at some point.
Networking owns this content.
Assignee: matt → gagan
Component: Preferences → Networking
QA Contact: sairuh → benc
no samir. this has not much to do with the fact that the preference is HTTP
specific. I am sure there could be other prefs that could follow this idea. It
really needs someone from prefs area. Over to prefs.
Assignee: gagan → bnesse
Component: Networking → Preferences: Backend
QA Contact: benc → sairuh
QA Contact: sairuh → benc
Nope, not BE-Prefs either. This sort of thing usually seems to end up in XP
Apps. ->pchen
Assignee: bnesse → pchen
Component: Preferences: Backend → XP Apps
So, rather than playing hot potato with this bug, can someone comment on my two
part suggestion? One of those is networking, and I can take the
infrastructure+unix side of that. The other is prefs/xpapps, I guess.

darin, mpt, anyone else?
i like the idea of picking this up automatically for new profiles, but does
anyone know how to get similar information under windows or under macos?
open transport on mac (theres a separate bug on that) and windows has an
internet control panel; there must be APIs to get at that data.

I would file a meta networking infrastructure bug, and have this + the OT one +
a new windows one depend on that, I guess.

The pref panel should have a "show me" button for this new option, which would
open the relevent control panels on mac/windows, and show a quick dialog with
the current settings of the environent vars. This will help debugging a _lot_.

The "initial value" thing I mention would depend on the infrastructure bug, if
we do want to go that way.
I'd imagine NSPR would have a way to access environment variables. AFAIK Mac has
no such concept. 
Just to clarify - yes, this will be platform specific code:
nsIOSProxy::FindProxyForURL. The code should cope with an instance of that
interface not existing, and disallow the choice of the new option from the ui in
that case.
Classic MacOS has "Internet Config", and OS X has a system level proxy pref.

At some point, we should add support for reading the system prefs.

I think most versions of IE cheat, they map their proxy config to the Internet
Config control panel, and they don't support other plats.
So we have agreement that this should be done.

I'll take this for unix + infrastructure. The profile stuff will be filed as a
separate bug once I'm done.
Assignee: pchen → bbaetz
Component: XP Apps → Networking
Target Milestone: --- → mozilla1.0
Mac OS 9 and earlier has no support for environment variables. In Mozilla, 
however, I added a facility which looks for a file called "ENVIRONMENT" in 
the application directory, which contains entries of the form:

NAME=VALUE

Calls to getenv() will return these values. This is really only intended for 
debugging, not for production code.
beard: But on osX, the official way is to use internet config, right?

I'll see if I have time to crank out some code this weekend for this.
Discussion of Mac-specific implementation issues are in bug 97214.
I have no time. I have a rough design sketched out, but...

->0.9.9, but +helpwanted. I really think that we do want this sooner rather than
later.
Keywords: helpwanted
Target Milestone: mozilla1.0 → mozilla0.9.9
No time; -> 1.1
Target Milestone: mozilla0.9.9 → mozilla1.1
No time for this, -> 1.1beta, may slip to 1.2
Target Milestone: mozilla1.1alpha → mozilla1.1beta
Blocks: 139393
-> default owner
Assignee: bbaetz → new-network-bugs
Target Milestone: mozilla1.1beta → ---
Summary: $http_proxy should influence proxy settings → Proxy: $http_proxy should influence proxy settings
*** Bug 13470 has been marked as a duplicate of this bug. ***
Summary: Proxy: $http_proxy should influence proxy settings → [RFE] Proxy: $http_proxy should influence proxy settings
Target Milestone: --- → Future
Summary: [RFE] Proxy: $http_proxy should influence proxy settings → Proxy: $http_proxy should influence proxy settings
Two suggestions:

- if an environment variable, for example $http_proxy doesn't exist then mozilla
should act as if there is a direct connection.  For example on my laptop: at
work I have a proxy setting (and I have a script which detects that I'm on the
work network and sets http_proxy), etc - but at home I have no $http_proxy set).

- The text on the preferences dialog should be something like:

  o Honor system environment variables ($http_proxy, $ftp_proxy, etc) if
    they are present, otherwise assume a direct internet connection.

I think we could just make "auto-detect proxy settings", which currently uses WPAD, check $http_proxy/$ftp_proxy first on Linux/Unix systems.
we have some code that asks gconf for the proxy settings... (extensions/system-pref iirc) maybe "autodetect" should ask that too. (it's currently a checkbox in Advanced)
I don't want to ask gconf unless use_system_prefs is on.

> (it's currently a checkbox in Advanced)

What checkbox?
[x] Use System Preferences
We have some differences in UI between mozilla/seamonkey and firefox.
Firefox 1.0.x has this "Auto-detect proxy settings for this network" but mozilla 1.7 doesn't have it, while mozilla has a checkbox for "Use system preferences".

I'm not sure what the best(TM) behaviour is.
I'm going to implement comment #26 (and try to start WPAD resolution earlier in the startup process so it doesn't slow down the time to display the first web page)
I have a patch for this which I'll attach in a moment.

Basically it hijacks the "Autodectect proxy settings for this network" preference, which currently just launches WPAD detection (which is just proxy-auto-config with the fixed URL 'http://wpad/wpad.dat'). The new behaviour of this preference setting is as follows:
-- On non-Unix platforms (those not using GTK2 as the toolkit) we try WPAD just as we used to.
-- On Unix platforms, if the environment variable DESKTOP_SESSION is "gnome" then we use the proxy settings from gconf. gconf can specify a PAC URL, in which case we will use it; hence it is possible to get WPAD back by the administrator setting 'http://wpad/wpad.dat' as the PAC URL in gconf.
-- On Unix platforms, if the environment variable DESKTOP_SESSION does not exist or is not is "gnome", then we honour *_PROXY environment variables. If the URL's scheme is XYZ then we look up the XYZ_PROXY environment variable for a proxy, or that does not exist, ALL_PROXY. If neither environment variable exists we fall back to try WPAD. Otherwise we try the indicated proxy, but we honour the NO_PROXY variable and force direct connections for URIs that match it.

Relationship with config.use_system_prefs: when gconf system-prefs is enabled, Mozilla's proxy settings are fully synchronized with the gconf settings --- either the user won't be able to change the proxy settings (if the gconf settings are read-only), or user changes will also update gconf. When gconf system-prefs is disabled, the user has the freedom to choose proxies, and this patch gives the user the ability to select gconf prefs if available.

The implementation introduces a new interface, nsISystemProxySettings. nsProtocolProxyService reinterprets eProxyConfig_WPAD to look for a service implementing this interface. If the service is found, we treat it as a sort of meta-PAC. The service can either return a real PAC URL to use, or otherwise it itself acts as a PAC, with nsProtocolProxyService passing in nsIURIs and getting back PAC-strings describing the proxy(ies) to use. This design accomodates arbitrarily complex system proxy policies.

Right now I only have a nsISystemProxySettings implementation for Unix-like systems, but it would be easy to provide alternative implementations for other platforms. This might be a slightly cleaner way to handle the Mac system proxy settings.
Assignee: general → roc
Attached patch patch (obsolete) — Splinter Review
One question about this patch is where the unixproxy component should go. I've put it in toolkit because it depends on the gconf toolkit component, but possibly it should go in extensions/.
(In reply to comment #33)
> Created an attachment (id=208222) [edit]
> patch

why are you using *_PROXY variables when the most used by far are $http_proxy and $ftp_proxy (notice the case)? 
Er yeah, Wolfgang fixed that but I need to fix it in my version of the patch.
Just to be sure, will this patch enable the same behaviour in mozilla-thunderbird?
Comment on attachment 208222 [details] [diff] [review]
patch

netwerk/base/public/nsISystemProxySettings.idl

the contractid should probably live in nsNetCID.h instead
Attached patch updated patchSplinter Review
Fixed the environment variable case issue. Also, I made "system proxy settings" be a new preference value instead of overloading _WPAD, add added UI for it, taken from Ron Crocker's patch in bug 66057.

Darin, where do you think unixproxy should be located, given that it depends on toolkit/components/gnome?
Attachment #208222 - Attachment is obsolete: true
Attached patch UI pieceSplinter Review
I left off the UI piece in the previous patch. Here it is.

I still need to sort out the placement of various components before I go for trunk review.
Comment on attachment 215059 [details] [diff] [review]
updated patch

>Index: toolkit/components/unixproxy/nsUnixSystemProxySettings.cpp
>+static PRBool
>+IsInNoProxyList(const nsACString& aHost, PRInt32 aPort, const char* noProxyVal)
>+{
>+  NS_ASSERTION(aPort >= 0, "Negative port?");

I'm not sure if that assertion is useful.
During my tests I figured out that it's pretty normal 
that the nsStandardURL has the default port -1
Then I need to add some code to map -1 to the correct default port.
I've got a new patch. I'll attach it later.
I wasn't sure how to handle a patch that applies to two bugs this one 66057 and  the os x specific version, 125995. So I'm posting it to both. 

This patch updates the previous patch for 66057 to the current CVS Head (I had a couple of problems applying the patch as a few files moved around), and also provides an implementation for retrieving the system settings from OS X.
 
I used a CVS checkout from yesterday for my testing, and verified that it basically works on both OS X and Linux--though under linux I only tested the environment variable proxy handling code.

I renamed "unixproxy" to "systemproxy" and added the OS X implementation. I also broke some of the environment handling code and utilities functions into a seperate file.
Attachment #239925 - Flags: review?(roc)
Attached patch my latest patchSplinter Review
Here's a copy of my latest patch. I should have attached it before; my humble apologies. It fixes a few issues and you should merge it into yours. (Taking the diff between this patch and my last patch would help you with that.)
Bug 354075 will complete the move of the gnome component to toolkit/system/gnome, and once that's done the patch for this bug should be updated with the new file locations for that component. Also, we'll want to remove mozgnome from the REQUIRES in systemproxy/Makefile.in. And also, I think in some newsgroup discussion we decided to put the new component in netwerk/system/systemproxy (a new directory).

Does that sound OK? Then we should get review from Christian Biesinger (biesi).
Depends on: 354075
Ok I merged the two patches in this bug... What I pulled out from yours was:

In browser/components/prefernces/connection.xul the prefwindow doctype was a bit more complicated.

In netwerk/base/src/nsProtocolProxyService.cpp::Resolve_Internal there was a return NS_OK to replace the line "// Otherwise, allow the WPAD PAC lookup to go ahead"

In toolkit/components/gnome/nsGConfService.cpp there were two more includes: nsSupportsPrimitives.h and nsArray.h

I also moved the REQUIRES mozgnome into the conditional block for the gnome support.

I don't suppose I could talk you into applying this version of the patch and seeing how it works under gnome? My linux boxes run KDE and I've been seduced by the gray-side of OS X and haven't been spending much time running linux on my desktop.

The renaming you suggested in comment #47 seem pretty reasonable. 

Though since this is the first time I've gotten involved with mozilla I don't have any strong opinions about the proper place for modules. 

Also I've got a couple of questions? What newsgroup was it where the suggestion of moving systemproxy to netwerk/system showed up? Also where can I find some documentation on what the magic markup is that makes a hyperlink to a bug? 

Should I go ahead and apply the patch in 354075 and then make an updated version of this patch or wait until the cvs tree updates?
Attachment #239925 - Attachment is obsolete: true
Attachment #239925 - Flags: review?(roc)
(In reply to comment #48)

Oh yeah I forgot to mention that I'm not sure if this patch will properly handle situations where the proxy exception list contains hosts of the form hostname:80 (or perhaps even when one connects to a host on the exception list with hostname:80). Basically I think the exception list and the base of the URI need to match, either both use the default port or both explicity list the :80.

I'm not sure how common this edge case'll be, but we might want to address it.
(In reply to comment #48)
> Also I've got a couple of questions? What newsgroup was it where the
> suggestion of moving systemproxy to netwerk/system showed up?

mozilla.dev.platform, thread subject "where to put per-platform system-services components"

> Also where can I find some documentation on what the magic markup is that
> makes a hyperlink to a bug?

In Bugzilla? You just say "bug " plus the bug number. Like "bug 354075".

> Should I go ahead and apply the patch in 354075 and then make an updated
> version of this patch or wait until the cvs tree updates?

I've just checked in the build system update from bug 354075. I will update your patch and rebuild, then you can test it on Mac, and if it's OK we'll go for review.

> Oh yeah I forgot to mention that I'm not sure if this patch will properly
> handle situations where the proxy exception list contains hosts of the form
> hostname:80

For Mac only, right? GNOME doesn't support port-based exceptions. I'll take a look at your code and give you some comments.
(In reply to comment #50)
> For Mac only, right? GNOME doesn't support port-based exceptions. I'll take a
> look at your code and give you some comments.

Oh, you mean the envvar stuff. Hmm.
You're right. We should fix this now because the easiest way is to just pass the default port to getProxyForURI as an additional parameter.
Your Mac code looks pretty good. I think we'd prefer it written Mozilla-style instead of Mac-style, e.g. using PRBool instead of "bool", InterCaps instead of underscored_words, and avoiding operator overloading. Biesi might be more lenient :-).

I'd make the little wrapper functions (SCProxySettings::ftp_enabled() etc) inline in the class definition.

+          CFIndex str_len = CFStringGetLength(str);
+          // the +1 is to include space for the null byte
+          char cStr[str_len+1];

This is a GCC extension. I've not seen it used before in Mozilla, I don't know what our policy is on using it in Mac-only code. I think we should probably avoid it if it's not too much pain.

+            // so are you supposed to include the null byte in a dependent 
+            // cstring?

No. What you have is correct.
(In reply to comment #51)
> (In reply to comment #50)
> > For Mac only, right? GNOME doesn't support port-based exceptions. I'll take a
> > look at your code and give you some comments.
> 
> Oh, you mean the envvar stuff. Hmm.
> 

I checked Safari, and it doesn't seem to parse entries with a port number correctly. Though the OS X dialog box certainly lets you enter them.
(In reply to comment #53)
> Your Mac code looks pretty good. I think we'd prefer it written Mozilla-style
> instead of Mac-style, e.g. using PRBool instead of "bool", InterCaps instead of
> underscored_words, and avoiding operator overloading. Biesi might be more
> lenient :-).

Heh, I'd think it's more the C++/Boost style than then Mac style, underscores seem to be falling out of favor. I can go ahead and make the it conform to your suggestions.

> +          CFIndex str_len = CFStringGetLength(str);
> +          // the +1 is to include space for the null byte
> +          char cStr[str_len+1];
> 
> This is a GCC extension. I've not seen it used before in Mozilla, I don't know
> what our policy is on using it in Mac-only code. I think we should probably
> avoid it if it's not too much pain.

I went looking through the variety of mozilla string types, and wasn't able to find a string class that lets you allocate a specific character buffer, is there a prefered idiom for creating temporary buffers? malloc, new, std::string, or something I missed in mozilla?
(In reply to comment #55)
> > This is a GCC extension. I've not seen it used before in Mozilla, I don't
> > know what our policy is on using it in Mac-only code. I think we should
> > probably avoid it if it's not too much pain.
> 
> I went looking through the variety of mozilla string types, and wasn't able
> to find a string class that lets you allocate a specific character buffer, is
> there a prefered idiom for creating temporary buffers? malloc, new,
> std::string, or something I missed in mozilla?

For this, I'd use nsAutoBuffer<char,SOME_CONSTANT>.
http://lxr.mozilla.org/mozilla/search?string=nsautobuffer

We have too many buffer and string types...
Attached patch updated againSplinter Review
Okay, here's my update. It updates to the new file locations, fixes the default port issue, and also updates the GConf GetStringList changes to build without access to private Gecko symbols, which seems to be required now. I also cleaned up a few stylistic issues:
-- We prefer to put most shared functions as static methods of a class, so I did that in nsSystemProxyUtils.
-- license boilerplate goes at the top of files, above #ifdef FOOBAR_H__
-- Don't do "if (blort == PR_TRUE)", just do "if (blort)"
-- some other minor things I forgot

Over to you, Diane
> -- We prefer to put most shared functions as static methods of a class, so I
> did that in nsSystemProxyUtils.

Ok. Though since I haven't seen that done before, I'm curious as to what the advantage is?

> -- Don't do "if (blort == PR_TRUE)", just do "if (blort)"

What about (Pointer != NULL) is there a strong style-guide recommendation? I noticed in your code that you seemed to prefer if (Pointer).


> Over to you, Diane

Ok I'll try to have my updates up sometime tomorrow.
(In reply to comment #58)
> > -- We prefer to put most shared functions as static methods of a class, so I
> > did that in nsSystemProxyUtils.
> 
> Ok. Though since I haven't seen that done before, I'm curious as to what the
> advantage is?

It's just a way to get namespaces that we used when real C++ namespaces were poorly supported in compilers.

> > -- Don't do "if (blort == PR_TRUE)", just do "if (blort)"
> 
> What about (Pointer != NULL) is there a strong style-guide recommendation? I
> noticed in your code that you seemed to prefer if (Pointer).

Yes, I think we generally do prefer "if (ptr)". Some other projects don't though. I can see arguments either way. The argument for "if (myBool)" is stronger, though ... I can't see any reason for == PR_TRUE.

Thanks!
(In reply to comment #57)

> Over to you, Diane
> 

Ok I merged everything in and added the "Use system settings" option back to connection.xul and connection.dtd.

Unfortunately it doesn't work now. What I've traced it to is in:

if (mProxyConfig == eProxyConfig_System) {
    mSystemProxySettings = do_GetService(NS_SYSTEMPROXYSETTINGS_CONTRACTID);
}

As far as I could tell do_GetService is returning NULL. libsystemproxy.a is being built, but when I looked in netwerk/build/libnecko.a the files don't seem to have be added (while "most" other network related compents are). I'm guessing that now that systemproxy is part of netwerk it should be included in libnecko? Any suggestions besides carefully reading the make scripts?
I think we'll have to carefully read make scripts :-).

http://lxr.mozilla.org/mozilla/source/netwerk/build/Makefile.in
I think we need to add the right thing to SHARED_LIBRARY_LIBS.

Also, we should move the build step out of toolkit_tiers and add "system" to the DIRS in netwerk/Makefile.in. netwerk/system will need a stub Makefile.in that adds systemproxy to DIRS if we're on Mac or Linux.
(In reply to comment #61)
> http://lxr.mozilla.org/mozilla/source/netwerk/build/Makefile.in
> I think we need to add the right thing to SHARED_LIBRARY_LIBS.

Oh! When I looked at SHARED_LIBRARY_LIBS I thought about dynamically loaded objects, not this is the list of libraries shared by this component.

When I made that update, nsOSXSystemProxy.o and nsSystemProxyUtils.o ended up in libnecko.

> Also, we should move the build step out of toolkit_tiers and add "system" to
> the DIRS in netwerk/Makefile.in. netwerk/system will need a stub Makefile.in
> that adds systemproxy to DIRS if we're on Mac or Linux.

I'd managed to figure out where I'd needed to add systemproxy to the right Makefile.in DIRS variable. Although I'm still stumped for a good makefile test for the unix proxy. (I've found ifeq ($(OS_ARCH),Darwin) for os x.

Unfortunately firefox still doesn't actually use the proxy. Now my suspicions are the XPCOM configuration isn't quite right. The Init function for nsOSXSystemProxy doesn't seem to be called when I launch firefox. When looking at most of the other modules in netwerk, most of them seem to belong to the netwerk module, and their NS_*_FACTORY_CONSTRUCTOR macros mostly appear to be in netwerk/build/nsNetModule.cpp.

I think I'd like to read up more on XPCOM, and see if I can figure out what's up Saturday.
(In reply to comment #62)
> > Also, we should move the build step out of toolkit_tiers and add "system" to
> > the DIRS in netwerk/Makefile.in. netwerk/system will need a stub Makefile.in
> > that adds systemproxy to DIRS if we're on Mac or Linux.
> 
> I'd managed to figure out where I'd needed to add systemproxy to the right
> Makefile.in DIRS variable. Although I'm still stumped for a good makefile test
> for the unix proxy. (I've found ifeq ($(OS_ARCH),Darwin) for os x.

I think we should just use MOZ_ENABLE_GTK2 for unixproxy.

> Unfortunately firefox still doesn't actually use the proxy. Now my suspicions
> are the XPCOM configuration isn't quite right. The Init function for
> nsOSXSystemProxy doesn't seem to be called when I launch firefox. When looking
> at most of the other modules in netwerk, most of them seem to belong to the
> netwerk module, and their NS_*_FACTORY_CONSTRUCTOR macros mostly appear to be
> in netwerk/build/nsNetModule.cpp.

The XPCOM component is not getting registered. I used to have systemproxy building as a standalone XPCOM component in its own shared library, but I removed the FORCE_SHARED_LIBRARY setting, which broke that I guess (not sure why things worked when I tested my patch). There is actually no reason to be a standalone XPCOM component, we should just link tightly into the rest of necko. That means we need to be added to nsNetModule, because our own module isn't going to be registered. It should be just a matter of moving this code

+#define NS_UNIXSYSTEMPROXYSERVICE_CID  /* 0fa3158c-d5a7-43de-9181-a285e74cf1d4 */\
+     { 0x0fa3158c, 0xd5a7, 0x43de, \
+       {0x91, 0x81, 0xa2, 0x85, 0xe7, 0x4c, 0xf1, 0xd4 } }
+
+NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsUnixSystemProxySettings, Init)
+
+static const nsModuleComponentInfo components[] = {
+  { "Unix System Proxy Settings Service",
+    NS_UNIXSYSTEMPROXYSERVICE_CID,
+    NS_SYSTEMPROXYSETTINGS_CONTRACTID,
+    nsUnixSystemProxySettingsConstructor }
+};

to the right place in nsNetModule.
Well, you'll also need to massage header file paths to get the right headers visible in nsNetModule.
Attached patch move system proxy into libnecko (obsolete) — Splinter Review
Ok, I have a couple of concerns about this patch.

My biggest concern is that I needed to modify browser/app/Makefile.in in order to add the -framework link option and I'm suspect this would break other applications that depend on libnecko.

I couldn't find a good make variable to set in the netwerk/build/Makefile.in that would actually influence the application build. Also I'm not certain but it seems like frameworks cannot be linked against static libraries. (If needed I can try testing that a bit more carefully).

The second concern is that nsNetModule.cpp looks pretty complex and I didn't want to add a conditional compliation block to include the os x and unix specific proxies, instead I renamed both classes to nsSystemProxySettings and moved the class definition to its own .h file. 

I was also put the NS_SYSTEMPROXYSETTINGS_CLASSNAME and NS_SYSTEMPROXYSETTINGS_CID in as well, which made defining the magic XPCOM blocks in nsNetModule.cpp a bit easier.

The downside is to make the same class header apply to both our classes I need to conditionally include the mGConf private variable. So mostly this modification tried to localize the complexity in netwerk/system/systemproxy to try and simplify its external interface.

It runs correctly on OS X, and I'm having build problems with some svg code on my linux box so wasn't able to test there. (And that was after trying to disable cairo which doesn't seem to link correctly on a debian unstable box).

Diane
Attachment #239975 - Attachment is obsolete: true
(In reply to comment #65)
> I couldn't find a good make variable to set in the netwerk/build/Makefile.in
> that would actually influence the application build. Also I'm not certain but
> it seems like frameworks cannot be linked against static libraries. (If needed
> I can try testing that a bit more carefully).

There are two kinds of builds: static builds, where all the Gecko components are linked into one big shared library, and nonstatic builds where components get their own shared libraries. In the latter case, adding -framework flags to netwerk/build should ensure that the libnecko shared library links correctly and everything will be fine. For the former case, I think things will work if we add to http://lxr.mozilla.org/mozilla/source/config/static-config.mk
so the final libxul shared library links to the frameworks (right, bsmedberg?)

> The downside is to make the same class header apply to both our classes I need
> to conditionally include the mGConf private variable. So mostly this
> modification tried to localize the complexity in netwerk/system/systemproxy to
> try and simplify its external interface.

I think this approach is OK. #ifdef GNOME probably isn't the right thing. You could actually just make mGConf unconditional, it should build on OSX because nsIGConfService is present there, just not implemented by any component. You should delete the associated comment. Also you can delete the commented-out component registration in nsOSXSystemProxySettings/nsUnixSystemProxySettings. Thanks!!!
Ok adding an ifeq/endif block to static-config.mk to add the -framework did work, and I removed the conditional around the mGConf variable.

I also applied to a clean tree and verified that this patch works on OS X.

I hope we're almost there. 

And I wanted to thank you with your help with the details of the build system.
Diane
Attachment #240791 - Attachment is obsolete: true
Why have you commented these out?

+# FRAMEWORKS += \
+#         -framework SystemConfiguration\
+#         -framework CoreFoundation\
+#         $(NULL)

+# EXTRA_DSO_LDOPTS += \
+#         $(FRAMEWORKS)   

This is needed for non-static builds, isn't it?
(In reply to comment #68)
> Why have you commented these out?

Because I forgot to delete them?

They're left over from when this was being built as a separate component, and I wasn't completely sure that they weren't needed any more. 
I think we need them in netwerk/build/Makefile.in to ensure that the libnecko shared library imports those libraries properly, for non-static builds.
(In reply to comment #70)
> I think we need them in netwerk/build/Makefile.in to ensure that the libnecko
> shared library imports those libraries properly, for non-static builds.
> 

Any suggestions about how I could force a non-static build?
I thought non-static was the default. Maybe you have --enable-static in your mozconfig somewhere?
(In reply to comment #72)
> I thought non-static was the default. Maybe you have --enable-static in your
> mozconfig somewhere?
> 

You're right! I'd just blindly set up my mozconfig from the OS X configuration. Sorry to have been pestering you with so many questions...

I've made the changes you suggested and will try testing tonight or tomorrow morning.
Honestly, the build system is pretty opaque to me too.
Attached patch Builds as shared & static (obsolete) — Splinter Review
Ok, new version. This has the framework options back in the shared configuration part of netwerk/build/Makefile.in

I built it as both --disable-shared --enable-static and --enable-static --disable shared. (Though to be fair when retesting this round of the static build I didn't do a make clean after the shared build).

The other possibly questionable thing is I used "FRAMEWORKS +=" instead of "=" or ":=". I don't think that variable was defined in this scope, but I suppose there might be a chance that it's used later. (Though I don't think multiple copies of the same -framework are a problem).
Attachment #240886 - Attachment is obsolete: true
Comment on attachment 240953 [details] [diff] [review]
Builds as shared & static

okay, let's get this going!
Attachment #240953 - Flags: superreview?(cbiesinger)
Attachment #240953 - Flags: review?(cbiesinger)
(In reply to comment #76)
> (From update of attachment 240953 [details] [diff] [review] [edit])
> okay, let's get this going!
> 

Yay!

I'm excited to have gotten this patch this far.
(In reply to comment #76)
> (From update of attachment 240953 [details] [diff] [review] [edit])
> okay, let's get this going!
> 

Just wanted to mention that Josh Aas was reviewing the old version of this patch that I'd left under bug 125995.

Of potential interest to you, he wanted the copyright headers cleaned up and I guessed how you should be attributed in the files you created, but you might want to review.
Just merge in any changes from Josh's review in bug 125995 and reattach the result here ... thanks
He had me update the copyright notices and return nsresults to indicate failure of a call instead of a PRBool.
Attachment #240953 - Attachment is obsolete: true
Attachment #242611 - Flags: review+
Attachment #240953 - Flags: superreview?(cbiesinger)
Attachment #240953 - Flags: review?(cbiesinger)
Comment on attachment 242611 [details] [diff] [review]
Merged in Josh's requested updates from bug 125995

still needs biesi love
Attachment #242611 - Flags: superreview?(cbiesinger)
Comment on attachment 242611 [details] [diff] [review]
Merged in Josh's requested updates from bug 125995

OK, my sincere apologies for taking so long to review this. This "real life"
thing kept me busy the last few months.

(Is there a reason you didn't use cvs diff?)

A Firefox peer will have to review the UI changes here. It would also be nice
if you could file bugs on SeaMonkey and Camino about this new pref value.

Please add the -p diff option next time...


+++ mozilla/netwerk/base/public/nsISystemProxySettings.idl	2006-10-12 23:56:06.000000000 -0700
+#define NS_SYSTEMPROXYSETTINGS_CONTRACTID "@mozilla.org/system-proxy-settings;1"

Don't put contract IDs in IDL files. Instead, put them in nsNetCID.h.

+ * This interface allows the proxy code to use platform-specific proxy
+ * settings when the proxy preference is set to "automatic discovery". If it can
+ * load a service with the above contract ID, it will use it to determine the
+ * PAC file name. If no PAC file is specified then the service itself will behave
+ * like a PAC file.

I'm not sure what the "it" refers to in the second line of this comment. Is
this a description of how the protocol proxy service behaves? If so, why is
this comment here instead of somewhere specific to the protocol proxy service?

+     * See nsIProxyAutoConfig::getProxyForURI; this function behaves exactly
+     * the same way.

So, if it behaves exactly the same way, why does its signature differ?



+++ mozilla/netwerk/base/src/nsProtocolProxyService.cpp	2006-10-13 00:16:26.000000000 -0700
 
+
 //----------------------------------------------------------------------------

Why this change?

+    if (mProxyConfig != eProxyConfig_PAC && mProxyConfig != eProxyConfig_WPAD
+        && mProxyConfig != eProxyConfig_System)

Please put operators at the end of a line instead of at the start of the next
line.

-        ConfigureFromPAC(tempString);

Hm... doesn't this mean that you're no longer reloading the PAC URL when PAC
is configured explicitly and the URL changes?

+    if (mProxyConfig != eProxyConfig_PAC && mProxyConfig != eProxyConfig_WPAD
+                && mProxyConfig != eProxyConfig_System)

Perhaps this should move to a helper function instead? (Otherwise, the same
operator comment, and the second line is incorrectly indented)

     NS_ENSURE_ARG_POINTER(uri);
-
     *usePAC = PR_FALSE;

why this change?

+            // Switch to new PAC file if that setting has changed.

Hm... I'm not sure I like this... Can't the system proxy service broadcast a
message if the PAC URL changes?

Doesn't ReloadPAC need a change too?

+++ mozilla/netwerk/build/Makefile.in	2006-10-12 23:26:13.000000000 -0700
-
+        

Please don't add trailing whitespace

+# It'd be nice if there was a way for testing, GTK and Darwin

There is such a way:
ifneq (,$(filter gtk2 mac cocoa,$(MOZ_WIDGET_TOOLKIT)))

+++ mozilla/netwerk/system/systemproxy/Makefile.in	2006-10-12 23:26:13.000000000 -0700
+REQUIRES = \
+        xpcom \
+        string \
+        necko

Please add a line with $(NULL). That way, if someone adds another dependency
at the end, the diff output will look nicer.

+++ mozilla/netwerk/system/systemproxy/nsOSXSystemProxySettings.cpp	2006-10-13 00:56:13.000000000 -0700
+ * The Initial Developer of the Original Code is
+ * Diane Trout.

Usually this includes an email address... (And usually the original author
isn't also listed as contributor)

+        aResult.Append(pacUrl);

This must be Assign, not Append. There is no guarantee that aResult is already
an empty string.

This happened in a few other places as well.

+  PRInt32 proxyPort=0;
+  PRBool isDirectHost=false;

Usually there are spaces around operators... and is there a reason you are
mixing "false" with a PRBool? That'd usually be PR_FALSE. I'd also move
isDirectHost to the line before it is used. But come to think of it, why not
make InExceptionList return the PRBool and remove this variable?

+    proxy.HTTPHost(proxyHost);
+    proxy.HTTPPort(proxyPort);

Hm... I'd name functions that don't return the value GetHTTPHost, etc. (return
meaning as actual return value)

+nsSystemProxySettings::GetProxyForURI(nsIURI* aURI, PRInt32 aDefaultPort,
+                                         nsACString& aResult)

Incorrect indentation on the second line...

+    return GetProxyForURIFromSystemConfig(proxySettings, scheme, host, port, aResult);
+  } else {

No point in an else after a return.

I didn't review this file in detail, I don't know the Mac APIs. I hope someone
else did.

+++ mozilla/netwerk/system/systemproxy/nsSystemProxySettings.h	2006-10-13 00:10:01.000000000 -0700
+ * The Initial Developer of the Original Code is
+ * Robert O'Callahan.

same comment as above :)


Is it really a good idea to use the same header for this service on all
platfoprms, meaning that OS X can't have a SCProxySettings member and must
have an mGConf member which it has no use for?

+  nsSystemProxySettings() {};  

please remove that semicolon here

+++ mozilla/netwerk/system/systemproxy/nsSystemProxyUtils.cpp	2006-10-13 00:11:32.000000000 -0700
+ * The Initial Developer of the Original Code is
+ * Robert O'Callahan.

as above

    nsCAutoString portStr2(portStr);

is that really needed? :/ i.e. portStr.ToInteger doesn't exist?

+    if (StringEndsWith(aHost, hostStr, nsCaseInsensitiveCStringComparator()))

Why's this a StringEndsWith instead of an equality comparison? The header
definitely needs more comments about what this function does.

BTW, a few lines in this file have trailing whitespace. Please remove that :)

  if (!isHTTP)
    return NS_ERROR_FAILURE;

why not use NS_ERROR_UNKNOWN_PROTOCOL?

Hm, what'd the desired result for a URI without an explicit port be? Your code
would put -1 as the port in the string, I believe. That doesn't seem
desirable.

+++ mozilla/netwerk/system/systemproxy/nsSystemProxyUtils.h	2006-10-13 00:06:58.000000000 -0700

BTW, I wouldn't use HPP in the include guards, the filename is .h not .hpp.

So the comments in this file don't help me much...
+   * Test a specific proxy entry against aHost:aPort. (handles optional port)

What does "handle" mean? I.e., if there is no :port in proxyEntry, what
happens?

+   * Format a proxy result and store it in aResult

Perhaps this should mention how it formats the result (i.e. in the format
expected by PAC), and what possible aType values are

+   * Construct a ProxyURI from environment variables (http_proxy, etc)

This is comment is somewhat misleading. This doesn't construct any URI. It
parses an URI and returns a PAC-like string identifying the proxy.

+  static nsresult GetProxyForURIFromEnvironment(const nsACString& aScheme,

Since this doesn't take a URI, perhaps the ForURI part should be removed from
the name.

Should perhaps mention that the scheme must be lowercase.

#endif /*NSSYSTEMPROXYENVIRONMENT_HPP_*/

please add a space after /* and before */.

+++ mozilla/netwerk/system/systemproxy/nsUnixSystemProxySettings.cpp	2006-10-13 00:20:00.000000000 -0700
+  // was not a GNOME session, using *_PROXY environment variables.

It's *_proxy :)

+  return NS_SUCCEEDED(aGConf->GetString(NS_LITERAL_CSTRING("/system/proxy/mode"), mode)) &&
+      mode.EqualsASCII(aMode);

please indent mode so that it ends up under the NS_ here

+IsProxyMode(nsIGConfService* aGConf, const char* aMode)

If you made this a member function, you wouldn't need the aGConf argument.

  if (StringBeginsWith(aIgnore, NS_LITERAL_CSTRING("*")) &&

how about: |if (aIgnore.First() == '*' &&| ?

Should /system/http_proxy/use_http_proxy be checked as well?

+++ mozilla/toolkit/system/gnome/nsGConfService.cpp	2006-10-12 23:26:13.000000000 -0700

+  virtual ~StringListEnumerator() {
+    g_slist_free(mList);
+  }

The documentation at
http://developer.gnome.org/doc/API/2.0/gconf/gconf-GConfClient.html#gconf-client-get-list
says:
> In the GCONF_VALUE_FLOAT and GCONF_VALUE_STRING cases, you must g_free() each list element.

+++ mozilla/xpcom/system/nsIGConfService.idl	2006-10-12 23:26:13.000000000 -0700

need a new IID
Attachment #242611 - Flags: superreview?(cbiesinger) → superreview-
diane, any chance of you updating the patch to address biesi's sr comments?
Attached patch Updated Unix patch (obsolete) — Splinter Review
Roc's old Unix patch carried through over a year of bitrot. I didn't bring along the Mac stuff since I don't understand Mac, and I'll do the UI bits later.
Attachment #293620 - Flags: superreview?(cbiesinger)
Attachment #293620 - Flags: review?(cbiesinger)
Just to mention, as Roc suggested I do, the patch above by itself has no UI, instead it uses a hidden preference in about:config by setting network.proxy.type to 5. This patch would be extraordinarily useful for Linux distributors especially for Firefox 3 where proxy integration has sorely lacked, as they can ship that preference as default in their Firefox 3 packages.
Attached patch Updated Unix patch 1.1 (obsolete) — Splinter Review
I really must remember the ifdefs since we cater to more than just Linux...
Attachment #293620 - Attachment is obsolete: true
Attachment #293721 - Flags: superreview?(cbiesinger)
Attachment #293721 - Flags: review?(cbiesinger)
Attachment #293620 - Flags: superreview?(cbiesinger)
Attachment #293620 - Flags: review?(cbiesinger)
Comment on attachment 293721 [details] [diff] [review]
Updated Unix patch 1.1

bad indentation (maybe tabs?):
+            nsresult rv = ConfigureFromPAC(PACURI);
+           if (NS_FAILED(rv))
+                return rv;

proxy_MaskIPv6Addr's indentation doesn't match the file.
All it would take to make UI for this is to add a radio button in the Connection dialog. This patch gives the strings that will allow for that.
Assignee: roc → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #298800 - Flags: ui-review?(beltzner)
Attachment #298800 - Flags: ui-review?(beltzner) → ui-review+
Attachment #298800 - Flags: approval1.9?
Comment on attachment 298800 [details] [diff] [review]
String additions for possible future UI

a=beltzner for 1.9
Attachment #298800 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Keywords: helpwanted
QA Contact: benc → networking
Target Milestone: Future → ---
Checking in browser/locales/en-US/chrome/browser/preferences/connection.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/connection.dtd,v  <--  connection.dtd
new revision: 1.8; previous revision: 1.7
done
Keywords: checkin-needed
Comment on attachment 293721 [details] [diff] [review]
Updated Unix patch 1.1

+    ACString getProxyForURI(in nsIURI aURI);

I think this should better return AUTF8String (so that people can specify an IDN host for the proxy)

+  nsCOMPtr<nsIMutableArray> items (do_CreateInstance(NS_ARRAY_CONTRACTID));

no space after items

+    nsCOMPtr<nsISupportsCString> obj (do_CreateInstance(NS_SUPPORTS_CSTRING_CONTRACTID));

no space after obj

Also, this should be an nsISupportsString, given that gconf stores UTF-8 data and nsISupportsCString only supports Latin1.

+  nsIArray      getStringList(in AUTF8String key);

would be nice to add an comment about the type of the items in the array

+                tempString.AssignLiteral("http://wpad/wpad.dat");

shouldn't that be WPAD_URL?

+            // Switch to new PAC file if that setting has changed. If the setting
+            // hasn't changed, ConfigureFromPAC will exit early.
+            nsresult rv = ConfigureFromPAC(PACURI);
+           if (NS_FAILED(rv))
+                return rv;

indentation is not correct here

+        eProxyConfig_System, // use system proxy settings if available, otherwise WPAD

the behaviour seems to rather be, "otherwise DIRECT", no?

+{
+  if (!mGConf || !IsProxyMode("auto"))
+    return NS_ERROR_FAILURE;

the IDL says to return an empty string in that case, not to throw an exception

+      if (err != 0) {

should be NS_FAILED(err)

+static void SetProxyResult(const char* aType, const nsACString& aHost,
+                               PRInt32 aPort, nsACString& aResult)

incorrect indentation on the second line
Attachment #293721 - Flags: superreview?(cbiesinger)
Attachment #293721 - Flags: superreview+
Attachment #293721 - Flags: review?(cbiesinger)
Attachment #293721 - Flags: review+
Attached patch Nits fixedSplinter Review
Thanks biesi!

Now please lets get this oft-desired patch into 1.9! The thing is, this is Novell code that Roc wrote and he tells me that Novell has been shipping this for years, so that means it must work well as it has undergone a lot of testing. It'll be about:config only with just this patch, and likely Beta 3, but UI would be trivial to implement since the string has already landed.

This is one of the best things we could ever do for Linux integration.
Attachment #293721 - Attachment is obsolete: true
Attachment #299799 - Flags: approval1.9?
Comment on attachment 299799 [details] [diff] [review]
Nits fixed

a1.9+=damons
Attachment #299799 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
RCS file: /cvsroot/mozilla/netwerk/base/public/nsISystemProxySettings.idl,v
done
Checking in netwerk/base/public/nsISystemProxySettings.idl;
/cvsroot/mozilla/netwerk/base/public/nsISystemProxySettings.idl,v  <--  nsISystemProxySettings.idl
initial revision: 1.1
done
Checking in netwerk/base/public/Makefile.in;
/cvsroot/mozilla/netwerk/base/public/Makefile.in,v  <--  Makefile.in
new revision: 1.120; previous revision: 1.119
done
Checking in toolkit/system/gnome/nsGConfService.cpp;
/cvsroot/mozilla/toolkit/system/gnome/nsGConfService.cpp,v  <--  nsGConfService.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in xpcom/system/nsIGConfService.idl;
/cvsroot/mozilla/xpcom/system/nsIGConfService.idl,v  <--  nsIGConfService.idl
new revision: 1.3; previous revision: 1.2
done
Checking in netwerk/base/src/nsPACMan.h;
/cvsroot/mozilla/netwerk/base/src/nsPACMan.h,v  <--  nsPACMan.h
new revision: 1.6; previous revision: 1.5
done
Checking in netwerk/base/src/nsProtocolProxyService.cpp;
/cvsroot/mozilla/netwerk/base/src/nsProtocolProxyService.cpp,v  <--  nsProtocolProxyService.cpp
new revision: 1.72; previous revision: 1.71
done
Checking in netwerk/base/src/nsProtocolProxyService.h;
/cvsroot/mozilla/netwerk/base/src/nsProtocolProxyService.h,v  <--  nsProtocolProxyService.h
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/toolkit-makefiles.sh;
/cvsroot/mozilla/toolkit/toolkit-makefiles.sh,v  <--  toolkit-makefiles.sh
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/Makefile.in;
/cvsroot/mozilla/toolkit/Makefile.in,v  <--  Makefile.in
new revision: 1.31; previous revision: 1.30
done
RCS file: /cvsroot/mozilla/toolkit/system/unixproxy/Makefile.in,v
done
Checking in toolkit/system/unixproxy/Makefile.in;
/cvsroot/mozilla/toolkit/system/unixproxy/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp,v
done
Checking in toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp;
/cvsroot/mozilla/toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp,v  <--  nsUnixSystemProxySettings.cpp
initial revision: 1.1
done
Checking in toolkit/library/libxul-config.mk;
/cvsroot/mozilla/toolkit/library/libxul-config.mk,v  <--  libxul-config.mk
new revision: 1.62; previous revision: 1.61
done
Checking in toolkit/library/nsStaticXULComponents.cpp;
/cvsroot/mozilla/toolkit/library/nsStaticXULComponents.cpp,v  <--  nsStaticXULComponents.cpp
new revision: 1.51; previous revision: 1.50
done
Checking in netwerk/build/nsNetCID.h;
/cvsroot/mozilla/netwerk/build/nsNetCID.h,v  <--  nsNetCID.h
new revision: 1.68; previous revision: 1.67
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Backed out most of this due to bustage on debug Linux. Most likely we're just missing something simple in toolkit/system/unixproxy/Makefile.in.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1201629360.1201629560.21211.gz&fulltext=1#err0

bsmedberg: ideas?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I relanded this. Ventron said he will file a bug on Mac adding support for this.
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
(In reply to comment #96)
> I relanded this. Ventron said he will file a bug on Mac adding support for
> this.
> 

The mac part was moved from bug 125995 to here ;-)
(In reply to comment #96)
> I relanded this. Ventron said he will file a bug on Mac adding support for
> this.
> 

Actually I was going to let a Mac user file that bug, but it seems I don't need to with bug 125995 :)
I'm trying to compile thunderbird and it fails since mozilla/toolkit/system/unixproxy does not provide the target "export".
Im working with ubuntu 7.10 and had no problems compiling firefox on 26.1.08.
Is anybody else experiencing this trouble?
Depends on: 421490
Depends on: 429520
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: