Bug 68602

Summary: [soup] Move important SoupSession feature initialization to WebCore
Product: WebKit Reporter: Wajahat Siddiqui <mdwajahatali.siddiqui>
Component: WebCore Misc.Assignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Minor CC: a.butenka, amruthraj, danw, darin, gyuyoung.kim, mrobinson, pnormand, rakuco, ravi.kasibhatla, svillar, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
proposed patch
mrobinson: review-
Proxy support in webprocess
none
proposed patch updated
none
Patch
none
Patch mrobinson: review+, gyuyoung.kim: commit-queue-

Description Wajahat Siddiqui 2011-09-22 02:35:57 PDT
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.
Comment 1 Wajahat Siddiqui 2011-09-22 02:52:36 PDT
Created attachment 108301 [details]
proposed patch

Attaching Patch for review ?
Comment 2 WebKit Review Bot 2011-09-22 02:56:00 PDT
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.
Comment 3 Wajahat Siddiqui 2011-09-22 03:37:21 PDT
(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
Comment 4 Wajahat Siddiqui 2011-09-22 03:43:47 PDT
Just figured out that setting proxy is done in webkit from app side (GtkLauncher)

Proposed Patch will be applicable for both webkit1 and webkit2
Comment 5 Martin Robinson 2011-09-22 08:29:57 PDT
libproxy already knows how to read the environment to find proxy details, doesn't it?
Comment 6 Martin Robinson 2011-09-22 09:07:20 PDT
Comment on attachment 108301 [details]
proposed patch

r- for now, unless we are sure libproxy doesn't do this already.
Comment 7 Wajahat Siddiqui 2011-09-22 21:56:05 PDT
(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
Comment 8 Martin Robinson 2011-09-23 06:48:25 PDT
(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?
Comment 9 Wajahat Siddiqui 2011-09-26 00:40:18 PDT
(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
Comment 10 Wajahat Siddiqui 2011-09-26 00:43:17 PDT
Created attachment 108636 [details]
Proxy support in webprocess

Proposed patch that will add proxy support in webprocess
Comment 11 WebKit Review Bot 2011-09-26 00:46:09 PDT
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.
Comment 12 Wajahat Siddiqui 2011-09-26 00:52:51 PDT
Created attachment 108639 [details]
proposed patch updated

style errors fixed
Comment 13 Martin Robinson 2011-09-26 08:17:15 PDT
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.
Comment 14 Alexander Butenko 2012-01-16 06:12:18 PST
if this bug was raised again ill add that even with libproxy we still lacking socks4 and socks5 support.
Comment 15 Sergio Villar Senin 2012-02-14 10:15:33 PST
Created attachment 126994 [details]
Patch
Comment 16 Sergio Villar Senin 2012-02-14 10:25:06 PST
(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.
Comment 17 Martin Robinson 2012-02-14 10:41:21 PST
(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.
Comment 18 Martin Robinson 2012-02-14 16:11:37 PST
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.
Comment 19 WebKit Review Bot 2012-02-15 01:17:39 PST
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.
Comment 20 Sergio Villar Senin 2012-02-15 01:42:30 PST
Created attachment 127135 [details]
Patch
Comment 21 Gyuyoung Kim 2012-02-15 02:00:55 PST
Comment on attachment 127135 [details]
Patch

Attachment 127135 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11523636
Comment 22 Sergio Villar Senin 2012-02-15 02:07:31 PST
(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 ?
Comment 23 WebKit Review Bot 2012-02-15 04:05:52 PST
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.
Comment 24 Dan Winship 2012-02-15 06:23:48 PST
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).
Comment 25 Martin Robinson 2012-02-16 07:13:31 PST
Comment on attachment 127135 [details]
Patch

Looks good, but please include Dan's suggestion.
Comment 26 Sergio Villar Senin 2012-02-16 08:42:18 PST
Committed r107941: <http://trac.webkit.org/changeset/107941>
Comment 27 Philippe Normand 2012-02-16 12:32:33 PST
Reverted r107941 for reason:

Broke 23 http tests on GTK

Committed r107968: <http://trac.webkit.org/changeset/107968>
Comment 28 Sergio Villar Senin 2012-02-16 12:56:49 PST
Committed r107973: <http://trac.webkit.org/changeset/107973>
Comment 29 Sergio Villar Senin 2012-02-16 12:58:42 PST
(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.