Bug 91747

Thiago. Just so you know, I discussed this issue with the libsoup maintainer and he would be willing to accept a patch to that libsoup does not try to use the proxy for localhost. I'm looking into this. This is not exactly the same thing as you want, but I have an idea how to do the no_proxy thing too.

(In reply to comment #1) > Thiago. Just so you know, I discussed this issue with the libsoup maintainer and he would be willing to accept a patch to that libsoup does not try to use the proxy for localhost. I'm looking into this. > > This is not exactly the same thing as you want, but I have an idea how to do the no_proxy thing too. Honoring the "no_proxy" maybe might not be the best solution, agree. Now that I have a more clear understanding of the problem, what I think should be done is the implementation of ProxyServer.h interface for libsoup. In a second patch, we can make it configurable via an API at the UIProcess. Anyway, I think this work is independent from patching libsoup.

Thiago, since this is libsoup oriented. Do you mind if I take over? I have experience with libsoup and I know how to proceed.

All yours. :)

Created attachment 153433 [details] Patch

Comment on attachment 153433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153433&action=review I wonder whether can we share this file with GTK port ? Anyway, if you wanna use XXXEfl.cpp/h, I think you need to reduce glib variable type. > Source/WebKit2/WebProcess/efl/SoupProxyResolverEfl.cpp:128 > +static gboolean idle_return_proxy_uri(void* data) In EFL port, we don't use glib type. s/gboolean/bool/g ?

Comment on attachment 153433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153433&action=review No need to share with GTK port since they are using the GNOME resolver which uses GNOME settings. Also I'm not sure how I can get rid of "g" types here. It has to be a GObject and we are using libsoup. I'm already not using the g_* functions. >> Source/WebKit2/WebProcess/efl/SoupProxyResolverEfl.cpp:128 >> +static gboolean idle_return_proxy_uri(void* data) > > In EFL port, we don't use glib type. s/gboolean/bool/g ? No, this is a callback for soup_add_completion() so I have to use gboolean here.

Comment on attachment 153433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153433&action=review If so, I can understand. BTW, as you know, EFL port needs to use our network library based on EFL library. But, it looks there is no network library in EFL. I hope EFL also supports network library in near future. > Source/WebKit2/WebProcess/efl/SoupProxyResolverEfl.cpp:142 > + return FALSE; Is this one can't be replaced with false ?

