Bug 118343 - [WK2][Soup] Support cache model in NetworkProcess
: [WK2][Soup] Support cache model in NetworkProcess
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Kwang Yul Seo
:
Depends on: 110115 118388
Blocks: 108832
  Show dependency treegraph
 
Reported: 2013-07-03 01:07 PDT by Kwang Yul Seo
Modified: 2013-12-09 04:03 PST (History)
8 users (show)

See Also:


Attachments
Patch (13.38 KB, patch)
2013-07-03 01:38 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
patch (15.01 KB, patch)
2013-09-26 09:04 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
patch (15.01 KB, patch)
2013-09-30 09:41 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (14.83 KB, patch)
2013-10-11 03:46 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
patch on top of attachment213980 (5.61 KB, patch)
2013-10-15 09:26 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (19.12 KB, patch)
2013-10-15 09:43 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (10.82 KB, patch)
2013-12-09 03:07 PST, Kwang Yul Seo
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2013-07-03 01:07:31 PDT
WebKit::NetworkProcess::platformSetCacheModel and WebKit::NetworkProcess::clearCacheForAllOrigins need to be implemented to support cache model in NetworkProcess.
Comment 1 Kwang Yul Seo 2013-07-03 01:38:01 PDT
Created attachment 205980 [details]
Patch
Comment 2 Kwang Yul Seo 2013-07-16 16:17:33 PDT
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]"
Comment 3 Csaba Osztrogonác 2013-08-26 08:10:16 PDT
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. :)
Comment 4 Csaba Osztrogonác 2013-09-26 08:18:43 PDT
(In reply to comment #1)
> Created an attachment (id=205980) [details]
> Patch

The patch is still up-to-date. Could you review it please?
Comment 5 Csaba Osztrogonác 2013-09-26 08:31:41 PDT
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.
Comment 6 Csaba Osztrogonác 2013-09-26 09:04:34 PDT
Created attachment 212715 [details]
patch

updated: one more include added, avoid the conflict with bug118388
Comment 7 Csaba Osztrogonác 2013-09-26 09:05:29 PDT
Now it depends on bug118388 too because of session variable.
Comment 8 Csaba Osztrogonác 2013-09-30 09:41:33 PDT
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.
Comment 9 Csaba Osztrogonác 2013-10-07 10:21:08 PDT
ping?
Comment 10 Brady Eidson 2013-10-07 12:09:42 PDT
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?
Comment 11 Csaba Osztrogonác 2013-10-10 07:31:51 PDT
(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 :)
Comment 12 Csaba Osztrogonác 2013-10-11 03:24:21 PDT
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
Comment 13 Csaba Osztrogonác 2013-10-11 03:46:12 PDT
Created attachment 213980 [details]
Patch

blind fix, will be built locally a little bit later
Comment 14 Csaba Osztrogonác 2013-10-11 03:47:13 PDT
(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.
Comment 15 WebKit Commit Bot 2013-10-11 03:47:58 PDT
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.
Comment 16 Csaba Osztrogonác 2013-10-11 03:58:22 PDT
(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.
Comment 17 Csaba Osztrogonác 2013-10-11 06:51:06 PDT
(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.
Comment 18 Csaba Osztrogonác 2013-10-14 10:28:13 PDT
Carlos, could you review it, please?
Comment 19 Carlos Garcia Campos 2013-10-15 00:23:56 PDT
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.
Comment 20 Csaba Osztrogonác 2013-10-15 09:23:57 PDT
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.
Comment 21 Csaba Osztrogonác 2013-10-15 09:26:53 PDT
Created attachment 214269 [details]
patch on top of attachment213980 [details]

I'll integrate it to https://bugs.webkit.org/attachment.cgi?id=213980 immediately.
Comment 22 Csaba Osztrogonác 2013-10-15 09:43:49 PDT
Created attachment 214274 [details]
Patch
Comment 23 WebKit Commit Bot 2013-10-15 09:45:28 PDT
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.
Comment 24 Csaba Osztrogonác 2013-10-18 06:48:15 PDT
ping?
Comment 25 Csaba Osztrogonác 2013-10-21 02:14:33 PDT
ping for review please
Comment 26 Csaba Osztrogonác 2013-10-21 23:00:55 PDT
ping
Comment 27 Csaba Osztrogonác 2013-10-22 23:21:07 PDT
ping
Comment 28 Carlos Garcia Campos 2013-10-29 06:01:30 PDT
(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.
Comment 29 Carlos Garcia Campos 2013-10-29 06:04:02 PDT
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?
Comment 30 Kwang Yul Seo 2013-12-08 04:11:31 PST
(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.
Comment 31 Kwang Yul Seo 2013-12-09 03:07:23 PST
Created attachment 218743 [details]
Patch
Comment 32 Kwang Yul Seo 2013-12-09 03:08:14 PST
(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!
Comment 33 WebKit Commit Bot 2013-12-09 03:09:45 PST
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.
Comment 34 Carlos Garcia Campos 2013-12-09 03:37:29 PST
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.
Comment 35 Kwang Yul Seo 2013-12-09 04:03:51 PST
Committed r160307: <http://trac.webkit.org/changeset/160307>