Last Comment Bug 682832 - Gnome 3 proxy settings ignored
: Gnome 3 proxy settings ignored
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Shell Integration (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: Firefox 11
Assigned To: Jan Horak
:
: Robert Strong [:rstrong] (use needinfo to contact me)
Mentors:
Depends on: 710972 715063 716467
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-29 06:56 PDT by Jan Horak
Modified: 2012-02-21 22:39 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
gsettings proxy patch v 1 (14.57 KB, patch)
2011-08-29 07:07 PDT, Jan Horak
karlt: review-
Details | Diff | Splinter Review
backtrace first run (20.32 KB, text/plain)
2011-10-18 05:57 PDT, Jan Horak
no flags Details
backtrace next run (14.89 KB, text/plain)
2011-10-18 05:58 PDT, Jan Horak
no flags Details
DumpJSStack (1.92 KB, text/plain)
2011-10-18 06:11 PDT, Jan Horak
no flags Details
gsettings proxy patch v2 (14.87 KB, patch)
2011-11-29 02:50 PST, Jan Horak
karlt: review-
Details | Diff | Splinter Review
gsettings proxy patch v3 (17.33 KB, patch)
2011-12-01 06:59 PST, Jan Horak
karlt: review-
Details | Diff | Splinter Review
gsettings proxy patch v4 (16.82 KB, patch)
2011-12-02 01:43 PST, Jan Horak
karlt: review+
Details | Diff | Splinter Review
gsettings proxy patch v5 (16.81 KB, patch)
2011-12-05 04:29 PST, Jan Horak
jhorak: review+
Details | Diff | Splinter Review

Description Jan Horak 2011-08-29 06:56:32 PDT
Gnome 3 uses GSettings for proxy configuration now. Firefox does not get proxy settings set by gnome-control-center when option 'Use system proxy settings' in Connection settings is set.
Comment 1 Jan Horak 2011-08-29 07:07:20 PDT
Created attachment 556542 [details] [diff] [review]
gsettings proxy patch v 1

Initial patch, not sure who to set as reviewer, feel free to reassign.
Comment 2 Karl Tomlinson (back Dec 13 :karlt) 2011-09-04 22:59:12 PDT
Is this setting fetched during start-up?

I'm worried about using GSettings during start-up. (Bug 611953 comment 37)
Using GSettings during a delayed request after the browser is visible would be acceptable, I hope.

A stack track at the first invocation of this code may tell the story.
Comment 3 Jan Horak 2011-10-18 05:57:56 PDT
Created attachment 567719 [details]
backtrace first run

Attaching backtrace of first run (when previous version of Firefox was executed last time).
Comment 4 Jan Horak 2011-10-18 05:58:47 PDT
Created attachment 567720 [details]
backtrace next run
Comment 5 Jan Horak 2011-10-18 06:11:04 PDT
Created attachment 567725 [details]
DumpJSStack

By checking js stacktrace getting proxy settings for this kind of URI makes sense. I can't imagine how to avoid it during startup.
Comment 6 Karl Tomlinson (back Dec 13 :karlt) 2011-10-24 21:42:26 PDT
Thank you.  I'm OK with checking settings during startup if it's only for the whatsnew page when there is a new version.

Looks like starting the proxy service is lazy intentionally:
http://hg.mozilla.org/mozilla-central/annotate/767693e248aa/netwerk/base/src/nsIOService.cpp#l653

I'll look further into your patch, thanks.
Comment 7 Karl Tomlinson (back Dec 13 :karlt) 2011-11-24 17:06:54 PST
Comment on attachment 556542 [details] [diff] [review]
gsettings proxy patch v 1

I looked at the GSettingsService changes.

There is a new symbol that needs to be added to the GSETTINGS_FUNCTIONS at the
top of the file for dynamic lookup, and some other things to touch up there so
that this compiles against old versions of GIO.

>+#include "nsISupports.h"
> #include "nsCOMPtr.h"
>+#include "nsIMutableArray.h"

I expect nsISupports.h does not need to be listed explicitly because
nsIMutableArray.h or nsISupportsPrimitives.h will pull it in.

>+   const gchar ** gs_strings = g_variant_get_strv(value, NULL);
>+   g_variant_unref(value);

gs_strings doesn't own the strings, only the list of pointers.
What owns the strings after value is released?

>+   if (!gs_strings) {
>+       // empty array
>+       return NS_OK;
>+   }

aResult needs to be set if returning NS_OK.

>+   nsCOMPtr<nsIMutableArray> items(do_CreateInstance(NS_ARRAY_CONTRACTID));
>+   if (!items)
>+     return NS_ERROR_OUT_OF_MEMORY;   

Leaks gs_strings.

Probably tidiest to move this construction to before other allocations.

>+     nsCOMPtr<nsISupportsString> obj(do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID));