(In reply to comment #8) > (From update of attachment 153433 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153433&action=review > > If so, I can understand. BTW, as you know, EFL port needs to use our network library based on EFL library. But, it looks there is no network library in EFL. I hope EFL also supports network library in near future. > > > Source/WebKit2/WebProcess/efl/SoupProxyResolverEfl.cpp:142 > > + return FALSE; > > Is this one can't be replaced with false ? Yes, I think this one can be replaced. I'll make another pass and get rid of as much glib'ism as possible.

Created attachment 153446 [details] Patch

Comment on attachment 153433 [details] Patch Thanks for the patch. IMO in addition to no_proxy, you should add localhost and 127.0.0.1 as non-proxy'ed domains by default. no_proxy is not exactly a standard and everyone running the unit tests will have them timeouting util realizing that these 2 domains have to be exported to the environment via no_proxy. Another thing is the Resolver code fits better at WebCore/platform/network/efl/ or maybe WebCore/platform/network/soup/efl/. It is purely network code and it could be reused by WK1.

I am not sure whether proxy_resolver works well in tizen. Anyway, now we are controlling proxy using API like webkit1/efl. Add keunsoon in CCs. keunsoon, can you check this ?

Created attachment 153452 [details] Patch

Comment on attachment 153452 [details] Patch Will use the resolver in WK1 as well.

Created attachment 153454 [details] Patch Use the proxy resolver in WebKit1 as well.

(In reply to comment #12) > I am not sure whether proxy_resolver works well in tizen. > > Anyway, now we are controlling proxy using API like webkit1/efl. > > Add keunsoon in CCs. > keunsoon, can you check this ? Sure, the proxy_resolver will work. This is internal libsoup stuff so it is unrelated to tizen. Just because you are using an API for controlling the proxy does not mean that the resolver cannot be used. The API can set/get the proxy settings to/from the resolver as shown in my patch for the WK1/EFL API. We could even add an API in WK1-EFL to add proxy exceptions now (if we want to). In any case, that would be in a separate patch.

Comment on attachment 153454 [details] Patch LGTM, thanks!

I'm not agree this patch because there can be multiple applications to use WebKit with differenct proxy address. I guess every application using WebKit should do export to set proxy address with your patch, am I right? (Fix me if I'm wrong.) And, is it possible they set their own proxy address without 'export'? One more thing, how does act ewk_proxy_set/get() with your patch?

Comment on attachment 153454 [details] Patch Somehow the proxy does not get ignored by default for localhost. I need to debug this.

Comment on attachment 153454 [details] Patch Nevermind, it is working. Setting r? again. export no_proxy="" means no exception unset no_proxy means default behavior (localhost as exception).

(In reply to comment #18) > I'm not agree this patch because there can be multiple applications to use WebKit with differenct proxy address. > > I guess every application using WebKit should do export to set proxy address with your patch, am I right? (Fix me if I'm wrong.) Ok, I fix you :) My patch is not really related to the environment variables. My patch adds a proxy resolver which can be used to customize the behavior (in particular, add proxy exceptions which was not supported before). As you can see from my patch, the resolver can be used by the ewk_network_proxy_set/get() API just fine, without the need for environment variables. > And, is it possible they set their own proxy address without 'export'? With WK1-EFL, you need to use the ewk_network_proxy_set/get() API, as you did before my patch. With WK2-EFL, you need to export the environment variables, as you did before my patch. The reason is that there is no ewk_network_proxy_set/get() API in WK2-EFL yet (as stated by Thiago). This should be addressed in a separate patch. For now, the only visible difference with my patch is that the proxy is not used by default for "localhost,127,0,0,1". This *is* needed for our unit tests to pass on a machine that has an HTTP proxy set. Another benefit of my patch is that we can add support for a ewk_network_proxy_exceptions_set()/get() API later.

Comment on attachment 153454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153454&action=review Can't all this be written so that it can be reused by as many ports as possible? Ie. keep the soup dependencies as minimal as possible? I am pretty sure Qt also doesn't parse the environment variables correctly... so at least that parsing part can be shared across EFL, GTK and Qt. > Source/WebCore/platform/network/soup/efl/SoupProxyResolverEfl.cpp:88 > + priv->proxyExceptions.clear(); > + String::fromUTF8(priv->noProxy.data()).split(',', priv->proxyExceptions); > + break; wget used to have a bug that it didn't remove spaces. You seem to have the same problem here > Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:71 > - const char* httpProxy = g_getenv("http_proxy"); > + const char* httpProxy = getenv("http_proxy"); What about https_proxy? I know some also use all_proxy for setting both http_proxy and https_proxy

Comment on attachment 153454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153454&action=review The parsing of the no_proxy environment variable is once line of code so I'm not sure how useful it would be to move the parsing out of SoupProxyResolverEfl and into a generic class. There is really no other parsing involved here. SoupProxyResolverEfl is actually just a lot of GObject boiler-plate code which is required by libsoup. >> Source/WebCore/platform/network/soup/efl/SoupProxyResolverEfl.cpp:88 >> + break; > > wget used to have a bug that it didn't remove spaces. You seem to have the same problem here Good point. I'll fix this now. >> Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp:71 >> + const char* httpProxy = getenv("http_proxy"); > > What about https_proxy? I know some also use all_proxy for setting both http_proxy and https_proxy There is not 100% related to this patch (adding support for "no_proxy"). Sure, we have a limitation: currently, it is not possible to set a different proxy for http and https (the http_proxy will be used for both). However, I'm not sure how useful this would be really. If we do think there is a need, it can be easily added in a separate patch later (since this is not strictly related to my bug).

Created attachment 153466 [details] Patch Correctly handle white spaces in the http_proxy environment variable, as spotted by Kenneth.

(In reply to comment #24) > Created an attachment (id=153466) [details] > Patch > > Correctly handle white spaces in the http_proxy environment variable, as spotted by Kenneth. I meant in "no_proxy" environment variable, sorry.

Christophe, thank you for fixing me and sorry for my humble understanding. I have several more question. I can understand getting environment is a kind of temporary solution until creating ewk_network_proxy_get/set() on WK2. Then, what is default proxy address if application does not set any environment variable for proxy on your latest patch? I'm sorry but I'm confusing it is localhost or system's default (even if it is not adopted to evironment variable.). And how is it going if proxy address is changed while loading something? Loaded libsoup session is canceled automatically or it is a another issue besides of this? I like your idea but I hope my question can help you to improve your patch.

(In reply to comment #26) > Christophe, thank you for fixing me and sorry for my humble understanding. > > I have several more question. > > I can understand getting environment is a kind of temporary solution until creating ewk_network_proxy_get/set() on WK2. Then, what is default proxy address if application does not set any environment variable for proxy on your latest patch? I'm sorry but I'm confusing it is localhost or system's default (even if it is not adopted to evironment variable.). Yes, I agree that the environment variable is a temporary solution until we have a proper API. However, note that my patch has NO impact on which proxy is being used. You seem to be talking about WK2 so here is what happens with and without my patch: - If the user has set the http_proxy variable before the WebProcess is spawned, then it will be used. - If the user has not set the http_proxy variable before the WebProcess is spawned, then NO proxy will be used. Here is what my patch changes: - If the user has set the http_proxy variable before the WebProcess is spawned, then it will be used, except if the destination URI is 127.0.0.1/localhost (because it is usually pointless). In addition, the user can set the "no_proxy" environment variable to something like: export no_proxy="localhost,127.0.0.1,.intel.com" in order to disable the proxy for localhost and intel.com domain. > > And how is it going if proxy address is changed while loading something? Loaded libsoup session is canceled automatically or it is a another issue besides of this? Currently, the http_proxy environment variable is only checked when the WebProcess is spawned. Therefore, if the client changes the http_proxy during run time, the change will only be taken into account for new WebProcesses (e.g. for new tabs opened by the client). We cannot get any notifications on environment variable changes. Yes, this is a limitation when using the environment variables. But then again, this behavior is not introduced by my patch and we know that the environment variable usage is temporary for WK2-EFL. Once we have a proper Ewk API for setting the proxy, this issue will be resolved.

I think you should keep the environment variable support even after you get api for the browser itself. It can be quite useful for simple debugging etc and it should override what the application sets in that case. like `http_proxy=http... MiniBrowser`

Comment on attachment 153466 [details] Patch I am ok with this, though I still think it should not necessarily be EFL specific but just Soup specific. EFL can then use it, but GTK can decide whether they want or not. This is soup and not really EFL specific

(In reply to comment #27) > (In reply to comment #26) > > Christophe, thank you for fixing me and sorry for my humble understanding. > > > > I have several more question. > > > > I can understand getting environment is a kind of temporary solution until creating ewk_network_proxy_get/set() on WK2. Then, what is default proxy address if application does not set any environment variable for proxy on your latest patch? I'm sorry but I'm confusing it is localhost or system's default (even if it is not adopted to evironment variable.). > > Yes, I agree that the environment variable is a temporary solution until we have a proper API. > > However, note that my patch has NO impact on which proxy is being used. You seem to be talking about WK2 so here is what happens with and without my patch: > - If the user has set the http_proxy variable before the WebProcess is spawned, then it will be used. > - If the user has not set the http_proxy variable before the WebProcess is spawned, then NO proxy will be used. > > Here is what my patch changes: > - If the user has set the http_proxy variable before the WebProcess is spawned, then it will be used, except if the destination URI is 127.0.0.1/localhost (because it is usually pointless). > In addition, the user can set the "no_proxy" environment variable to something like: export no_proxy="localhost,127.0.0.1,.intel.com" in order to disable the proxy for localhost and intel.com domain. > Thank you for your explain. But I still cannot get the answer for default proxy if there is no setting for proxy. Could you mention this for stupid me? Here is my question again. If some application does not set http_proxy and no_proxy variable at all before spawning WebProcess, and it also does not call any ewk_network_proxy_set(). Then how WebKit does behave? I guess SoupProxyURIResolver is not created and inserted to libsoup as soup session feature, so, it is upto libsoup's default behavior, am I right? (Fix me again if I'm wrong.) And one more question for patch's structure. SoupProxyURIResolver on your patch is implementing soup session feature's interface, which has no dependency to Efl port. If so, I guess it is better to put Soup port rather than Efl port. What do you think so? Thank you in advance.

Comment on attachment 153466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153466&action=review > Source/WebCore/ChangeLog:20 > + * platform/network/soup/efl/SoupProxyResolverEfl.cpp: Added. Can't we not just add a network/soup/ProxyResolver.cpp?

(In reply to comment #31) > (From update of attachment 153466 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153466&action=review > > > Source/WebCore/ChangeLog:20 > > + * platform/network/soup/efl/SoupProxyResolverEfl.cpp: Added. > > Can't we not just add a network/soup/ProxyResolver.cpp? Ok, I'll do that :)

> Thank you for your explain. But I still cannot get the answer for default proxy if there is no setting for proxy. Could you mention this for stupid me? > Here is my question again. If some application does not set http_proxy and no_proxy variable at all before spawning WebProcess, and it also does not call any ewk_network_proxy_set(). Then how WebKit does behave? I guess SoupProxyURIResolver is not created and inserted to libsoup as soup session feature, so, it is upto libsoup's default behavior, am I right? (Fix me again if I'm wrong.) You are right, if the http_proxy environment variable is not set and if the application does not call ewk_network_proxy_set(), then the SoupProxyURIResolver is not created and it becomes libsoup's default behavior. Is that a problem? As far as I understand, it was the same behavior before my patch. > > And one more question for patch's structure. > SoupProxyURIResolver on your patch is implementing soup session feature's interface, which has no dependency to Efl port. If so, I guess it is better to put Soup port rather than Efl port. What do you think so? Agreed.

(In reply to comment #33) > You are right, if the http_proxy environment variable is not set and if the application does not call ewk_network_proxy_set(), then the SoupProxyURIResolver is not created and it becomes libsoup's default behavior. > > Is that a problem? As far as I understand, it was the same behavior before my patch. > No problem. I just wanted to double check to be sure your patch is working on Tizen platform. > > > > And one more question for patch's structure. > > SoupProxyURIResolver on your patch is implementing soup session feature's interface, which has no dependency to Efl port. If so, I guess it is better to put Soup port rather than Efl port. What do you think so? > > Agreed. Thank you again!!

Created attachment 153481 [details] Patch Take feedback into consideration, hopefully we are all in agreement now :) The resolver is no longer EFL-specific and I clarified the Changelogs.

(In reply to comment #35) > Created an attachment (id=153481) [details] > Patch > > Take feedback into consideration, hopefully we are all in agreement now :) > > The resolver is no longer EFL-specific and I clarified the Changelogs. LGTM from my side. :)

Comment on attachment 153481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153481&action=review LGTM. Sorry for not pointing out the nit before. > Source/WebCore/platform/network/soup/ProxyResolverSoup.cpp:83 > + uri = static_cast<SoupURI*>(g_value_get_boxed(value)); > + if (priv->proxyURI) > + soup_uri_free(priv->proxyURI); > + > + priv->proxyURI = uri ? soup_uri_copy(uri) : 0; > + break; nit: uri could have been define here since it's used inside this condition.

Created attachment 153486 [details] Patch Fix Thiago's nit.

Created attachment 153488 [details] Patch

LGTM, thanks.

Comment on attachment 153488 [details] Patch Clearing flags on attachment: 153488 Committed r123218: <http://trac.webkit.org/changeset/123218>

All reviewed patches have been landed. Closing bug.