Bug 120833

Summary: [GTK] Desktop proxy settings are ignored inside the internal jhbuild
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cgarcia, commit-queue, gustavo, rniwa, svillar, zan
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch proposal
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Patch proposal none

Description Mario Sanchez Prada 2013-09-06 02:34:32 PDT
Currently, regardless of which proxy configuration you have defined in your desktop settings (none, auto or manual), tools run inside the WebKitGTK+'s internal jhbuild environment (e.g. GtkLauncher, MiniBrowser...) will always ignore them and assume 'none', making it impossible to run those tools easily in certain environment (e.g. restricted corporate networks).

For instance, if I try to run GtkLauncher like this from here:

  $ Tools/Scripts/run-launcher --gtk http://www.asdf.com
  

All I get is GtkLauncher trying to load that web site forever, since it should use the proxy settings I've defined in my system and that I can check from command line through gsettings:

  $ gsettings get 'org.gnome.system.proxy' 'mode'
  'auto'

  $ gsettings get 'org.gnome.system.proxy' 'autoconfig-url'
  'http://<super-secret-IP>:<super-secret-PORT>/proxy.pac'


However, if I run the same commands inside the jhbuild shell (run with Tools/jhbuild/jhbuild-wrapper --gtk shell), all I get is this:

  $ gsettings get 'org.gnome.system.proxy' 'mode'
  'none'

  $ gsettings get 'org.gnome.system.proxy' 'autoconfig-url'
  ''

So the problem seems to be that we are using the GSettings memory backend instead the dconf one, and we are not getting any warning such as "GLib-GIO-Message: Using the 'memory' GSettings backend." because we are explicitly setting the memory backend in our jhbuild environment. As from Tools/gtk/jhbuildrc file:

  [...]
  # We often end up using the memory backend anyway, so explicitly choosing
  # it will prevent the warning message.
  os.environ['GSETTINGS_BACKEND'] = 'memory'
  [...]


So, IMHO the solution to this issue would be to make sure we build the dconf module inside the internal jhbuild (so we can use the gsettings dconf backend) and make it the default backend in jhbuildrc. That way we will not only be able to use our tools properly behind a proxy, but we also will be able to access any other configuration options from gsettings in case we need them (and potentially uncover other issues we might be having because of that).

I'll be providing a patch soon for that.
Comment 1 Zan Dobersek 2013-09-06 02:48:47 PDT
I guess the dconf module will be optional?

Could the memory backend somehow be populated with the necessary information when the Jhbuild environment is initialized? For instance passing values for specifically listed keys from the outside environment's settings into the fresh memory backend using `gsettings set ...`?
Comment 2 Mario Sanchez Prada 2013-09-06 03:09:35 PDT
Created attachment 210715 [details]
Patch proposal

This is the patch implementing the proposed solution.

I've tested it extensively here and works great, responding as expected in all possible proxy situations: none, auto and manual. This patch fixes such a painful situation when developed behind a proxy, so I personally would appreciate if we could get it reviewed soon :-D

/me grins twice
Comment 3 Sergio Villar Senin 2013-09-06 03:34:28 PDT
(In reply to comment #2)
> Created an attachment (id=210715) [details]
> Patch proposal
> 
> This is the patch implementing the proposed solution.
> 
> I've tested it extensively here and works great, responding as expected in all possible proxy situations: none, auto and manual. This patch fixes such a painful situation when developed behind a proxy, so I personally would appreciate if we could get it reviewed soon :-D
> 
> /me grins twice

As commented in private IRC I'd prefer not to add dconf and instead add something to the jhbuildrc (likely using gsettings gobject introspection) to read the proxy settings (from the system or from the "classic" http_proxy env var) and update the gsettings memory backend.
Comment 4 Build Bot 2013-09-06 05:59:32 PDT
Comment on attachment 210715 [details]
Patch proposal

Attachment 210715 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1714148

New failing tests:
media/video-object-fit.html
Comment 5 Build Bot 2013-09-06 05:59:34 PDT
Created attachment 210724 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4
Comment 6 Mario Sanchez Prada 2013-09-06 06:21:59 PDT
(In reply to comment #3)
> [...]
> As commented in private IRC I'd prefer not to add dconf and instead add
> something to the jhbuildrc (likely using gsettings gobject introspection)
> to read the proxy settings (from the system or from the "classic"
> http_proxy env var) and update the gsettings memory backend.

We already discussed that idea here but the problem we found is that checking the http_proxy env var won't fix the problem if you don't have it set up in your system, even when you have the desktop proxy settings set in gsettings.

Besides, that will work fine as long as you are ok with setting up an http(s) proxy, but we would need to add support for more variables if we wanted to support other things, such as autoconfig URLs (e.g. auto_proxy env var).

In any case, after realizing that the original approach has not been very well received, perhaps I should give this idea a try and see how it works.
Comment 7 Mario Sanchez Prada 2013-09-06 10:18:02 PDT
Created attachment 210766 [details]
Patch proposal

After some investigation, I realized that we can not use the trick of setting some configuration values in the memory backend from jhbuildrc (as suggested, based on the presence of the typical environment variables) because the memory backend provides in-process persistence only, and the jhbuild shell is not actually the process we are interested in, but the apps we launch in that environment (e.g. MiniBrowser), which won't read anything we set from jhbuildrc.

So, after talking to Gustavo in IRC, we believe that a better approach that will make lives easier for everybody without compromising the default configuration, would be to add dconf as an optional jhbuild module AND remove the line from jhbuildrc that explicitly sets the memory backend just for the sake of removing the usual warning you will see letting you know about that.

That way, someone willing to use the default behaviour won't have to do anything different and just will get that warning (which IMHO is useful anyway, to remind you what you are using), and someone willing to have access to the global settings will just have to run Tools/jhbuild/jhbuild-wrapper --gtk build dconf previously, pretty much as it's mentioned in https://trac.webkit.org/wiki/WebKitGTK/StartHacking#PersistentGSettings.
Comment 8 Gustavo Noronha (kov) 2013-09-06 10:21:51 PDT
Comment on attachment 210766 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=210766&action=review

> Tools/gtk/jhbuildrc:-61
> -# We often end up using the memory backend anyway, so explicitly choosing
> -# it will prevent the warning message.
> -os.environ['GSETTINGS_BACKEND'] = 'memory'
> -

Maybe we could convince the gsettings people to drop or provide a way to disable the warning, but it's not that big of a problem.
Comment 9 Mario Sanchez Prada 2013-09-06 10:30:02 PDT
Comment on attachment 210766 [details]
Patch proposal

Thanks Gustavo!
Comment 10 WebKit Commit Bot 2013-09-06 11:18:45 PDT
Comment on attachment 210766 [details]
Patch proposal

Clearing flags on attachment: 210766

Committed r155197: <http://trac.webkit.org/changeset/155197>
Comment 11 WebKit Commit Bot 2013-09-06 11:18:48 PDT
All reviewed patches have been landed.  Closing bug.