84560 2012-04-22 21:39:43 -0700 REGRESSION(113604): [Soup] Some pages that use synchronous XMLHttpRequests freeze the browser 2012-04-23 22:21:50 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mrobinson mrobinson danw gustavo rakuco svillar webkit.review.bot oldest_to_newest 607440 0 mrobinson 2012-04-22 21:39:43 -0700 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. 607442 1 138290 mrobinson 2012-04-22 21:44:48 -0700 Created attachment 138290 Patch 607444 2 webkit.review.bot 2012-04-22 21:47:49 -0700 Attachment 138290 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. 607521 3 svillar 2012-04-23 00:39:16 -0700 (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. 607960 4 138290 xan.lopez 2012-04-23 13:16:23 -0700 Comment on attachment 138290 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. 607967 5 mrobinson 2012-04-23 13:22:00 -0700 (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! 607973 6 138290 xan.lopez 2012-04-23 13:28:04 -0700 Comment on attachment 138290 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). 607994 7 danw 2012-04-23 13:56:37 -0700 (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. 608008 8 mrobinson 2012-04-23 14:10:04 -0700 (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/ 608283 9 mrobinson 2012-04-23 18:33:12 -0700 Committed r114980: <http://trac.webkit.org/changeset/114980> 138290 2012-04-22 21:44:48 -0700 2012-04-23 22:21:50 -0700 Patch bug-84560-20120422214447.patch text/plain 4998 mrobinson U3VidmVyc2lvbiBSZXZpc2lvbjogMTE0ODY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNGIwNmZkZDIxMWJjZThl ZDk5MmI2ODVhZDUxZGY2M2Y2YzY0MTZkMS4uMjkzODFjOTA4NGY0MDQwNmFhODU0N2JhMWE4YzNi YjZmMzFkMjJkYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDI4IEBACiAyMDEyLTA0LTIyICBNYXJ0 aW4gUm9iaW5zb24gIDxtcm9iaW5zb25AaWdhbGlhLmNvbT4KIAorICAgICAgICBSRUdSRVNTSU9O KDExMzYwNCk6IFtTb3VwXSBTb21lIHBhZ2VzIHRoYXQgdXNlIHN5bmNocm9ub3VzIFhNTEh0dHBS ZXF1ZXN0cyBmcmVlemUgdGhlIGJyb3dzZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTg0NTYwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgV2hlbiBraWNraW5nIG9mZiBhIHN5bmNocm9ub3VzIFhNTEh0dHBS ZXF1ZXN0LCBhZGQgb25lIHRvIHRoZSBjb25uZWN0aW9uCisgICAgICAgIGxpbWl0LiBUaGlzIGVu c3VyZXMgdGhhdCB3aGVuIGEgcGFnZSBzdGFydHMgYSBzeW5jaHJvbm91cyByZXF1ZXN0LCB3aGls ZQorICAgICAgICBhbHJlYWR5IGF0IHRoZSBjb25uZWN0aW9uIGxpbWl0IHRoZSByZXF1ZXN0IHdp bGwgbm90IGRlYWRsb2NrLgorCisgICAgICAgICogcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291 cmNlSGFuZGxlU291cC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpXZWJDb3JlU3luY2hyb25vdXNM b2FkZXI6OldlYkNvcmVTeW5jaHJvbm91c0xvYWRlcik6IEFjY2VwdCBhIG5ldyBTb3VwU2Vzc2lv bgorICAgICAgICBhcmd1bWVudCBzbyB0aGF0IHdlIGNhbiBnZXQgdGhlIGNvcnJlY3QgU291cFNl c3Npb24gZm9yIHRoZSBuZXR3b3JraW5nIGNvbnRleHQuIEJ1bXAKKyAgICAgICAgdGhlIGNvbm5l Y3Rpb24gbGltaXQuCisgICAgICAgIChXZWJDb3JlOjpXZWJDb3JlU3luY2hyb25vdXNMb2FkZXI6 On5XZWJDb3JlU3luY2hyb25vdXNMb2FkZXIpOiBEZWNyZW1lbnQgdGhlIGNvbm5lY3Rpb24gbGlt aXQuCisgICAgICAgIChXZWJDb3JlOjpXZWJDb3JlU3luY2hyb25vdXNMb2FkZXI6OmFkanVzdE1h eENvbm5lY3Rpb25zKTogQWRkZWQgdGhpcyBoZWxwZS5yagorICAgICAgICAoV2ViQ29yZVN5bmNo cm9ub3VzTG9hZGVyKTogQWRkZWQgYSBuZXcgU291cFNlc3Npb24gbWVtYmVyLgorICAgICAgICAo V2ViQ29yZTo6c2Vzc2lvbkZyb21Db250ZXh0KTogQWRkZWQgdGhpcyBoZWxwZXIuCisgICAgICAg IChXZWJDb3JlOjpSZXNvdXJjZUhhbmRsZUludGVybmFsOjpzb3VwU2Vzc2lvbik6IFVzZSB0aGUg bmV3IHNlc3Npb25Gcm9tQ29udGV4dCBoZWxwZXIuCisgICAgICAgIChXZWJDb3JlOjpSZXNvdXJj ZUhhbmRsZTo6bG9hZFJlc291cmNlU3luY2hyb25vdXNseSk6IFBhc3MgdGhlIFNvdXBTZXNzaW9u IGZyb20gdGhlIE5ldHdvcmtpbmdDb250ZXh0CisgICAgICAgIHRvIHRoZSBzeW5jaHJvbm91cyBs b2FkZXIuCisKKzIwMTItMDQtMjIgIE1hcnRpbiBSb2JpbnNvbiAgPG1yb2JpbnNvbkBpZ2FsaWEu Y29tPgorCiAgICAgICAgIFtDYWlyb10gSW1wbGVtZW50IENvbXBvc2l0ZURpZmZlcmVuY2UKICAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTc3MzU0CiAKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9SZXNvdXJjZUhh bmRsZVNvdXAuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291 cmNlSGFuZGxlU291cC5jcHAKaW5kZXggY2Q0MTk5NzhhYzQwYjhlN2U5NTM3NzcyMDk1ZjQ4YTU5 ZmE2OWNhMC4uYzc4ZDgyZTYwMzc1ZTY4YTRjNTZlODk1NDNhNDBhNGY2MGM1MjU1ZSAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlSGFuZGxl U291cC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291 cmNlSGFuZGxlU291cC5jcHAKQEAgLTc1LDkgKzc1LDEwIEBAIGNsYXNzIFdlYkNvcmVTeW5jaHJv bm91c0xvYWRlciA6IHB1YmxpYyBSZXNvdXJjZUhhbmRsZUNsaWVudCB7CiAgICAgV1RGX01BS0Vf Tk9OQ09QWUFCTEUoV2ViQ29yZVN5bmNocm9ub3VzTG9hZGVyKTsKIHB1YmxpYzoKIAotICAgIFdl YkNvcmVTeW5jaHJvbm91c0xvYWRlcihSZXNvdXJjZUVycm9yJiBlcnJvciwgUmVzb3VyY2VSZXNw b25zZSYgcmVzcG9uc2UsIFZlY3RvcjxjaGFyPiYgZGF0YSkKKyAgICBXZWJDb3JlU3luY2hyb25v dXNMb2FkZXIoUmVzb3VyY2VFcnJvciYgZXJyb3IsIFJlc291cmNlUmVzcG9uc2UmIHJlc3BvbnNl LCBTb3VwU2Vzc2lvbiogc2Vzc2lvbiwgVmVjdG9yPGNoYXI+JiBkYXRhKQogICAgICAgICA6IG1f ZXJyb3IoZXJyb3IpCiAgICAgICAgICwgbV9yZXNwb25zZShyZXNwb25zZSkKKyAgICAgICAgLCBt X3Nlc3Npb24oc2Vzc2lvbikKICAgICAgICAgLCBtX2RhdGEoZGF0YSkKICAgICAgICAgLCBtX2Zp bmlzaGVkKGZhbHNlKQogICAgIHsKQEAgLTg4LDE0ICs4OSwzMyBAQCBwdWJsaWM6CiAgICAgICAg IEdSZWZQdHI8R01haW5Db250ZXh0PiBpbm5lck1haW5Db250ZXh0ID0gYWRvcHRHUmVmKGdfbWFp bl9jb250ZXh0X25ldygpKTsKICAgICAgICAgZ19tYWluX2NvbnRleHRfcHVzaF90aHJlYWRfZGVm YXVsdChpbm5lck1haW5Db250ZXh0LmdldCgpKTsKICAgICAgICAgbV9tYWluTG9vcCA9IGdfbWFp bl9sb29wX25ldyhpbm5lck1haW5Db250ZXh0LmdldCgpLCBmYWxzZSk7CisKKyAgICAgICAgYWRq dXN0TWF4Q29ubmVjdGlvbnMoMSk7CiAgICAgfQogCiAgICAgfldlYkNvcmVTeW5jaHJvbm91c0xv YWRlcigpCiAgICAgeworICAgICAgICBhZGp1c3RNYXhDb25uZWN0aW9ucygtMSk7CiAgICAgICAg IGdfbWFpbl9jb250ZXh0X3BvcF90aHJlYWRfZGVmYXVsdChnX21haW5fY29udGV4dF9nZXRfdGhy ZWFkX2RlZmF1bHQoKSk7CiAgICAgICAgIGxvYWRpbmdTeW5jaHJvbm91c1JlcXVlc3QgPSBmYWxz ZTsKICAgICB9CiAKKyAgICB2b2lkIGFkanVzdE1heENvbm5lY3Rpb25zKGludCBhZGp1c3RtZW50 KQorICAgIHsKKyAgICAgICAgaW50IG1heENvbm5lY3Rpb25zLCBtYXhDb25uZWN0aW9uc1Blckhv c3Q7CisgICAgICAgIGdfb2JqZWN0X2dldChtX3Nlc3Npb24sCisgICAgICAgICAgICAgICAgICAg ICBTT1VQX1NFU1NJT05fTUFYX0NPTk5TLCAmbWF4Q29ubmVjdGlvbnMsCisgICAgICAgICAgICAg ICAgICAgICBTT1VQX1NFU1NJT05fTUFYX0NPTk5TX1BFUl9IT1NULCAmbWF4Q29ubmVjdGlvbnNQ ZXJIb3N0LAorICAgICAgICAgICAgICAgICAgICAgTlVMTCk7CisgICAgICAgIG1heENvbm5lY3Rp b25zICs9IGFkanVzdG1lbnQ7CisgICAgICAgIG1heENvbm5lY3Rpb25zUGVySG9zdCArPSBhZGp1 c3RtZW50OworICAgICAgICBnX29iamVjdF9zZXQobV9zZXNzaW9uLAorICAgICAgICAgICAgICAg ICAgICAgU09VUF9TRVNTSU9OX01BWF9DT05OUywgbWF4Q29ubmVjdGlvbnMsCisgICAgICAgICAg ICAgICAgICAgICBTT1VQX1NFU1NJT05fTUFYX0NPTk5TX1BFUl9IT1NULCBtYXhDb25uZWN0aW9u c1Blckhvc3QsCisgICAgICAgICAgICAgICAgICAgICBOVUxMKTsKKworICAgIH0KKwogICAgIHZp cnR1YWwgYm9vbCBpc1N5bmNocm9ub3VzQ2xpZW50KCkKICAgICB7CiAgICAgICAgIHJldHVybiB0 cnVlOwpAQCAtMTMzLDYgKzE1Myw3IEBAIHB1YmxpYzoKIHByaXZhdGU6CiAgICAgUmVzb3VyY2VF cnJvciYgbV9lcnJvcjsKICAgICBSZXNvdXJjZVJlc3BvbnNlJiBtX3Jlc3BvbnNlOworICAgIFNv dXBTZXNzaW9uKiBtX3Nlc3Npb247CiAgICAgVmVjdG9yPGNoYXI+JiBtX2RhdGE7CiAgICAgYm9v bCBtX2ZpbmlzaGVkOwogICAgIEdSZWZQdHI8R01haW5Mb29wPiBtX21haW5Mb29wOwpAQCAtMTUx LDkgKzE3MiwxNCBAQCBSZXNvdXJjZUhhbmRsZUludGVybmFsOjp+UmVzb3VyY2VIYW5kbGVJbnRl cm5hbCgpCiB7CiB9CiAKK3N0YXRpYyBTb3VwU2Vzc2lvbiogc2Vzc2lvbkZyb21Db250ZXh0KE5l dHdvcmtpbmdDb250ZXh0KiBjb250ZXh0KQoreworICAgIHJldHVybiAoY29udGV4dCAmJiBjb250 ZXh0LT5pc1ZhbGlkKCkpID8gY29udGV4dC0+c291cFNlc3Npb24oKSA6IFJlc291cmNlSGFuZGxl OjpkZWZhdWx0U2Vzc2lvbigpOworfQorCiBTb3VwU2Vzc2lvbiogUmVzb3VyY2VIYW5kbGVJbnRl cm5hbDo6c291cFNlc3Npb24oKQogewotICAgIHJldHVybiAobV9jb250ZXh0ICYmIG1fY29udGV4 dC0+aXNWYWxpZCgpKSA/IG1fY29udGV4dC0+c291cFNlc3Npb24oKSA6IFJlc291cmNlSGFuZGxl OjpkZWZhdWx0U2Vzc2lvbigpOworICAgIHJldHVybiBzZXNzaW9uRnJvbUNvbnRleHQobV9jb250 ZXh0LmdldCgpKTsKIH0KIAogUmVzb3VyY2VIYW5kbGU6On5SZXNvdXJjZUhhbmRsZSgpCkBAIC03 NTEsNyArNzc3LDcgQEAgdm9pZCBSZXNvdXJjZUhhbmRsZTo6bG9hZFJlc291cmNlU3luY2hyb25v dXNseShOZXR3b3JraW5nQ29udGV4dCogY29udGV4dCwgY29uc3QKICAgICB9CiAjZW5kaWYKIAot ICAgIFdlYkNvcmVTeW5jaHJvbm91c0xvYWRlciBzeW5jTG9hZGVyKGVycm9yLCByZXNwb25zZSwg ZGF0YSk7CisgICAgV2ViQ29yZVN5bmNocm9ub3VzTG9hZGVyIHN5bmNMb2FkZXIoZXJyb3IsIHJl c3BvbnNlLCBzZXNzaW9uRnJvbUNvbnRleHQoY29udGV4dCksIGRhdGEpOwogICAgIFJlZlB0cjxS ZXNvdXJjZUhhbmRsZT4gaGFuZGxlID0gY3JlYXRlKGNvbnRleHQsIHJlcXVlc3QsICZzeW5jTG9h ZGVyLCBmYWxzZSAvKmRlZmVyc0xvYWRpbmcqLywgZmFsc2UgLypzaG91bGRDb250ZW50U25pZmYq Lyk7CiAgICAgaWYgKCFoYW5kbGUpCiAgICAgICAgIHJldHVybjsK