Bug 84560 – REGRESSION(113604): [Soup] Some pages that use synchronous XMLHttpRequests freeze the browser

• Component: Platform
• Status: RESOLVED FIXED

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 138290 [details]
Patch

[WebKit Review Bot]

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.

[Sergio Villar Senin]

(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.

[Xan Lopez]

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.

[Martin Robinson]

(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!

[Xan Lopez]

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).

[Dan Winship]

(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.

[Martin Robinson]

(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/

[Martin Robinson]

Committed r114980: <http://trac.webkit.org/changeset/114980>