"Use System Proxy Settings" should be default for new profiles.

RESOLVED FIXED in mozilla1.9.3a1

Status

()

--
minor
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: andrewshilliday, Assigned: andrewshilliday)

Tracking

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
blocking1.9.1 -

Firefox Tracking Flags

(status1.9.2 .4-fixed, status1.9.1 ?)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5

With 1.9.1, OS X (and Linux) users have the option to have Firefox use the system proxy settings instead of manually setting them to mimic whatever settings are used.  This has the advantage that if users alter their system proxy settings (e.g., change network location), Firefox automatically makes the switch to the new settings.  

When new users install Firefox, the default setting is "no proxy".  Once this new feature of using system settings is fully implemented and tested (dependency on bug 457377), it should be the default setting for new users (i.e., new profiles).

Reproducible: Always
(Assignee)

Updated

10 years ago
Depends on: 457377
Do we really have to limit it to one OS or can we have this advantage on all platforms?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted1.9.1.x?
Flags: blocking1.9.1?
OS: Mac OS X → All
Version: unspecified → Trunk
There is no way this is going to block the release of Firefox 3.5 at this point, and you should know that. Please don't just flip every flag to get driver attention.
Flags: blocking1.9.1? → blocking1.9.1-

Comment 3

10 years ago
Windows doesn't have a "system proxy" for Firefox 3.5, but it's available on
the trunk (see bug 485764)

