Bug 25263 – [Gtk] WebKit GTK with libsoup won't recognize proxies

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

Environment:
    Debian 2.6.18 amd64
    latest libsoup & libproxy till now
    WebKit 1.1.5 (should also affect latest nightly build)

My computer connect with Internet through a HTTP proxy, which is set in the 'http_proxy' environment variable. 

I've been working on WebKit for months, at first, while I was using 1.0.1, (with curl as its backend), everything's all right. But recently I updated it to 1.1.5, and found that GtkLauncher could not load http://www.google.com.

It took me some time to realize that the network backend has been changed to libsoup. And I did test my libsoup, which worked well.

It took me another some time to compare WebCore/platform/network/soup/ResourceHandlSoup.cpp and test.c in libsoup tests. And at last my conclusion is maybe the proxy-resolver feature is not enabled. So I made a dirty patch:

1.in configure, add dependency rule of libsoup-gnome-2.4 (I failed in editing configure.ac and re-automake, for some unknown reason)

2.in WebCore/platform/network/soup/ResourceHandleSoup.cpp
    1) at the beginning, add
#include <libsoup/soup-gnome.h>

    2) at arount line 444, replace the body of function 'static SoupSession* createSoupSession()' with the following code

    return soup_session_async_new_with_options(SOUP_SESSION_ADD_FEATURE_BY_TYPE, SOUP_TYPE_GNOME_FEATURES_2_26, NULL);

I don't know whether it'll affect other parts of webkitgtk, but at least it works for me

so please check this, thx

Created attachment 29573 [details]
patch for enable proxy in libsoup

based on Webkit-r42569

[Jan Alonzo]

(In reply to comment #1)
> Created an attachment (id=29573) [review]
> patch for enable proxy in libsoup
> 
> based on Webkit-r42569
> 

This is good. For adding the configure option, you could just look at the LIBSOUP parts in configure.ac and add a LIBSOUP_GNOME and check for libsoup-gnome-2.4.

Please add a ChangeLog and set the review flag to "?" as well so reviewers can review your changes.

Thanks,

Created attachment 29611 [details]
to enable proxy-resolver feature in libsoup-gnome

Detect libsoup-gnome, and use it instead of libsoup, if exists.
If libsoup-gnome exists, enable SOUP_TYPE_GNOME_FEATURES_2_26 when creating a new soup session

[Xan Lopez]

I'm really not sure that this should be done in webkit, or that it should autodetect the library and enable the proxy without explicit setup (we are not doing this for anything that it's not a hard dependency AFAIK).

[Jan Alonzo]

Comment on attachment 29611 [details]
to enable proxy-resolver feature in libsoup-gnome

(In reply to comment #3)
> Created an attachment (id=29611) [review]
> to enable proxy-resolver feature in libsoup-gnome
> 
> Detect libsoup-gnome, and use it instead of libsoup, if exists.
> If libsoup-gnome exists, enable SOUP_TYPE_GNOME_FEATURES_2_26 when creating a
> new soup session

Since libsoup-gnome is not a hard-dependency, this feature should be configurable at configure time and should be disabled by default. I would suggest adding --with-gnome or --with-libsoup-gnome in configure.ac for this.

I'm going to say r- for now until have this feature configurable.

Created attachment 30529 [details]
enable libsoup-gnome features, default=no

this is based on r43933 because I can't build latest svn version, see https://bugs.webkit.org/show_bug.cgi?id=25910

Created attachment 30531 [details]
added an option to enable GNOME-specific features of libsoup, default=no

this is based on r43961, because latest svn version of webkitgtk  could not compile, see https://bugs.webkit.org/show_bug.cgi?id=25910

Created attachment 30535 [details]
added an option to enable GNOME-specific features of libsoup, default=no

this is based on r43961, since I couldn't compile the lastest svn version of webkitgtk, see https://bugs.webkit.org/show_bug.cgi?id=25910

besides, sorry for the last 2 failed patches, this one should work

[Jan Alonzo]

Comment on attachment 30535 [details]
added an option to enable GNOME-specific features of libsoup, default=no

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 43961)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,12 @@
> [phone]  WANG Lu <[e-mail]> 
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +		Added --with-libsoup-gnome (default=no) to enable GNOME-specific
> +		features of libsoup, an important one is the proxy-resovler feature

