Bug 68602 – [soup] Move important SoupSession feature initialization to WebCore

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wajahat Siddiqui]

It is good to have soup to use proxy from 'http_proxy' environment variable 
when it is exported either from ui or manually.

This will be very useful for people working on machines connected to proxy network.

[Wajahat Siddiqui]

Created attachment 108301 [details]
proposed patch

Attaching Patch for review ?

[WebKit Review Bot]

Attachment 108301 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:881:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wajahat Siddiqui]

(In reply to comment #2)
> Attachment 108301 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:881:  Use 0 instead of NULL.  [readability/null] [5]
> Total errors found: 1 in 2 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I think these are false positives as in g_object_set() we use NULL instead of 0

[Wajahat Siddiqui]

Just figured out that setting proxy is done in webkit from app side (GtkLauncher)

Proposed Patch will be applicable for both webkit1 and webkit2

[Martin Robinson]

libproxy already knows how to read the environment to find proxy details, doesn't it?

[Martin Robinson]

Comment on attachment 108301 [details]
proposed patch

r- for now, unless we are sure libproxy doesn't do this already.

[Wajahat Siddiqui]

(In reply to comment #6)
> (From update of attachment 108301 [details])
> r- for now, unless we are sure libproxy doesn't do this already.

In case of Minibrowser, it does not do. It does only for GtkLauncher in 

Tools/GtkLauncher/main.c which is gauded by #ifndef WEBKIT2 ???

...
#ifndef WEBKIT2
#ifdef SOUP_TYPE_PROXY_RESOLVER_DEFAULT
    soup_session_add_feature_by_type(webkit_get_default_session(), SOUP_TYPE_PROXY_RESOLVER_DEFAULT);
#else
    const char *httpProxy = g_getenv("http_proxy");
    if (httpProxy) {
        SoupURI *proxyUri = soup_uri_new(httpProxy);
        g_object_set(webkit_get_default_session(), SOUP_SESSION_PROXY_URI, proxyUri, NULL);
        soup_uri_free(proxyUri);
    }
#endif
#endif
...
 
I think we need to set proxy at a common place in WebCore while calling ResourceHandle::defaultSession() instead at the app level if there are no design issues with this change ? This will be applicable for both GtkLauncher and Minibrowser

[Martin Robinson]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 108301 [details] [details])
> > r- for now, unless we are sure libproxy doesn't do this already.
> 
> In case of Minibrowser, it does not do. It does only for GtkLauncher in 

Could it be that the WebProcess is just not properly inheriting the parent's environment?

[Wajahat Siddiqui]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 108301 [details] [details] [details])
> > > r- for now, unless we are sure libproxy doesn't do this already.
> > 
> > In case of Minibrowser, it does not do. It does only for GtkLauncher in 
> 
> Could it be that the WebProcess is just not properly inheriting the parent's environment?

No it does not and other ports like Elf does add proxy from environment in case of WK2

[Wajahat Siddiqui]

Created attachment 108636 [details]
Proxy support in webprocess

Proposed patch that will add proxy support in webprocess

[WebKit Review Bot]

Attachment 108636 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wajahat Siddiqui]

Created attachment 108639 [details]
proposed patch updated

style errors fixed

[Martin Robinson]

Comment on attachment 108639 [details]
proposed patch updated

I feel like this is still just a work-around. Soup knows how to integrate with libproxy -- which is how we should be reading proxy variables. Even this patch misses https_proxy, etc. We need to find a reasonable way to have Soup use SOUP_TYPE_PROXY_RESOLVER_DEFAULT without reimplementing libproxy in the WebProcess and without depending on Gnome libraries in WebKit.

[Alexander Butenko]

if this bug was raised again ill add that even with libproxy we still lacking socks4 and socks5 support.

[Sergio Villar Senin]

Created attachment 126994 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #13)
> (From update of attachment 108639 [details])
> I feel like this is still just a work-around. Soup knows how to integrate with libproxy -- which is how we should be reading proxy variables. Even this patch misses https_proxy, etc. We need to find a reasonable way to have Soup use SOUP_TYPE_PROXY_RESOLVER_DEFAULT without reimplementing libproxy in the WebProcess and without depending on Gnome libraries in WebKit.