Note: bug bug 457377 is only for the SOCKS proxies
(In reply to comment #2)
> There is no way this is going to block the release of Firefox 3.5 at this
> point, and you should know that. Please don't just flip every flag to get
> driver attention.

Sorry, that was by accident. I wanted to flag blocking1.9.2.
Flags: blocking1.9.2?
(Assignee)

Comment 5

10 years ago
I'd agree this should be for all systems and not just OSX just wasn't sure "use system settings" was available to windows users.  Thanks Jo.

Updated

9 years ago
Flags: blocking1.9.2? → blocking1.9.2-
(Assignee)

Comment 6

9 years ago
Created attachment 392739 [details] [diff] [review]
Makes "use system settings" default option for all platforms

Removed the ifndef/ifdef conditions so that option is default for all platforms.  I'm not sure if there is a way to make this still conditional on whether the systemproxy component is available for the platform, anyone have any thoughts?
Attachment #392739 - Flags: review?(beltzner)
Attachment #392739 - Attachment mime type: application/octet-stream → text/plain
Attachment #392739 - Attachment is patch: true
status1.9.1: --- → ?
Flags: wanted1.9.1.x?

Comment 7

9 years ago
This fix will be the only way we can use Firefox at our school. Currently "use system settings" must be configured for each user!!!!!! There's no way we would take the time to log in as each user and set this option. Our content filtering is managed via a proxy server. I can't let students or teachers use Firefox without this setting in place. It's far easier to deny access to Firefox, as unfortunate as that is. Most of our users prefer Firefox and that's the only browser working for Teacher Web which our teachers utilize for updating their web pages. I certainly hope something can be done, as this is a MAJOR pitfall for us.
Comment on attachment 392739 [details] [diff] [review]
Makes "use system settings" default option for all platforms

Actual review needs to be done by a viable reviewer.
Attachment #392739 - Flags: ui-review?(beltzner)
Attachment #392739 - Flags: review?(bzbarsky)
Attachment #392739 - Flags: review?(beltzner)
Component: General → Networking
QA Contact: general → networking
So how does the "5" setting behave if we in fact do not have a systemproxy component?
(In reply to comment #9)
> So how does the "5" setting behave if we in fact do not have a systemproxy
> component?

This line is called both in nsProtocolProxyService::Init() and when the network.proxy. prefs branch changes:
http://mxr.mozilla.org/mozilla1.9.1/source/netwerk/base/src/nsProtocolProxyService.cpp#408
do_GetService() is called with "@mozilla.org/system-proxy-settings;1"
Not sure what do_GetService() does when the service isn't available. Does it throw?
It returns null.
Comment on attachment 392739 [details] [diff] [review]
Makes "use system settings" default option for all platforms

r- pending either an explanation of why this behaves correctly (identically to not using a proxy) on setups where there is no system proxy settings component or changes to the code to make it do so.
Attachment #392739 - Flags: review?(bzbarsky) → review-
Comment on attachment 392739 [details] [diff] [review]
Makes "use system settings" default option for all platforms

I don't think this needs a ui-review; if the default change is idempotent in terms of user experience (ie: it still works) then fine. Sounds like Boris needs convincing, though!
Attachment #392739 - Flags: ui-review?(beltzner)
In the interest of adding more detail on how this affects users, here's what I recently experienced:

1. A friend wanted to install Firefox on her work computer. It was locked down, so I didn't know whether it would work, but it seems the work that has been done in this area with installing without admin privileges actually allows this. Awesome.

2. She proceeds to start Firefox, and doesn't want to import the bookmarks or anything else. I assume this also means that she didn't get any proxy settings imported.

3. When Firefox tries to load a page, it fails. We later find that this is because it's configured to have "No Proxy" instead of automatically detecting the proxy. Switching to Auto for proxy makes the browser work as it should.

If I wasn't there to help, she probably would have said "Firefox doesn't work, I'll have to use IE" instead.

Of course, making it work in settings where there are no proxies when it's set to "auto" is key. Not sure if it already does that, when we switched her to Auto, it worked on several non-work networks too.
I think we all understand why we want this fixed. I just want to make sure the code is correct in a particular edge case; it looks fine otherwise.

It also looks like we need someone to pick up and own this bug...

Comment 16

9 years ago
Comment on attachment 392739 [details] [diff] [review]
Makes "use system settings" default option for all platforms

So. afaict, *most* of the code wants mSystemProxySettings to be null.

Except. I believe this code:
1226 nsresult
1227 nsProtocolProxyService::Resolve_Internal(nsIURI *uri,
1228                                          const nsProtocolInfo &info,
1229                                          PRBool *usePAC,
1230                                          nsIProxyInfo **result)

1234     *usePAC = PR_FALSE;
1235     *result = nsnull;

1240     if (mSystemProxySettings) {
1242         if (NS_SUCCEEDED(mSystemProxySettings->GetPACURI(PACURI)) &&
1247             if (NS_FAILED(rv))
1248                 return rv;

here we have a mSystemProxySettings and are falling out of the if block to line 1260

1249         } else {
1251             nsresult rv = mSystemProxySettings->GetProxyForURI(uri, proxy);
1252             if (NS_SUCCEEDED(rv)) {
1253                 ProcessPACString(proxy, result);
1254                 return NS_OK;
1255             }
1256             // no proxy, stop search
1257             return NS_OK;
1258         }
1259     }
1260 
1263     if (mProxyConfig == eProxyConfig_Direct ||
1264             (mProxyConfig == eProxyConfig_Manual &&
1265              !CanUseProxy(uri, info.defaultPort)))
1266         return NS_OK;

1268     // Proxy auto config magic...
1269     if (mProxyConfig == eProxyConfig_PAC || mProxyConfig == eProxyConfig_WPAD ||
1270         mProxyConfig == eProxyConfig_System) {
1271         // Do not query PAC now.
1272         *usePAC = PR_TRUE;
1273         return NS_OK;

here we've decided to usePAC even though we don't have a working systemproxy
Attachment #392739 - Flags: review-

Updated

9 years ago
Depends on: 529773
note bug 529773 (and its patch)
Comment on attachment 392739 [details] [diff] [review]
Makes "use system settings" default option for all platforms

The checked-in patch for bug 529773 addresses bz's concern, so I've requested re-review of the patch on this bug.
Attachment #392739 - Flags: review?(bzbarsky)
Attachment #392739 - Flags: review-
Comment on attachment 392739 [details] [diff] [review]
Makes "use system settings" default option for all platforms

r=bzbarsky
Attachment #392739 - Flags: review?(bzbarsky) → review+
Assignee: nobody → andrewshilliday
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/9f2860ad939e
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 392739 [details] [diff] [review]
Makes "use system settings" default option for all platforms

Requesting approval1.9.2.1 as it's a safe change, and allows Firefox to "work" (without further configuration) when a user's system has proxy settings already configured.
Attachment #392739 - Flags: approval1.9.2.1?
It's not a safe change unless bug 529773 is landed first.  And it's not obvious to me how safe that is.
Ah I guess that landed on 1.9.2.  OK.
Comment on attachment 392739 [details] [diff] [review]
Makes "use system settings" default option for all platforms

Moving nomination to 1.9.2.3, we'll take it when the tree re-opens.
Attachment #392739 - Flags: approval1.9.2.2? → approval1.9.2.3?
Comment on attachment 392739 [details] [diff] [review]
Makes "use system settings" default option for all platforms

a=beltzner for 1.9.2.4
Attachment #392739 - Flags: approval1.9.2.4? → approval1.9.2.4+
You need to log in before you can comment on or make changes to this bug.