Please fix the spaces here.

> Index: configure.ac
> ===================================================================
> --- configure.ac	(revision 43933)
> +++ configure.ac	(working copy)
> @@ -497,7 +497,6 @@ AC_ARG_ENABLE([jit],
>  if test "$enable_jit" = "yes"; then
>      case "$host_cpu" in
>          i*86|x86_64)
> -            AC_DEFINE([ENABLE_JIT], [1], [Define to enable JIT])

Is this part of the patch? 

>              AC_DEFINE([ENABLE_YARR], [1], [Define to enable YARR])
>              AC_DEFINE([ENABLE_YARR_JIT], [1], [Define to enable YARR JIT])
>              AC_DEFINE([ENABLE_JIT_OPTIMIZE_CALL], [1], [Define to enable optimizing calls])
> @@ -550,10 +549,28 @@ else
>     CFLAGS="$CFLAGS -O0"
>  fi
>  
> -PKG_CHECK_MODULES([LIBSOUP],
> -                  [libsoup-2.4 >= $LIBSOUP_REQUIRED_VERSION])
> -AC_SUBST([LIBSOUP_CFLAGS])
> -AC_SUBST([LIBSOUP_LIBS])
> +# check whether to enable libsoup-gnome
> +AC_MSG_CHECKING([whether to use GNOME-specific features of libsoup])
> +AC_ARG_WITH(libsoup_gnome,
> +              AC_HELP_STRING([--with-libsoup-gnome],
> +                             [using GNOME-specific features of libsoup [default=no]]),
> +              [],[with_libsoup_gnome="no"])
> +AC_MSG_RESULT([$with_libsoup_gnome])
> +
> +if test "$with_libsoup_gnome" = "yes"; then
> +    PKG_CHECK_MODULES([LIBSOUP],
> +                      [libsoup-gnome-2.4 >= $LIBSOUP_REQUIRED_VERSION]) # FIXME: does libsoup-gnome need a separated REQUIRED_VERSION ?

FIXME is not required. They should be the same.

> +    AC_DEFINE([ENABLE_LIBSOUP_GNOME],[1],[Define to enable libsoup-gnome])

Please use USE_SOUP_GNOME and this should be AM_CONDITIONAL. And move this in the end of this file.

> +    AC_SUBST(LIBSOUPE_CFLAGS)
> +    AC_SUBST(LIBSOUPE_LIBS)

LIBSOUPE?

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 43961)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> [phone]  WANG Lu  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +		If libsoup-gnome is enabled, add proper parameters to enable
> +		GNOME-specific features while creating a new soup session.

Fix the indentation please.

> Index: WebCore/platform/network/soup/ResourceHandleSoup.cpp
> ===================================================================
> --- WebCore/platform/network/soup/ResourceHandleSoup.cpp	(revision 43933)
> +++ WebCore/platform/network/soup/ResourceHandleSoup.cpp	(working copy)
> @@ -48,7 +48,12 @@
>  #include <fcntl.h>
>  #include <gio/gio.h>
>  #include <gtk/gtk.h>
> +
> +#ifdef ENABLE_LIBSOUP_GNOME 

Should be USE_SOUP_GNOME per comment above. Then USE(SOUP_GNOME) here.

>  #include <libsoup/soup.h>
> +

Not required.

