[WK2][Soup] Add platform specific stubs for NetworkProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Balazs Kelemen]

Mostly stubs.

[Balazs Kelemen]

Created attachment 188906 [details]
Patch

[Kwang Yul Seo]

Created attachment 205783 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #2)
I made three changes to the original patch:
1. Build fix for NetworkResourceLoadScheduler::platformInitializeMaximumHTTPConnectionCountPerHost as the return type is changed to void.

2. Add NetworkProcessGtk.cpp

3. Add stubs for two new platform methods: NetworkProcess::clearCacheForAllOrigins and NetworkProcess::platformTerminate.

[Kwang Yul Seo]

Created attachment 205966 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #4)
Merged NetworkProcessGtk.cpp and NetworkProcessEfl.cpp into NetworkProcessSoup.cpp.

[Sergio Villar Senin]

Comment on attachment 205966 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        No new tests since it is not used yet and does not supposed to change behavior anyway.

Nit: is not supposed

> Source/WebCore/platform/network/NetworkStorageSession.h:61
> +    bool isPrivateBrowsingSession() const { return m_isPrivate; }

Why is this here? Looks like it does not have much to do with the patch, and it's something that should look the same in all platforms.

> Source/WebCore/platform/network/NetworkStorageSession.h:77
> +    bool m_isPrivate;

Ditto.

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:38
> +    , m_isPrivate(false)

Ditto.

> Source/WebKit2/NetworkProcess/soup/NetworkResourceLoadSchedulerSoup.cpp:36
> +    m_maxRequestsInFlightPerHost = unlimitedConnectionCount;

Why such a huge value? We're currently using 6. Increasing this number won't improve performance as many hosts will refuse to establish new connections.

[Kwang Yul Seo]