So, as Martin says, libproxy perfectly handles different types of proxy configurations. In the patch I uploaded a couple of minutes ago I'm adding the default soup proxy resolver (I understand that we do not want to depend on libsoup-gnome) to the SoupSession used by the WebProcess. That default proxy resolver will use gio to handle all the proxy stuff.

That means that gio will delegate all the proxy requests to the default module it has registered for proxy resolution. We can find two of those modules in the glib-networking library libsoup depends on. Those modules are libgiognomeproxy (that uses the GNOME network infrastructure to get the proxy) and libgiolibproxy (able to resolve proxy's using different methods, from gnome stuff to environment variables). If both of them are present it will use the GNOME one because it has a higher priority (although that can be changed using the GIO_USE_PROXY_RESOLVER environment variable).

Adding Dan to the loop as he might want to correct me if I was wrong.

[Martin Robinson]

(In reply to comment #16)
> So, as Martin says, libproxy perfectly handles different types of proxy configurations. In the patch I uploaded a couple of minutes ago I'm adding the default soup proxy resolver (I understand that we do not want to depend on libsoup-gnome) to the SoupSession used by the WebProcess. That default proxy resolver will use gio to handle all the proxy stuff.

After talking, Sergio and I agreed that it would probably be better to move important SoupSession feature initialization to WebCore since WebCore requires some of these features. This should clear up the issue as a side-effect of the change.

[Martin Robinson]

Comment on attachment 108639 [details]
proposed patch updated

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

> Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:63
>      soup_session_add_feature_by_type(session, WEB_TYPE_AUTH_DIALOG);

I think the only thing we cannot move here is WEB_TYPE_AUTH_DIALOG, as that depends on GTK+. I'd like to get rid of it anyhow, so that we can expose an asynchronous API.

[WebKit Review Bot]

Attachment 126994 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   fb50dbd..a9730c4  master     -> origin/master
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168753 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/wk2/Skipped
Auto-merging Source/WebCore/ChangeLog
Auto-merging Source/WebCore/css/CSSCalculationValue.cpp
Auto-merging Source/WebCore/css/CSSCalculationValue.h
Auto-merging Source/WebCore/css/CSSParser.cpp
Auto-merging Source/WebKit/mac/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergio Villar Senin]

Created attachment 127135 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 127135 [details]
Patch

Attachment 127135 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11523636

[Sergio Villar Senin]

(In reply to comment #21)
> (From update of attachment 127135 [details])
> Attachment 127135 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/11523636

What version of libsoup are you using Kim ?

[WebKit Review Bot]

Attachment 127135 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 0dfd183742a71cb5de5dadc3ae177fc72b63a194 != 9cdcda984def14b8bf8a32b6da6784c8a6ef7b3a
rereading 8567f8d3c2539a28a496edaf1048483e973975c2
	M	LayoutTests/fast/forms/radio-nested-labels.html
	M	LayoutTests/ChangeLog
107798 = 3671b2d23de7ade4cb1d1e78a3f6f7673db6a6c9 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dan Winship]

makes sense to me

I'd also suggest

  SOUP_SESSION_USE_SYSTEM_CA_FILE, TRUE,

to make it default to validating https certificates against the system CA list (and remove the code from epiphany [and presumably midori, etc] that loads that list by hand).

[Martin Robinson]

Comment on attachment 127135 [details]
Patch

Looks good, but please include Dan's suggestion.

[Sergio Villar Senin]

Committed r107941: <http://trac.webkit.org/changeset/107941>

[Philippe Normand]

Reverted r107941 for reason:

Broke 23 http tests on GTK

Committed r107968: <http://trac.webkit.org/changeset/107968>

[Sergio Villar Senin]

Committed r107973: <http://trac.webkit.org/changeset/107973>

[Sergio Villar Senin]

(In reply to comment #24)
> makes sense to me
> 
> I'd also suggest
> 
>   SOUP_SESSION_USE_SYSTEM_CA_FILE, TRUE,
> 
> to make it default to validating https certificates against the system CA list (and remove the code from epiphany [and presumably midori, etc] that loads that list by hand).

I had finally landed the patch without this as many http tests (related to SSL) started to fail on bots. Will take a look later why this happened, but as it's a different issue I decided to upload the original patch as is.