682832 - Gnome 3 proxy settings ignored

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Jan Horak [:jhorak]]

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 [:jhorak]]

Attachment: gsettings proxy patch v 1

Initial patch, not sure who to set as reviewer, feel free to reassign.

Comment 2

[Karl Tomlinson (:karlt)]

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 [:jhorak]]

Attachment: backtrace first run

Attaching backtrace of first run (when previous version of Firefox was executed last time).

Comment 4

[Jan Horak [:jhorak]]

Attachment: backtrace next run

Comment 5

[Jan Horak [:jhorak]]

Attachment: 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 (:karlt)]

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 (:karlt)]

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 [:jhorak]]

(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 (:karlt)]

(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 [:jhorak]]

Attachment: 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 (:karlt)]

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 (:karlt)]

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 [:jhorak]]

Attachment: 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 (:karlt)]

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 [:jhorak]]

Attachment: 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 (:karlt)]

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 [:jhorak]]

Attachment: gsettings proxy patch v5

Thanks for review, attaching patch with replaced PR_FALSE.

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2f33758829d2

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/2f33758829d2

Comment 20

[Theodore Lee]

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 (:karlt)]

We'll need bug 713802 to get support in nightlies.