Bug 117842 - WebProcess hangs loading eff.org (Waiting forever on a sync XHR, NetworkProcess unable to service it)
Summary: WebProcess hangs loading eff.org (Waiting forever on a sync XHR, NetworkProce...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-06-20 11:48 PDT by Brady Eidson
Modified: 2013-06-20 13:13 PDT (History)
2 users (show)

See Also:


Attachments
Patch v1 (13.15 KB, patch)
2013-06-20 12:09 PDT, Brady Eidson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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