Would it make sense to instead use nsISupportsCString?
That would be more consistent with the AUTF8Strings returned by getString and
used in parameters, and it is probably the format more useful to the caller in
this case.

Think about whether using NS_NewAdoptingUTF8StringEnumerator instead of
nsIMutableArray would simplify things:
http://hg.mozilla.org/mozilla-central/annotate/84117219ded0/xpcom/ds/nsStringEnumerator.h#l90
Comment 8 Jan Horak 2011-11-28 04:25:56 PST
(In reply to Karl Tomlinson (:karlt) from comment #7)
> Think about whether using NS_NewAdoptingUTF8StringEnumerator instead of
> nsIMutableArray would simplify things:
> http://hg.mozilla.org/mozilla-central/annotate/84117219ded0/xpcom/ds/
> nsStringEnumerator.h#l90

Isn't NewAdoptingUTF8StringEnumerator available only for internal API? I'm having problem with compilation when file including nsStringEnumerator.h (it seems to include nsString.h which needs to have MOZILLA_INTERNAL_API defined and I don't see MOZILLA_INTERNAL_API in Makefile or in compilation output - in toolkit/system/gnome directory).
Comment 9 Karl Tomlinson (back Dec 13 :karlt) 2011-11-28 16:25:35 PST
(In reply to jhorak from comment #8)
> Isn't NewAdoptingUTF8StringEnumerator available only for internal API?

Looks like you are right, based on bug 466622 comment 16 and 17.
Comment 10 Jan Horak 2011-11-29 02:50:12 PST
Created attachment 577534 [details] [diff] [review]
gsettings proxy patch v2

Thanks for reviewing. Attaching patch which should respect mentioned issues.

>There is a new symbol that needs to be added to the GSETTINGS_FUNCTIONS at the
>top of the file for dynamic lookup, and some other things to touch up there so
>that this compiles against old versions of GIO.
Could you be more specific (the other things)?

Please have a look.
Comment 11 Karl Tomlinson (back Dec 13 :karlt) 2011-11-29 19:36:17 PST
Comment on attachment 577534 [details] [diff] [review]
gsettings proxy patch v2

>   FUNC(g_variant_get_string, const char *, (GVariant* value, gsize* length)) \
>   FUNC(g_variant_is_of_type, gboolean, (GVariant* value, const GVariantType* type)) \
>   FUNC(g_variant_new_int32, GVariant *, (gint32 value)) \
>   FUNC(g_variant_new_boolean, GVariant *, (gboolean value)) \
>   FUNC(g_variant_new_string, GVariant *, (const char* string)) \
>+  FUNC(g_variant_get_strv, GVariant *, (gsize* length)) \

These were roughly in an order.
Can you insert get_strv after get_string, please?

(In reply to jhorak from comment #10)
> Could you be more specific (the other things)?

Check the #define statements before and after GSETTIINGS_FUNCTIONS as any new
symbols need to be added here.

A #define is needed for g_variant_get_strv and G_VARIANT_TYPE_STRING_ARRAY.

>+  if (mGConf && IsProxyMode("auto")) {
>+    return mGConf->GetString(NS_LITERAL_CSTRING("/system/proxy/autoconfig_url"),
>+                             aResult);
>   }

>+  if (mGSettings) {
>+     // Check if mode is auto

I assume GSettings should override GConf settings.
Otherwise I assume those who upgrade to GNOME 3 will still be using their old
GConf settings, but the configuration utility will change the GSettings
values.

>+     nsCString proxyMode; 
>+     nsCOMPtr<nsIGSettingsCollection> proxy_settings;
>+     mGSettings->GetCollectionForSchema(NS_LITERAL_CSTRING("org.gnome.system.proxy"), 
>+                                        getter_AddRefs(proxy_settings));
>+     if (proxy_settings) {
>+       nsresult rv = proxy_settings->GetString(NS_LITERAL_CSTRING("mode"), proxyMode);

Move the proxyMode declaration to within the "if (proxy_setttings)" block
where it is used.

>+         return proxy_settings->GetString(NS_LITERAL_CSTRING("autoconfig-url"), 
>+                                             aResult);

Alignment.

>+  PRInt32 port;
>+  rv = proxy_settings->GetInt(NS_LITERAL_CSTRING("port"), &port);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+    
>+  SetProxyResult(aType, host, port, aResult);
>+  return NS_OK;

Need to return NS_ERROR_FAILURE when the port is 0.

       'Each of the 4 proxy types is enabled if its "host" key is
        non-empty and its "port" key is non-0.'

http://git.gnome.org/browse/gsettings-desktop-schemas/tree/schemas/org.gnome.system.proxy.gschema.xml.in.in

>+  // Check if proxy is enabled, flag is only in schema org.gnome.system.proxy.http,
>+  // there is no separate flag for each schema.
>+  nsresult rv = mGSettings->GetCollectionForSchema(NS_LITERAL_CSTRING("org.gnome.system.proxy.http"),
>+                                                   getter_AddRefs(proxy_settings));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = proxy_settings->GetBoolean(NS_LITERAL_CSTRING("enabled"), &masterProxySwitch);
>+  NS_ENSURE_SUCCESS(rv, rv);

"enabled" is described as "Unused", so don't use this key.

>+  bool useHttpProxyForAll = false;
>+  // This setting sometimes doesn't exist, don't bail on failure
>+  proxy_settings->GetBoolean(NS_LITERAL_CSTRING("use-same-proxy"), &useHttpProxyForAll);
>+
>+  if (!useHttpProxyForAll) {
>+    rv = SetProxyResultFromGSettings("org.gnome.system.proxy.socks", "SOCKS", aResult);
>+    if (NS_SUCCEEDED(rv))
>+      return rv;
>+  }
>+  

Similarly, "use-same-proxy" is also "Unused".

>+  if (aScheme.LowerCaseEqualsLiteral("http") || useHttpProxyForAll) {
>+    rv = SetProxyResultFromGSettings("org.gnome.system.proxy.http", "PROXY", aResult);
>+  } else if (aScheme.LowerCaseEqualsLiteral("https")) {
>+    rv = SetProxyResultFromGSettings("org.gnome.system.proxy.https", "PROXY", aResult);

Need to handle this case:

       "If an http proxy is configured, but an https proxy is not,
        then the http proxy is also used for https."
Comment 12 Karl Tomlinson (back Dec 13 :karlt) 2011-11-30 19:51:25 PST
Comment on attachment 577534 [details] [diff] [review]
gsettings proxy patch v2

>+      obj->SetData(nsCString(*p_gs_strings));

nsDependentCString is more conventional here.

>-static bool GConfIgnoreHost(const nsACString& aIgnore,
>-                              const nsACString& aHost)
>+static PRBool HostIgnoredByProxy(const nsACString& aIgnore,
>+                                 const nsACString& aHost)

We now use bool/true/false.

>+  nsDependentCSubstring aIgnoreStripped(start, slash);

The a- prefix on variables is used for parameters (arguments).
As this is not a parameter, so call it "ignoreStripped".

(In reply to Karl Tomlinson (:karlt) from comment #11)
> >+  if (mGConf && IsProxyMode("auto")) {
> >+    return mGConf->GetString(NS_LITERAL_CSTRING("/system/proxy/autoconfig_url"),
> >+                             aResult);
> >   }
> 
> >+  if (mGSettings) {
> >+     // Check if mode is auto
> 
> I assume GSettings should override GConf settings.
> Otherwise I assume those who upgrade to GNOME 3 will still be using their old
> GConf settings, but the configuration utility will change the GSettings
> values.

The existance of GSettings does not imply that gsettings-desktop-schemas is
installed, but, if there is an org.gnome.system.proxy schema, and mode is not
auto, then best to respect the desktop-schemas settings by not falling back to
GConf here.

>+  if (mGSettings)
>+    return GetProxyFromGSettings(scheme, host, port, aResult);
>+  else
>+    return GetProxyFromGConf(scheme, host, port, aResult);

Also here, best to fall back to GConf if org.gnome.system.proxy does not
exist.
Comment 13 Jan Horak 2011-12-01 06:59:35 PST
Created attachment 578249 [details] [diff] [review]
gsettings proxy patch v3

Attaching next version. Mentioned issues should be resolved. Also changed UUID of nsIGSettingsService due to its change.
Comment 14 Karl Tomlinson (back Dec 13 :karlt) 2011-12-01 18:41:41 PST
Comment on attachment 578249 [details] [diff] [review]
gsettings proxy patch v3

>+  nsCOMPtr<nsIGSettingsCollection> proxy_settings;
>+
>+  // Check if proxy is enabled, flag is only in schema org.gnome.system.proxy.http,
>+  // there is no separate flag for each schema.
>+  nsresult rv = mGSettings->GetCollectionForSchema(NS_LITERAL_CSTRING("org.gnome.system.proxy.http"),
>+                                                   getter_AddRefs(proxy_settings));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = mGSettings->GetCollectionForSchema(NS_LITERAL_CSTRING("org.gnome.system.proxy"),
>+                                          getter_AddRefs(proxy_settings));

The "org.gnome.system.proxy.http" schema is now not needed here.

>-  if (!mGConf)
>+  if (!mGConf || !mGSettings)
>     return GetProxyFromEnvironment(scheme, host, port, aResult);

This isn't what we want.  This will use the environment unless GSettings and
GConf are available.  Best to treat this as a fallback after the GSettings and
GConf code.

>+    /* Check for org.gnome.syste.proxy schema */
>+    nsCOMPtr<nsIGSettingsCollection> proxy_settings;
>+    mGSettings->GetCollectionForSchema(NS_LITERAL_CSTRING("org.gnome.system.proxy"),
>+                                       getter_AddRefs(proxy_settings));
>+    /* If schema is found let GSettings determine the right proxy, otherwise fallback to gconf. */
>+    if (proxy_settings)
>+      return GetProxyFromGSettings(scheme, host, port, aResult);

GetCollectionForSchema is an expensive operation, so I don't want the
"org.gnome.system.proxy" schema fetched both here and in GetProxyFromGSettings.

I think it should be fine to fall back to GConf and/or environment variables
when GetProxyFromGSettings returns a failure code.
Comment 15 Jan Horak 2011-12-02 01:43:14 PST
Created attachment 578520 [details] [diff] [review]
gsettings proxy patch v4

- Removed forgotten org.gnome.system.proxy.http
- GetProxyFromEnvironment moved to the end as the last method to use to determine proxy
- Part for checking org.gnome.syste.proxy schema removed.
Comment 16 Karl Tomlinson (back Dec 13 :karlt) 2011-12-04 16:30:26 PST
Comment on attachment 578520 [details] [diff] [review]
gsettings proxy patch v4

>+      items->AppendElement(obj, PR_FALSE);

Can you update PR_FALSE with false, please?
Comment 17 Jan Horak 2011-12-05 04:29:51 PST
Created attachment 579030 [details] [diff] [review]
gsettings proxy patch v5

Thanks for review, attaching patch with replaced PR_FALSE.
Comment 19 Matt Brubeck (:mbrubeck) 2011-12-05 10:46:37 PST
https://hg.mozilla.org/mozilla-central/rev/2f33758829d2
Comment 20 Theodore Lee 2012-02-21 22:27:13 PST
Does this fix cover setting a SOCKS proxy? The 2012-02-21 Nightly still doesn't seem to pick up SOCKS proxy settings from GNOME 3.
Comment 21 Karl Tomlinson (back Dec 13 :karlt) 2012-02-21 22:39:17 PST
We'll need bug 713802 to get support in nightlies.

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