>  static SoupSession* createSoupSession()
>  {
> +#ifdef ENABLE_LIBSOUP_GNOME

USE(SOUP_GNOME) here.

> +    return soup_session_async_new_with_options(SOUP_SESSION_ADD_FEATURE_BY_TYPE, SOUP_TYPE_GNOME_FEATURES_2_26,
> +                                               NULL);

And put this all in one line please.

r- for now until the comments above have been addressed. 

Thanks!

Created attachment 30810 [details]
added an option to enable GNOME-specific features of libsoup, default=no

[Jan Alonzo]

Comment on attachment 30810 [details]
added an option to enable GNOME-specific features of libsoup, default=no

r=me

[Jan Alonzo]

Landed in r44479. 

Thanks a lot!

(In reply to comment #12)
> Landed in r44479. 
> 

[Xan Lopez]

I think this patch should be reverted, please see bug #26242 for rationale.

[Jan Alonzo]

(In reply to comment #14)
> I think this patch should be reverted, please see bug #26242 for rationale.

This currently defaults to 'no' so only people who want it can have it. Let's discuss better ways to handle optional features in the future in bug #26242, and fix whatever is needed to be fixed in that bug. I rather have people behind a proxy can still browse the web using webkitgtk today instead of waiting for a concensus on #26242 (we may hit a road block and decide to recommit this patch again, who knows). 

Can I close this bug again and track whatever needs to be fixed in bug #26242?

[Xan Lopez]

(In reply to comment #15)
> (In reply to comment #14)
> > I think this patch should be reverted, please see bug #26242 for rationale.
> 
> This currently defaults to 'no' so only people who want it can have it. Let's
> discuss better ways to handle optional features in the future in bug #26242,
> and fix whatever is needed to be fixed in that bug. I rather have people behind
> a proxy can still browse the web using webkitgtk today instead of waiting for a
> concensus on #26242 (we may hit a road block and decide to recommit this patch
> again, who knows).

I think this is ignoring the main point of the issue: the 'people' we care about here are distros, and they will have to make a choice if we make a release with this option. That will only add a new variable to the equation and make things worse, when it seems that we already have a consensus that what we are doing is wrong.

Besides, I believe this feature request in particular is slighltly artificial. I don't think GtkLauncher is a good use case for almost anything, and the WebKit apps that provide the hability to navigate the Web can trivially use the GNOME proxy settings if they want to (Epiphany is, for example). So it's not like reverting this patch will prevent people from using proxies, it will only put us back where we were before, when they have to manually request it in their applications.

[Jan Alonzo]

(In reply to comment #16)
> 
> I think this is ignoring the main point of the issue: the 'people' we care
> about here are distros, and they will have to make a choice if we make a
> release with this option. That will only add a new variable to the equation and
> make things worse, when it seems that we already have a consensus that what we
> are doing is wrong.

Honestly, I wasn't aware of any concensus. I was assuming you're all OK with this since there were no comments since the patched was marked as r?
> 
> Besides, I believe this feature request in particular is slighltly artificial.
> I don't think GtkLauncher is a good use case for almost anything, and the
> WebKit apps that provide the hability to navigate the Web can trivially use the
> GNOME proxy settings if they want to (Epiphany is, for example). So it's not

How about mobile platforms that want to use WebKitGtk? Or users that don't want GNOME but want to use WebKitGtk? 

[Xan Lopez]

(In reply to comment #17)
> > Besides, I believe this feature request in particular is slighltly artificial.
> > I don't think GtkLauncher is a good use case for almost anything, and the
> > WebKit apps that provide the hability to navigate the Web can trivially use the
> > GNOME proxy settings if they want to (Epiphany is, for example). So it's not
> 
> How about mobile platforms that want to use WebKitGtk? Or users that don't want
> GNOME but want to use WebKitGtk? 
> 

I'm not sure I get what you mean. If they don't want to use GNOME I guess they won't have much use for a --with-libsoup-gnome option, no matter if it's in WebKit or elsewhere. The only thing I'm saying is that whoever was interested in this configure option in WebKit can trivially replicate the functionality in his own application, like Epiphany is doing. If you are not interested you don't have to do anything.

[Jan Alonzo]

Comment on attachment 30810 [details]
added an option to enable GNOME-specific features of libsoup, default=no

Clearing r+, setting it back to r?. Patch reverted in r44494 as suggested in this bug.

[Xan Lopez]

Comment on attachment 30810 [details]
added an option to enable GNOME-specific features of libsoup, default=no

I think we can clear the review flag here for now?

[Christian Dywan]

I think this is overkill. Enabling proxy server support or all GNOME features is a one-liner per GNOME application. And in my opinion, libsoup should have a SOUP_TYPE_PROXY that uses whatever is available without requiring everything at build time. Or something that takes a string argument.

[Christian Dywan]

Concluding it is a one-liner to add the GNOME proxy resolver to GNOME applications, we shouldn't do that in WebKitGTK+. And http_proxy can be handled in libsoup, I filed https://bugzilla.gnome.org/show_bug.cgi?id=605048 for that. So this is resolved as far as WebKitGTK+ is concerned.