Bug 84560 - REGRESSION(113604): [Soup] Some pages that use synchronous XMLHttpRequests freeze the browser
Summary: REGRESSION(113604): [Soup] Some pages that use synchronous XMLHttpRequests fr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-22 21:39 PDT by Martin Robinson
Modified: 2012-04-23 22:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.88 KB, patch)
2012-04-22 21:44 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2012-04-22 21:39:43 PDT
As noted at https://bugs.webkit.org/show_bug.cgi?id=68238, pages that launch a synchronous XMLHttpRequest when at the connection limit get trapped in the interior main loop. The sycnrhonous request will wait for an open connection indefintely, but no existing connections will complete, since they are running in the exterior main loop.
Comment 1 Martin Robinson 2012-04-22 21:44:48 PDT
Created attachment 138290 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-22 21:47:49 PDT
Attachment 138290 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:109:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:115:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Sergio Villar Senin 2012-04-23 00:39:16 PDT
(In reply to comment #1)
> Created an attachment (id=138290) [details]
> Patch

It does not look like the "right thing to do" indeed but at least it fixes the issue.
Comment 4 Xan Lopez 2012-04-23 13:16:23 PDT
Comment on attachment 138290 [details]
Patch

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

Would adding the request in a queue until you are  below the connection threshold against the semantics of sync requests or something? Seems like the "normal" thing to do.

If that's too complicated I guess this is OK as long as we are sure that there's no way to run more than one sync request under any circumstance.

> Source/WebCore/ChangeLog:17
> +        (WebCore::WebCoreSynchronousLoader::adjustMaxConnections): Added this helpe.rj

You have a type in the 'helpe.rj' thing.
Comment 5 Martin Robinson 2012-04-23 13:22:00 PDT
(In reply to comment #4)
> (From update of attachment 138290 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138290&action=review
> 
> Would adding the request in a queue until you are  below the connection threshold against the semantics of sync requests or something? Seems like the "normal" thing to do.

Unfortunately not. We have to run the request when WebKit sees it to ensure web compatibility.

> If that's too complicated I guess this is OK as long as we are sure that there's no way to run more than one sync request under any circumstance.

This should never happen. I can add an assertion to verify this behavior.
 
> > Source/WebCore/ChangeLog:17
> > +        (WebCore::WebCoreSynchronousLoader::adjustMaxConnections): Added this helpe.rj
> 
> You have a type in the 'helpe.rj' thing.

Will fix!
Comment 6 Xan Lopez 2012-04-23 13:28:04 PDT
Comment on attachment 138290 [details]
Patch

An assert seems good. Also something that fails in non-debug builds, at least that makes sure we cannot go nuts (g_return_if_fail or something along those lines).
Comment 7 Dan Winship 2012-04-23 13:56:37 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > If that's too complicated I guess this is OK as long as we are sure that there's no way to run more than one sync request under any circumstance.
> 
> This should never happen.

what if you send a synchronous xmlhttprequest from an onprogress handler of another synchronous xmlhttprequest?

but anyway, the code ought to still work fine in that case; the inner request will just bump max-conns up 1 more run, then drop it back down to where it found it, and then the outer request will finish and drop it back down to where it found it.
Comment 8 Martin Robinson 2012-04-23 14:10:04 PDT
(In reply to comment #7)

> what if you send a synchronous xmlhttprequest from an onprogress handler of another synchronous xmlhttprequest?

The spec seems to suggest that progress events are no fired when loading synchronous XMLHttpRequests: http://www.w3.org/TR/XMLHttpRequest/
Comment 9 Martin Robinson 2012-04-23 18:33:12 PDT
Committed r114980: <http://trac.webkit.org/changeset/114980>