Bug 118343 – [WK2][Soup] Support cache model in NetworkProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kwang Yul Seo]

WebKit::NetworkProcess::platformSetCacheModel and WebKit::NetworkProcess::clearCacheForAllOrigins need to be implemented to support cache model in NetworkProcess.

[Kwang Yul Seo]

Created attachment 205980 [details]
Patch

[Kwang Yul Seo]

Comment on attachment 205980 [details]
Patch

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

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:81
> +    g_object_set(session, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE,
> +        SOUP_SESSION_SSL_STRICT, FALSE, 0);

The last argument of g_object_set should be NULL instead of 0. Otherwise, the compiler warns "missing sentinel in function call  [-Wformat]"

[Csaba Osztrogonác]

just a note: this patch might conflict with the patch in https://bugs.webkit.org/show_bug.cgi?id=118520. Only the Source/WebKit2/GNUmakefile.list.am, it depends on which patch lands eaarlier. :)

[Csaba Osztrogonác]

(In reply to comment #1)
> Created an attachment (id=205980) [details]
> Patch

The patch is still up-to-date. Could you review it please?

[Csaba Osztrogonác]

Comment on attachment 205980 [details]
Patch

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

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:79
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();

Just a note: bug118388 is going to add a session named variable to the same block. 
This one or the other one should be modified/removed before landing.

Maybe the best thing would be to let the other bug define and initialize this variable and simple use it here.

[Csaba Osztrogonác]

Created attachment 212715 [details]
patch

updated: one more include added, avoid the conflict with bug118388

[Csaba Osztrogonác]

Now it depends on bug118388 too because of session variable.

[Csaba Osztrogonác]

Created attachment 213006 [details]
patch

(In reply to comment #2)
> (From update of attachment 205980 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205980&action=review
> 
> > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:81
> > +    g_object_set(session, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE,
> > +        SOUP_SESSION_SSL_STRICT, FALSE, 0);
> 
> The last argument of g_object_set should be NULL instead of 0. Otherwise, the compiler warns "missing sentinel in function call  [-Wformat]"

Fixed this warning too.

[Csaba Osztrogonác]

ping?

[Brady Eidson]

Comment on attachment 213006 [details]
patch

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

I'm fine with this if Soup reviewers are.

> Source/WebKit2/ChangeLog:15
> +        The following things were fixed by Csaba Osztrogonác:

Encoding issue in the ChangeLog?

[Csaba Osztrogonác]

(In reply to comment #10)
> (From update of attachment 213006 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213006&action=review
> 
> I'm fine with this if Soup reviewers are.

cc Carlos as SOUP reviewer. Any objection?
 
> > Source/WebKit2/ChangeLog:15
> > +        The following things were fixed by Csaba Osztrogonác:
> 
> Encoding issue in the ChangeLog?
No, the bug is in the pretty diff script, not in my name :)

[Csaba Osztrogonác]

Comment on attachment 213006 [details]
patch

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

> Source/WebKit2/GNUmakefile.list.am:483
> +	Source/WebKit2/Shared/soup/CacheModelHelper.cpp \
> +	Source/WebKit2/Shared/soup/CacheModelHelper.h \

I'm going to add the new files to cmake buildsystem too.

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:38
> +#include <libsoup/soup.h>

ditto

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:80
> +    // Despite using system CAs to validate certificates we're
> +    // accepting invalid certificates by default. New API will be
> +    // added later to let client accept/discard invalid certificates.
> +    g_object_set(session, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE,
> +        SOUP_SESSION_SSL_STRICT, FALSE, NULL);
> +

Now all unix ports use soup, but it won't be true in the future
after Nix will switch to curl. That's why I'm going to add a
USE(SOUP) guard for this block.

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:86
> +    if (SoupSessionFeature* soupCache = soup_session_get_feature(session, SOUP_TYPE_CACHE)) {
> +        soup_cache_flush(SOUP_CACHE(soupCache));
> +        soup_cache_dump(SOUP_CACHE(soupCache));
> +    }

ditto

[Csaba Osztrogonác]

Created attachment 213980 [details]
Patch

blind fix, will be built locally a little bit later

[Csaba Osztrogonác]

(In reply to comment #10)
> > Source/WebKit2/ChangeLog:15
> > +        The following things were fixed by Csaba Osztrogonác:
> 
> Encoding issue in the ChangeLog?
Ooops, you were right, it was really buggy, I fixed it too.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp', u'Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp', u'Source/WebKit2/PlatformEfl.cmake', u'Source/WebKit2/PlatformGTK.cmake', u'Source/WebKit2/Shared/soup/CacheModelHelper.cpp', u'Source/WebKit2/Shared/soup/CacheModelHelper.h', u'Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp']" exit_code: 1
Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:82:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 9 files


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

[Csaba Osztrogonác]

(In reply to comment #15)
> Attachment 213980 [details] did not pass style-queue:
> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:82:  Use 0 instead of NULL.  [readability/null] [5]
> Total errors found: 1 in 9 files

False positive report, we have to use NULL here to 
avoid build failure, see Comment #2 for details.

[Csaba Osztrogonác]

(In reply to comment #13)
> Created an attachment (id=213980) [details]
> Patch
> 
> blind fix, will be built locally a little bit later

I tested, it builds locally with enabled network process.

[Csaba Osztrogonác]

Carlos, could you review it, please?

[Carlos Garcia Campos]

Comment on attachment 213980 [details]
Patch

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

Looks good to me in general, I have a few comments.

> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:32
> +#include "Logging.h"

What is logging used for?

> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:64
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();

You don't need the WebCore:: prefix.

> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:87
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();

Ditto.

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:81
> +    g_object_set(session, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE,

Where does this session came from?

> Source/WebKit2/Shared/soup/CacheModelHelper.cpp:48
> +    return WebCore::getVolumeFreeSizeForPath(cacheDir.get());

You don't need the WebCore:: prefix here either.

> Source/WebKit2/Shared/soup/CacheModelHelper.cpp:68
> +uint64_t getMemorySize()
> +{
> +    static uint64_t kDefaultMemorySize = 512;
> +#if !OS(WINDOWS)
> +    long pageSize = sysconf(_SC_PAGESIZE);
> +    if (pageSize == -1)
> +        return kDefaultMemorySize;
> +
> +    long physPages = sysconf(_SC_PHYS_PAGES);
> +    if (physPages == -1)
> +        return kDefaultMemorySize;
> +
> +    return ((pageSize / 1024) * physPages) / 1024;
> +#else
> +    // Fallback to default for other platforms.
> +    return kDefaultMemorySize;
> +#endif
> +}

Mac also has a function to get the memory size duplicated in WebProcessMac.mm and NetworkProcessMac.mm, I think we could add a common interface to WebCore platform with both implementations there, since this method doesn't depend on any cache model. getCacheDiskFreeSize() is simple enough that we could have it duplicated in WebProcessSoup and NetworkProcessSoup so that we don't need this new file just for a simple function.

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:-80
> -

I think you should also check in the web process if the network process is in use to not use the disk cache or other network stuff.

[Csaba Osztrogonác]

Comment on attachment 213980 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:32
>> +#include "Logging.h"
> 
> What is logging used for?

We don't need it, I'll remove.

>> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:64
>> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();
> 
> You don't need the WebCore:: prefix.

Will remove it.

>> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:81
>> +    g_object_set(session, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE, TRUE,
> 
> Where does this session came from?

Tricky. :) It comes from http://trac.webkit.org/changeset/157406

>> Source/WebKit2/Shared/soup/CacheModelHelper.cpp:48
>> +    return WebCore::getVolumeFreeSizeForPath(cacheDir.get());
> 
> You don't need the WebCore:: prefix here either.

Will remove.

>> Source/WebKit2/Shared/soup/CacheModelHelper.cpp:68
>> +}
> 
> Mac also has a function to get the memory size duplicated in WebProcessMac.mm and NetworkProcessMac.mm, I think we could add a common interface to WebCore platform with both implementations there, since this method doesn't depend on any cache model. getCacheDiskFreeSize() is simple enough that we could have it duplicated in WebProcessSoup and NetworkProcessSoup so that we don't need this new file just for a simple function.

I checked the Mac implementation, they use exactly same memorySize function, and similar
volumeFreeSize() functions. The only difference is the parameter type (NSString* vs const String&)
Additionally they have a same volumeFreeSize implementation (as the WebProcess one) in
Source/WebKit/mac/Misc/WebKitSystemBits.m and an ancient version of memorySize.

I don't think if we should do this unification here. It can be done as a followup patch,
it must be a low hanging fruit for somebody with Mac build environment.

>> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:-80
>> -
> 
> I think you should also check in the web process if the network process is in use to not use the disk cache or other network stuff.

Good point, I'll fix it.

[Csaba Osztrogonác]

Created attachment 214269 [details]
patch on top of attachment213980 [details]

I'll integrate it to https://bugs.webkit.org/attachment.cgi?id=213980 immediately.

[Csaba Osztrogonác]

Created attachment 214274 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp', u'Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp', u'Source/WebKit2/PlatformEfl.cmake', u'Source/WebKit2/PlatformGTK.cmake', u'Source/WebKit2/Shared/soup/CacheModelHelper.cpp', u'Source/WebKit2/Shared/soup/CacheModelHelper.h', u'Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp']" exit_code: 1
Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:95:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 9 files


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

[Csaba Osztrogonác]

ping?

[Csaba Osztrogonác]

ping for review please

[Csaba Osztrogonác]

ping

[Csaba Osztrogonác]

ping

[Carlos Garcia Campos]

(In reply to comment #20)
> >> Source/WebKit2/Shared/soup/CacheModelHelper.cpp:68
> >> +}
> > 
> > Mac also has a function to get the memory size duplicated in WebProcessMac.mm and NetworkProcessMac.mm, I think we could add a common interface to WebCore platform with both implementations there, since this method doesn't depend on any cache model. getCacheDiskFreeSize() is simple enough that we could have it duplicated in WebProcessSoup and NetworkProcessSoup so that we don't need this new file just for a simple function.
> 
> I checked the Mac implementation, they use exactly same memorySize function, and similar
> volumeFreeSize() functions. The only difference is the parameter type (NSString* vs const String&)
> Additionally they have a same volumeFreeSize implementation (as the WebProcess one) in
> Source/WebKit/mac/Misc/WebKitSystemBits.m and an ancient version of memorySize.
> 
> I don't think if we should do this unification here. It can be done as a followup patch,
> it must be a low hanging fruit for somebody with Mac build environment.
>

If we are going to do it in a follow up patch, I prefer to duplicate the code for now, instead of adding new files that we are going to remove later. It will make this patch simpler too.

[Carlos Garcia Campos]

Comment on attachment 214274 [details]
Patch

Would it be possible to have m_usesNetworkProcess unconditionally and initialize it to false when !ENABLE(NETWORK_PROCESS) so that we don't need an #if every time the variable is used?

[Kwang Yul Seo]

(In reply to comment #29)
> (From update of attachment 214274 [details])
> Would it be possible to have m_usesNetworkProcess unconditionally and initialize it to false when !ENABLE(NETWORK_PROCESS) so that we don't need an #if every time the variable is used?

It sounds good, but it should be a separate refactoring patch because the current convention of guarding m_usesNetworkProcess with ENABLE(NETWORK_PROCESS) is all over WebKit2.

[Kwang Yul Seo]

Created attachment 218743 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #28)
> If we are going to do it in a follow up patch, I prefer to duplicate the code for now, instead of adding new files that we are going to remove later. It will make this patch simpler too.

Done!

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp', u'Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp', u'Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:95:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 4 files


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

[Carlos Garcia Campos]

Comment on attachment 218743 [details]
Patch

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

> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:36
> +

Please, remove this empty line

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:40
> +#if USE(SOUP)
> +#include <libsoup/soup.h>
> +#endif

I think we don't need to keep the alphabetic order for headers added inside an #if block, so I think it's better to move this below right after the #if PLATFORM(EFL) block

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:104
> +#if ENABLE(NETWORK_PROCESS)
> +    if (!m_usesNetworkProcess) {
> +#endif
> +        SoupSession* session = WebCore::ResourceHandle::defaultSession();
> +        cache = SOUP_CACHE(soup_session_get_feature(session, SOUP_TYPE_CACHE));
> +        diskFreeSize = getCacheDiskFreeSize(cache) / 1024 / 1024;
> +#if ENABLE(NETWORK_PROCESS)
> +    }
> +#endif

I think we could define those unconditionally and return false when network process is not enabled, but if we can probably do it in a follow up patch to not block this one.

[Kwang Yul Seo]

Committed r160307: <http://trac.webkit.org/changeset/160307>