(In reply to comment #6)
> Why such a huge value? We're currently using 6. Increasing this number won't improve performance as many hosts will refuse to establish new connections.

There is a comment in ResourceRequestSoup.cpp:

unsigned initializeMaximumHTTPConnectionCountPerHost()
{
    // Soup has its own queue control; it wants to have all requests
    // given to it, so that it is able to look ahead, and schedule
    // them in a good way.
    return 10000;
}

I will add the same comment here.

BTW, where did you get the number 6?

[Kwang Yul Seo]

(In reply to comment #6)
> > Source/WebCore/platform/network/NetworkStorageSession.h:61
> > +    bool isPrivateBrowsingSession() const { return m_isPrivate; }
> 
> Why is this here? Looks like it does not have much to do with the patch, and it's something that should look the same in all platforms.

isPrivateBrowsingSession() is required when NetworkProcess is enabled in WebPlatformStrategies.cpp.

String WebPlatformStrategies::cookiesForDOM(const NetworkStorageSession& session, const KURL& firstParty, const KURL& url)
{
#if ENABLE(NETWORK_PROCESS)
    if (WebProcess::shared().usesNetworkProcess()) {
        String result;
        if (!WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::CookiesForDOM(session.isPrivateBrowsingSession(), firstParty, url), Messages::NetworkConnectionToWebProcess::CookiesForDOM::Reply(result), 0))
            return String();
        return result;
    }   
#endif


Currently, only Mac port provides this method. To build WebKit Gtk or Efl port with NetworkProcess enabled, we need to add a dummy implementation of isPrivateBrowsingSession().

Instead of duplicating isPrivateBrowsingSession() method and m_isPrivate member variable, I will move the method out of PLATFORM(MAC) guard and make the body return false for non-Mac ports. That will make it clear that it is a dummy method.

Actually supporting private browsing in NetworkProcess will be done later.

[Sergio Villar Senin]

(In reply to comment #7)
> (In reply to comment #6)
> > Why such a huge value? We're currently using 6. Increasing this number won't improve performance as many hosts will refuse to establish new connections.
> 
> There is a comment in ResourceRequestSoup.cpp:
> 
> unsigned initializeMaximumHTTPConnectionCountPerHost()
> {
>     // Soup has its own queue control; it wants to have all requests
>     // given to it, so that it is able to look ahead, and schedule
>     // them in a good way.
>     return 10000;
> }
> 
> I will add the same comment here.
> 
> BTW, where did you get the number 6?

Yeah you're right, I forgot that we set that kind of "unlimited" value for WebKit because we control that at libsoup level. Check ResourceHandleSoup which is the place where we create the SoupSession

[Sergio Villar Senin]

(In reply to comment #8)
> (In reply to comment #6)
> > > Source/WebCore/platform/network/NetworkStorageSession.h:61
> > > +    bool isPrivateBrowsingSession() const { return m_isPrivate; }
> > 
> > Why is this here? Looks like it does not have much to do with the patch, and it's something that should look the same in all platforms.
> 
> isPrivateBrowsingSession() is required when NetworkProcess is enabled in WebPlatformStrategies.cpp.
> 
> String WebPlatformStrategies::cookiesForDOM(const NetworkStorageSession& session, const KURL& firstParty, const KURL& url)
> {
> #if ENABLE(NETWORK_PROCESS)
>     if (WebProcess::shared().usesNetworkProcess()) {
>         String result;
>         if (!WebProcess::shared().networkConnection()->connection()->sendSync(Messages::NetworkConnectionToWebProcess::CookiesForDOM(session.isPrivateBrowsingSession(), firstParty, url), Messages::NetworkConnectionToWebProcess::CookiesForDOM::Reply(result), 0))
>             return String();
>         return result;
>     }   
> #endif

Yeah I know, but as you can see that's common to all platforms so it makes no sense to add it only for soup.

> Currently, only Mac port provides this method. To build WebKit Gtk or Efl port with NetworkProcess enabled, we need to add a dummy implementation of isPrivateBrowsingSession().
> 
> Instead of duplicating isPrivateBrowsingSession() method and m_isPrivate member variable, I will move the method out of PLATFORM(MAC) guard and make the body return false for non-Mac ports. That will make it clear that it is a dummy method.

That's exactly what I was suggesting.

[Kwang Yul Seo]

Created attachment 206004 [details]
Patch

[Kwang Yul Seo]

Created attachment 206005 [details]
Patch

[Brady Eidson]

Comment on attachment 206005 [details]
Patch

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

r- because of the WebCore change.

> Source/WebCore/platform/network/NetworkStorageSession.h:-57
> +    bool isPrivateBrowsingSession() const
> +    {
> +#if PLATFORM(MAC) || USE(CFNETWORK)
> +        return m_isPrivate;
> +#else
> +        return false;
> +#endif
> +    }
> +
>  #if PLATFORM(MAC) || USE(CFNETWORK)
>      // May be null, in which case a Foundation default should be used.
>      CFURLStorageSessionRef platformSession() { return m_platformSession.get(); }
>      RetainPtr<CFHTTPCookieStorageRef> cookieStorage() const;
> -    bool isPrivateBrowsingSession() const { return m_isPrivate; }

No.  This is silly.

Private browsing is a cross-platform concept that's been exposed to all of WebCore for years.  It is going to be getting *more* important going forward, not less.  It would be wise for every port to start handling it properly.

And we can't change WebKit1-style clients (present or future) based on one platform's WK2-specific needs.

If this behavior is truly required (for now?), make the storage session always have m_isPrivate == false in platform-specific code.

> Source/WebKit2/NetworkProcess/soup/NetworkResourceLoadSchedulerSoup.cpp:41
> +    // Soup has its own queue control; it wants to have all requests
> +    // given to it, so that it is able to look ahead, and schedule
> +    // them in a good way.
> +    // See also ResourceRequestSoup.cpp
> +    static const unsigned unlimitedConnectionCount = 10000;
> +    m_maxRequestsInFlightPerHost = unlimitedConnectionCount;

I would recommend against this design decision.

Even if the networking library handles per-host scheduling correctly (and I'm sure libsoup does), there's plenty of Web-platform specific knowledge that can help prioritization better than a networking library alone can do.

There will be work in this area in the medium term that you won't get the benefit of if you go forward with the unlimited queue approach.

Additionally, we've seen IPC race conditions related to synchronous XMLHTTPRequests where we've learned you can get into serious trouble endlessly servicing asynchronous requests while a WebProcess is waiting on a sync XHR.

[Balazs Kelemen]

> > Source/WebKit2/NetworkProcess/soup/NetworkResourceLoadSchedulerSoup.cpp:41
> > +    // Soup has its own queue control; it wants to have all requests
> > +    // given to it, so that it is able to look ahead, and schedule
> > +    // them in a good way.
> > +    // See also ResourceRequestSoup.cpp
> > +    static const unsigned unlimitedConnectionCount = 10000;
> > +    m_maxRequestsInFlightPerHost = unlimitedConnectionCount;
> 
> I would recommend against this design decision.
> 
> Even if the networking library handles per-host scheduling correctly (and I'm sure libsoup does), there's plenty of Web-platform specific knowledge that can help prioritization better than a networking library alone can do.
> 
> There will be work in this area in the medium term that you won't get the benefit of if you go forward with the unlimited queue approach.
> 
> Additionally, we've seen IPC race conditions related to synchronous XMLHTTPRequests where we've learned you can get into serious trouble endlessly servicing asynchronous requests while a WebProcess is waiting on a sync XHR.

These are very valid points. I expect it is a complex problem to find the balance across libsoup and WebKit. Probably the best would be to manage this entirely in WebKit but I guess libsoup does not have an option to turn off it's scheduling heuristics. I think the unlimited queue approach can be ok for now (we also have this for WebKit1), just add a FIXME that it should be revisited.

[Kwang Yul Seo]

(In reply to comment #13)
> No.  This is silly.
> 
> Private browsing is a cross-platform concept that's been exposed to all of WebCore for years.  It is going to be getting *more* important going forward, not less.  It would be wise for every port to start handling it properly.
> 
> And we can't change WebKit1-style clients (present or future) based on one platform's WK2-specific needs.
> 
> If this behavior is truly required (for now?), make the storage session always have m_isPrivate == false in platform-specific code.

I am wiling to support private browsing for libsoup. But because this patch simply adds platform specific stubs to support NetworkProcess, I will make the storage session always have m_isPrivate == false in platform-specific code for now.
 
> I would recommend against this design decision.
> 
> Even if the networking library handles per-host scheduling correctly (and I'm sure libsoup does), there's plenty of Web-platform specific knowledge that can help prioritization better than a networking library alone can do.
> 
> There will be work in this area in the medium term that you won't get the benefit of if you go forward with the unlimited queue approach.
> 
> Additionally, we've seen IPC race conditions related to synchronous XMLHTTPRequests where we've learned you can get into serious trouble endlessly servicing asynchronous requests while a WebProcess is waiting on a sync XHR.

I agree. I will leave a FIXME comment as Balazs suggested and discuss this issue with libsoup maintainers.

[Kwang Yul Seo]

Created attachment 206200 [details]
Patch

[EFL EWS Bot]

Comment on attachment 206200 [details]
Patch

Attachment 206200 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1048113

[EFL EWS Bot]

Comment on attachment 206200 [details]
Patch

Attachment 206200 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1010063

[Early Warning System Bot]

Comment on attachment 206200 [details]
Patch

Attachment 206200 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1002196

[Early Warning System Bot]

Comment on attachment 206200 [details]
Patch

Attachment 206200 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/994615

[Build Bot]

Comment on attachment 206200 [details]
Patch

Attachment 206200 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/994617

[Kwang Yul Seo]

Created attachment 206202 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #22)
Build fix by reordering m_isPrivate.

[Sergio Villar Senin]

Comment on attachment 206202 [details]
Patch

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

Looks much better now.

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:40
> +    // FIXME: Support private browsing.

Not sure we need this comment here, we already have the ASSERT_NOT_REACHED() y createPrivateBrowsingSession() which servers as a good reminder.

[Kwang Yul Seo]

(In reply to comment #24)
> Not sure we need this comment here, we already have the ASSERT_NOT_REACHED() y createPrivateBrowsingSession() which servers as a good reminder.

Yeah, but ASSERT_NOT_REACHED() does not necessarily mean that we have a plan to implement this. So I think adding a FIXME comment here is okay.

[Build Bot]

Comment on attachment 206202 [details]
Patch

Attachment 206202 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1085352

[Kwang Yul Seo]

Created attachment 207494 [details]
Patch

[Kwang Yul Seo]

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

Updated the patch as soup now supports private browsing. (Bug 118657)

[Csaba Osztrogonác]

(In reply to comment #28)
> (In reply to comment #27)
> > Created an attachment (id=207494) [details] [details]
> > Patch
> 
> Updated the patch as soup now supports private browsing. (Bug 118657)

The patch is still up-to-date, could you review it please?

[Carlos Garcia Campos]

Comment on attachment 207494 [details]
Patch

I guess you should also add the new files to the makefile.

[Csaba Osztrogonác]

(In reply to comment #30)
> (From update of attachment 207494 [details])
> I guess you should also add the new files to the makefile.

Sure. The original plan was to add all needed file in one big patch 
in bug110139 and bug118231. But it's safe to add the new files in this
patch, because they are NETWORK_PROCESS guarded. I'll add them.

[Csaba Osztrogonác]

Created attachment 213880 [details]
=patch for landing

new files added to buildsystems, let's see the EWS check

[WebKit Commit Bot]

Comment on attachment 213880 [details]
=patch for landing

Clearing flags on attachment: 213880

Committed r157254: <http://trac.webkit.org/changeset/157254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.