Add support for respecting system proxy settings

[andrunko]

Ok, this is an attempt to implement support for respecting system proxy settings on the flatpak version.

Some general notes on implementation:

• The implementation uses the same codepath used when --winhttp-proxy-resolver is passed as param, which uses the net::ProxyResolver interface to determine the proxy given an url:
  • This is due to limitations in the portal APIs which don't allow access to the system proxy settings, and with that no reliable way to implement a proper ProxyConfigService when on flatpak
  • Despite the name of the param, this flag is not Windows(tm) specific and also used on Mac builds
  • This param may be removed at some point in the future, see https://bugs.chromium.org/p/chromium/issues/detail?id=644030 but it should be replaced with https://bugs.chromium.org/p/chromium/issues/detail?id=1032820, which from the looks of it would not require too many changes to the impl here
  • To avoid having to pass this param I've enabled this codepath automatically when detecting we are running on flatpak
• This means that proxy resolution is done by invoking net::ProxyResolver::GetProxyForURL() (instead of relying on the internal chromium mechanism based on the proxy settings, set to "mode=auto" in this case), which in turn invokes g_proxy_resolver_lookup which uses the portal API via D-Bus
  • To avoid having one D-Bus call per net::ProxyResolver::GetProxyForURL(), I've implemented a (very) simple cache that gets invalidated every 1min (same default timeout used on chromium dns refresh)
• Another issue is that the impl will invoke the portal even if proxy is disabled, also because of a limitation in the portal API where there is no way to know whether the proxy is disabled
• We could skip using the portal on KDE, given we have access to $HOME today (and the impl just reads .ini files), but I decided to go with the same impl when running on flatpak, but have no hard-preference here

On the bright side, it seems to work fine from my tests (could use some more tests though) and changes to the system settings (tested on GNOME/Endless) are reflected correctly (considering the cache invalidation timeout of 1min).

I am not really happy with the amount of D-Bus calls trigerred here tbh (esp. when proxy is disabled), but I can't think of another way to implement this with the current portal APIs - we could tweak the cache invalidation timeout but apart from that I don't think we could do much to improve.

See flatpak/xdg-desktop-portal#554 for the portal API issues/limitations mentioned above.

Thoughts?

Closes #36

[flathubbot]

Started test build 34844

[flathubbot]

Build 34844 failed

[flathubbot]

Started test build 34845

[flathubbot]

Build 34845 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/33556/org.chromium.Chromium.flatpakref

[refi64]

Question: do we actually need a separate thread here? GLib declares global structures to be thread-safe, so I believe g_proxy_resolver_lookup_async could be called from the same thread, locking the cache if necessary.

[andrunko]

Tbh I wasn't sure either and on a quick look at the code (portal impls) I couldn't confirm so I went with the safest option. Maybe @pwithnall would be able to confirm/clarify here.

[pwithnall]

@refi64 is right: you should be able to expect that the proxy resolver returned by g_proxy_resolver_get_default() is thread-safe.

The gsimpleproxyresolver.c implementation is thread-safe as long as there are no calls to g_simple_proxy_resolver_set_*() after any threads have been spawned.

The gproxyresolverportal.c implementation is entirely thread-safe, because it just passes calls through to a GDBusProxy around the portal, which is thread-safe.

The gproxyresolvergnome.c implementation (in glib-networking) looks like it’s thread-safe (it has a mutex), but I haven’t checked it thoroughly.

The glibproxyresolver.c (libproxy) implementation (in glib-networking) also looks like it has thought about threading, because it already does its libproxy calls in a worker thread.

tl;dr: You should be able to drop your worker thread here. If any threading bugs do appear, they are likely bugs in GLib; but I wouldn’t expect any to appear if you’re using any implementation except gsimpleproxyresolver.c. It would be unlikely you’d end up using that implementation, as it’s a fallback.

[andrunko]

Thanks @pwithnall, @refi64. I've updated the impl to drop the glib worker thread. Some possible improvements using current GProxyResolver/Portal interface APIs (for the future - I don't plan to work on them for the time being):

• avoid calls to g_proxy_resolver_lookup() if there is an existing inflight request for the same url
  • although net::ProxyResolver::GetProxyForURL() is always invoked from the same thread for the same net::ProxyResolver instance, different instances may be invoked from different threads but still use the same GProxyResolver instance...
• using a single/global cache for all proxy resolver instances
  • as above, given we use the same GProxyResolver instance behind the sceces, we could consider a global cache
• make cache smarter
• allow async net::ProxyResolver::GetProxyForURL() calls (although currently the only consumer is net::MultiThreadProxyResolver which always expects a sync call)

[flathubbot]

Started test build 35109

[flathubbot]

Build 35109 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/33804/org.chromium.Chromium.flatpakref