Bug 117842

Summary: WebProcess hangs loading eff.org (Waiting forever on a sync XHR, NetworkProcess unable to service it)
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, sam
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
Patch v1 ap: review+

Description Brady Eidson 2013-06-20 11:48:58 PDT
WebProcess hangs loading eff.org (Waiting forever on a sync XHR, NetworkProcess unable to service it)

This is the known theoretical "6 + 1 problem" where 6 asynchronous requests to a host are already outstanding while a 7th sync XHR is made.

In single WebProcess mode and WebKit 1, the sync XHR will time out and the WebProcess will recover.

With the NetworkProcess the WebProcess waits forever on the NetworkProcess, and the NetworkProcess doesn't even try to serve the 7th request.

A lot more could be said about this.  But the TLDR; is that we need to bump the platform connection-per-host limit to 7, and only use that 7th slot for a sync XHR if 6 loads are already in progress.

In radar as <rdar://problem/14112329>
Comment 1 Brady Eidson 2013-06-20 12:09:11 PDT
Created attachment 205108 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2013-06-20 12:55:03 PDT
Comment on attachment 205108 [details]
Patch v1 

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

> Source/WebKit2/NetworkProcess/HostRecord.cpp:198
> +    ASSERT(loader && loader->connectionToWebProcess());

Two lines.

> Source/WebKit2/NetworkProcess/HostRecord.cpp:204
> +    if (loader->connectionToWebProcess()->isSerialLoadingEnabled() && m_loadersInProgress.size() >= 1)
> +        return true;

What is serial loading? Sounds like we have the exactly the same issue with it if there is one outstanding request, and WebProcess starts a sync one.

> Source/WebKit2/NetworkProcess/HostRecord.cpp:213
> +    // If we're already past the limit of the number of loaders in flight, we won't even serve new synchronous requests right now.
> +    if (m_loadersInProgress.size() > m_maxRequestsInFlight)
> +        return true;

Perhaps assert that at least one active loader is synchronous?

> Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h:29
> +#include "HostRecord.h"

Why?
Comment 3 Brady Eidson 2013-06-20 13:02:40 PDT
(In reply to comment #2)
> (From update of attachment 205108 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205108&action=review
> 
> > Source/WebKit2/NetworkProcess/HostRecord.cpp:198
> > +    ASSERT(loader && loader->connectionToWebProcess());
> 
> Two lines.

Yup!

> > Source/WebKit2/NetworkProcess/HostRecord.cpp:204
> > +    if (loader->connectionToWebProcess()->isSerialLoadingEnabled() && m_loadersInProgress.size() >= 1)
> > +        return true;
> 
> What is serial loading? Sounds like we have the exactly the same issue with it if there is one outstanding request, and WebProcess starts a sync one.

It's SPI for DRT/WKTR for a tiny subset of tests.  None with sync XHR.  Shouldn't be a factor in real browsing.

> > Source/WebKit2/NetworkProcess/HostRecord.cpp:213
> > +    // If we're already past the limit of the number of loaders in flight, we won't even serve new synchronous requests right now.
> > +    if (m_loadersInProgress.size() > m_maxRequestsInFlight)
> > +        return true;
> 
> Perhaps assert that at least one active loader is synchronous?

It'll be a lot of debug-only code, but I think it's worthwhile.

> > Source/WebKit2/NetworkProcess/NetworkResourceLoadScheduler.h:29
> > +#include "HostRecord.h"
> 
> Why?

Leftover from earlier patch, and I'd fixed locally but not committed - Thanks for spotting it.

Thanks!
Comment 4 Brady Eidson 2013-06-20 13:13:09 PDT
http://trac.webkit.org/changeset/151795