137139 2014-09-25 22:31:29 -0700 Failed to run SunSpider 1.0.2 in Chrome 2016-03-09 13:30:28 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P1 Major --- 1 yang.gu webkit-unassigned barraclough benjamin commit-queue darin dewei_zhu fpizlo ggaren mjs msaboff rniwa oldest_to_newest 1037410 0 yang.gu 2014-09-25 22:31:29 -0700 I tried both Chrome for Linux and Android, and it had high possibility I could not get the final result. The reason is though we should have 26 cases for each round, but actually I could not get the results of all the them. If some of them are "undefined", the results.html could not parse the result. The issue is very easy to reproduce if you directly (not using web server) run sunspider 1.0.2 in Chrome. The root cause as I think is below: Snippet of code in function next and recordResult function next() { ... testFrame.contentDocument.open(); testFrame.contentDocument.write(testContents[testIndex]); testFrame.contentDocument.close(); window.setTimeout(next, 0); ... } function recordResult(time) { if (currentRepeat >= 0) // negative repeats are warmups output[currentRepeat][tests[testIndex]] = time; } Chrome uses standalone process to handle iframe. Thus there is a race condition here that before the recordResult is executed, function next is triggered in a new round and the iframe is filled with new content. The result from previous round gets no chance to be recorded. I changed the logic as below (will send out a patch), and everything recovers for me. function next() { ... testFrame.contentDocument.open(); testFrame.contentDocument.write(testContents[testIndex]); testFrame.contentDocument.close(); ... } function recordResult(time) { if (currentRepeat >= 0) // negative repeats are warmups output[currentRepeat][tests[testIndex]] = time; window.setTimeout(next, 0); } Please verify this issue. 1037421 1 238698 yang.gu 2014-09-25 23:41:52 -0700 Created attachment 238698 Patch 1037425 2 yang.gu 2014-09-25 23:51:27 -0700 Not sure if I have submitted the patch in a correct way, as I couldn't find commit message in my git log as below: Fix race condition in SunSpider 1.0.2 for Chrome Chrome uses standalone process to handle iframe. Thus there is a chance before recordResult is executed, a new round of test is triggered and the content of iframe is replaced. The result of previous case would never be recorded and result in errors in results.html. What should I do? 1037530 3 benjamin 2014-09-26 12:27:12 -0700 (In reply to comment #2) > Not sure if I have submitted the patch in a correct way, as I couldn't find commit message in my git log as below: > > Fix race condition in SunSpider 1.0.2 for Chrome > > Chrome uses standalone process to handle iframe. Thus there is a chance > before recordResult is executed, a new round of test is triggered and > the content of iframe is replaced. The result of previous case would > never be recorded and result in errors in results.html. > > What should I do? The complete explanation should be in your changelog. 1037717 4 238795 yang.gu 2014-09-27 17:46:53 -0700 Created attachment 238795 Patch 1037718 5 yang.gu 2014-09-27 17:48:28 -0700 I updated the patch with new ChangeLog. Please take a review again. Thanks! 1039419 6 ggaren 2014-10-03 17:11:56 -0700 The document.open/write/close APIs are synchronous. So, the target document should call back to recordResult synchronously before any new loading takes place. What is it about Chrome's process model that prevents document.open/write/close from being synchronous? Is this a bug in Chrome? Why is a timeout of 0 sufficient? If the target iframe is truly decoupled and asynchronous, the required timeout could be unpredictable, and much longer than 0. 1172542 7 238795 darin 2016-03-09 09:00:37 -0800 Comment on attachment 238795 Patch Nk we won't make this change. SunSpider is mostly obsolete and people should use JetStream http://browserbench.org/JetStream/ 1172543 8 darin 2016-03-09 09:00:59 -0800 I meant to say I think. 1172593 9 fpizlo 2016-03-09 10:06:15 -0800 (In reply to comment #7) > Comment on attachment 238795 [details] > Patch > > Nk we won't make this change. SunSpider is mostly obsolete and people should > use JetStream http://browserbench.org/JetStream/ I think that if this can be fixed easily then we would do it. We still let people run SunSpider and it's a shame if the benchmark itself has a bug that causes it to not give any results in some cases. 1172654 10 darin 2016-03-09 11:58:10 -0800 OK, would you like to review the patch then? 1172662 11 238795 fpizlo 2016-03-09 12:09:41 -0800 Comment on attachment 238795 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238795&action=review I'll let the r- stand because I don't agree with the diagnosis or fix. > PerformanceTests/SunSpider/ChangeLog:11 > + Chrome uses standalone process to handle iframe. Thus there is a > + chance before recordResult is executed, a new round of test is > + triggered and the content of iframe is replaced. The result of > + previous case would never be recorded and result in errors in > + results.html. I don't buy this. To my understanding, Chrome doesn't do it this way. > PerformanceTests/SunSpider/ChangeLog:12 > + The commit also removes some unexpected trailing whitespaces. We usually don't mix style changes with functional changes like this, especially when it's just whitespace. > PerformanceTests/SunSpider/resources/driver-TEMPLATE.html:29 > - OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. Please revert. > PerformanceTests/SunSpider/resources/driver-TEMPLATE.html:57 > - for (var start = new Date; new Date - start < warmupMS; ) { > + for (var start = new Date; new Date - start < warmupMS;) { Revert. > PerformanceTests/SunSpider/resources/driver-TEMPLATE.html:65 > -function start() > +function start() Revert. 1172725 12 238795 dewei_zhu 2016-03-09 13:30:28 -0800 Comment on attachment 238795 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238795&action=review >> PerformanceTests/SunSpider/ChangeLog:11 >> + results.html. > > I don't buy this. To my understanding, Chrome doesn't do it this way. Correct me if I'm wrong. According to our observation, the problem is that we are loading sub-resources (<link rel="stylesheet" href="../sunspider.css">) in the tests. This will trigger a separate thread loading the resource. And next round could happen before resource loading is finished. I submitted a related patch for benchmark automation. https://bugs.webkit.org/show_bug.cgi?id=152938 Solution proposed by this patch is one of the options when we tried to fix this. But since 'sunspider.css' is not used, and changing the the way runs the test sounds more risky, we decide to remove those resource in the tests. 238698 2014-09-25 23:41:52 -0700 2014-09-27 17:46:42 -0700 Patch bug-137139-20140926143855.patch text/plain 2661 yang.gu U3VidmVyc2lvbiBSZXZpc2lvbjogMTczOTkxCmRpZmYgLS1naXQgYS9QZXJmb3JtYW5jZVRlc3Rz L1N1blNwaWRlci9DaGFuZ2VMb2cgYi9QZXJmb3JtYW5jZVRlc3RzL1N1blNwaWRlci9DaGFuZ2VM b2cKaW5kZXggOWQ2MmJjMTZjNmQ2ZWE2MWNmMTBhOGExOTczYmU5NmJlN2NlZDMzMC4uMzE1ZGFm MjU0MGE5YzI2ODQ2NTI1ZmRlOTZhMzdiNTgxYjUwYmI3OSAxMDA2NDQKLS0tIGEvUGVyZm9ybWFu Y2VUZXN0cy9TdW5TcGlkZXIvQ2hhbmdlTG9nCisrKyBiL1BlcmZvcm1hbmNlVGVzdHMvU3VuU3Bp ZGVyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEyIEBACisyMDE0LTA5LTI1ICBZYW5nIEd1ICA8eWFu Zy5ndUBpbnRlbC5jb20+CisKKyAgICAgICAgRmFpbGVkIHRvIHJ1biBTdW5TcGlkZXIgMS4wLjIg aW4gQ2hyb21lCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMzcxMzkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICAqIHJlc291cmNlcy9kcml2ZXItVEVNUExBVEUuaHRtbDoKKwogMjAxNC0wNy0wOCAgR2VyZ28g QmFsb2doICA8Z2JhbG9naC51LXN6ZWdlZEBwYXJ0bmVyLnNhbXN1bmcuY29tPgogCiAgICAgICAg IEFkZCBvcHRpb24gdG8gc3BlY2lmeSBvdXRwdXQgZmlsZSBsb2NhdGlvbiBpbiBzdW5zcGlkZXIu CmRpZmYgLS1naXQgYS9QZXJmb3JtYW5jZVRlc3RzL1N1blNwaWRlci9yZXNvdXJjZXMvZHJpdmVy LVRFTVBMQVRFLmh0bWwgYi9QZXJmb3JtYW5jZVRlc3RzL1N1blNwaWRlci9yZXNvdXJjZXMvZHJp dmVyLVRFTVBMQVRFLmh0bWwKaW5kZXggZTY0ZmIyM2M3MTdlNWFkN2JlMWY2OGQwMjNjNDI5MTA5 ZGMzNmJkNy4uZmU2MzM4MTI3ZGNkNDY3ZDAwNTYxZDAyNjI4YmQ2MGM2ZDUzY2M0NSAxMDA2NDQK LS0tIGEvUGVyZm9ybWFuY2VUZXN0cy9TdW5TcGlkZXIvcmVzb3VyY2VzL2RyaXZlci1URU1QTEFU RS5odG1sCisrKyBiL1BlcmZvcm1hbmNlVGVzdHMvU3VuU3BpZGVyL3Jlc291cmNlcy9kcml2ZXIt VEVNUExBVEUuaHRtbApAQCAtMjYsNyArMjYsNyBAQAogIFBST0ZJVFM7IE9SIEJVU0lORVNTIElO VEVSUlVQVElPTikgSE9XRVZFUiBDQVVTRUQgQU5EIE9OIEFOWSBUSEVPUlkKICBPRiBMSUFCSUxJ VFksIFdIRVRIRVIgSU4gQ09OVFJBQ1QsIFNUUklDVCBMSUFCSUxJVFksIE9SIFRPUlQKICAoSU5D TFVESU5HIE5FR0xJR0VOQ0UgT1IgT1RIRVJXSVNFKSBBUklTSU5HIElOIEFOWSBXQVkgT1VUIE9G IFRIRSBVU0UKLSBPRiBUSElTIFNPRlRXQVJFLCBFVkVOIElGIEFEVklTRUQgT0YgVEhFIFBPU1NJ QklMSVRZIE9GIFNVQ0ggREFNQUdFLiAKKyBPRiBUSElTIFNPRlRXQVJFLCBFVkVOIElGIEFEVklT RUQgT0YgVEhFIFBPU1NJQklMSVRZIE9GIFNVQ0ggREFNQUdFLgogLS0+CiAKIDx0aXRsZT5TdW5T cGlkZXIgMS4wLjIgSmF2YVNjcmlwdCBCZW5jaG1hcmsgKEBTVUlURUAgdGVzdCBzdWl0ZSAtIElu IFByb2dyZXNzLi4uKTwvdGl0bGU+CkBAIC01NCw3ICs1NCw3IEBAIGZvciAodmFyIGkgPSAwOyBp IDwgb3V0cHV0Lmxlbmd0aDsgaSsrKSB7CiAKIGZ1bmN0aW9uIHdhcm11cCgpCiB7Ci0gICAgZm9y ICh2YXIgc3RhcnQgPSBuZXcgRGF0ZTsgbmV3IERhdGUgLSBzdGFydCA8IHdhcm11cE1TOyApIHsK KyAgICBmb3IgKHZhciBzdGFydCA9IG5ldyBEYXRlOyBuZXcgRGF0ZSAtIHN0YXJ0IDwgd2FybXVw TVM7KSB7CiAgICAgICAgIGZvciAodmFyIGkgPSAwOyBpIDwgMTAwOyArK2kpIHsKICAgICAgICAg ICAgIGlmIChNYXRoLmF0YW4oTWF0aC5hY29zKE1hdGguYXNpbihNYXRoLnJhbmRvbSgpKSkpID4g NCkgLy8gQWx3YXlzIGZhbHNlLgogICAgICAgICAgICAgICAgIHJldHVybjsKQEAgLTYyLDcgKzYy LDcgQEAgZnVuY3Rpb24gd2FybXVwKCkKICAgICB9CiB9CiAKLWZ1bmN0aW9uIHN0YXJ0KCkgCitm dW5jdGlvbiBzdGFydCgpCiB7CiAgICAgd2luZG93LnNldFRpbWVvdXQobmV4dCwgMTI4KTsKIH0K QEAgLTgzLDkgKzgzLDcgQEAgZnVuY3Rpb24gbmV4dCgpCiAgICAgICAgIHRlc3RGcmFtZS5jb250 ZW50RG9jdW1lbnQub3BlbigpOwogICAgICAgICB0ZXN0RnJhbWUuY29udGVudERvY3VtZW50Lndy aXRlKHRlc3RDb250ZW50c1t0ZXN0SW5kZXhdKTsKICAgICAgICAgdGVzdEZyYW1lLmNvbnRlbnRE b2N1bWVudC5jbG9zZSgpOwotCi0gICAgICAgIHdpbmRvdy5zZXRUaW1lb3V0KG5leHQsIDApOwot ICAgIH0gZWxzZSBpZiAoKytjdXJyZW50UmVwZWF0IDwgcmVwZWF0Q291bnQpIHsgCisgICAgfSBl bHNlIGlmICgrK2N1cnJlbnRSZXBlYXQgPCByZXBlYXRDb3VudCkgewogICAgICAgICBkb2N1bWVu dC5nZXRFbGVtZW50QnlJZCgiY291bnRkb3duIikuaW5uZXJIVE1MID0gcmVwZWF0Q291bnQgLSBj dXJyZW50UmVwZWF0OwogICAgICAgICB0ZXN0SW5kZXggPSAtMTsKIApAQCAtOTksNiArOTcsOCBA QCBmdW5jdGlvbiByZWNvcmRSZXN1bHQodGltZSkKIHsKICAgICBpZiAoY3VycmVudFJlcGVhdCA+ PSAwKSAvLyBuZWdhdGl2ZSByZXBlYXRzIGFyZSB3YXJtdXBzCiAgICAgICAgIG91dHB1dFtjdXJy ZW50UmVwZWF0XVt0ZXN0c1t0ZXN0SW5kZXhdXSA9IHRpbWU7CisKKyAgICB3aW5kb3cuc2V0VGlt ZW91dChuZXh0LCAwKTsKIH0KIAogZnVuY3Rpb24gZmluaXNoKCkK 238795 2014-09-27 17:46:53 -0700 2016-03-09 09:00:37 -0800 Patch bug-137139-20140928084404.patch text/plain 3053 yang.gu U3VidmVyc2lvbiBSZXZpc2lvbjogMTczOTkxCmRpZmYgLS1naXQgYS9QZXJmb3JtYW5jZVRlc3Rz L1N1blNwaWRlci9DaGFuZ2VMb2cgYi9QZXJmb3JtYW5jZVRlc3RzL1N1blNwaWRlci9DaGFuZ2VM b2cKaW5kZXggOWQ2MmJjMTZjNmQ2ZWE2MWNmMTBhOGExOTczYmU5NmJlN2NlZDMzMC4uODY2YzJm MzBjYmY3MTFjMmEwNTA0NmE1OWFiMjljMzk5YmNiMDI2MyAxMDA2NDQKLS0tIGEvUGVyZm9ybWFu Y2VUZXN0cy9TdW5TcGlkZXIvQ2hhbmdlTG9nCisrKyBiL1BlcmZvcm1hbmNlVGVzdHMvU3VuU3Bp ZGVyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDE0LTA5LTI1ICBZYW5nIEd1ICA8eWFu Zy5ndUBpbnRlbC5jb20+CisKKyAgICAgICAgRml4IHJhY2UgY29uZGl0aW9uIGluIFN1blNwaWRl ciAxLjAuMiBmb3IgQ2hyb21lCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xMzcxMzkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KyAgICAgICAgQ2hyb21lIHVzZXMgc3RhbmRhbG9uZSBwcm9jZXNzIHRvIGhhbmRsZSBpZnJhbWUu IFRodXMgdGhlcmUgaXMgYQorICAgICAgICBjaGFuY2UgYmVmb3JlIHJlY29yZFJlc3VsdCBpcyBl eGVjdXRlZCwgYSBuZXcgcm91bmQgb2YgdGVzdCBpcworICAgICAgICB0cmlnZ2VyZWQgYW5kIHRo ZSBjb250ZW50IG9mIGlmcmFtZSBpcyByZXBsYWNlZC4gVGhlIHJlc3VsdCBvZgorICAgICAgICBw cmV2aW91cyBjYXNlIHdvdWxkIG5ldmVyIGJlIHJlY29yZGVkIGFuZCByZXN1bHQgaW4gZXJyb3Jz IGluCisgICAgICAgIHJlc3VsdHMuaHRtbC4KKyAgICAgICAgVGhlIGNvbW1pdCBhbHNvIHJlbW92 ZXMgc29tZSB1bmV4cGVjdGVkIHRyYWlsaW5nIHdoaXRlc3BhY2VzLgorCisgICAgICAgICogcmVz b3VyY2VzL2RyaXZlci1URU1QTEFURS5odG1sOgorCiAyMDE0LTA3LTA4ICBHZXJnbyBCYWxvZ2gg IDxnYmFsb2doLnUtc3plZ2VkQHBhcnRuZXIuc2Ftc3VuZy5jb20+CiAKICAgICAgICAgQWRkIG9w dGlvbiB0byBzcGVjaWZ5IG91dHB1dCBmaWxlIGxvY2F0aW9uIGluIHN1bnNwaWRlci4KZGlmZiAt LWdpdCBhL1BlcmZvcm1hbmNlVGVzdHMvU3VuU3BpZGVyL3Jlc291cmNlcy9kcml2ZXItVEVNUExB VEUuaHRtbCBiL1BlcmZvcm1hbmNlVGVzdHMvU3VuU3BpZGVyL3Jlc291cmNlcy9kcml2ZXItVEVN UExBVEUuaHRtbAppbmRleCBlNjRmYjIzYzcxN2U1YWQ3YmUxZjY4ZDAyM2M0MjkxMDlkYzM2YmQ3 Li5mZTYzMzgxMjdkY2Q0NjdkMDA1NjFkMDI2MjhiZDYwYzZkNTNjYzQ1IDEwMDY0NAotLS0gYS9Q ZXJmb3JtYW5jZVRlc3RzL1N1blNwaWRlci9yZXNvdXJjZXMvZHJpdmVyLVRFTVBMQVRFLmh0bWwK KysrIGIvUGVyZm9ybWFuY2VUZXN0cy9TdW5TcGlkZXIvcmVzb3VyY2VzL2RyaXZlci1URU1QTEFU RS5odG1sCkBAIC0yNiw3ICsyNiw3IEBACiAgUFJPRklUUzsgT1IgQlVTSU5FU1MgSU5URVJSVVBU SU9OKSBIT1dFVkVSIENBVVNFRCBBTkQgT04gQU5ZIFRIRU9SWQogIE9GIExJQUJJTElUWSwgV0hF VEhFUiBJTiBDT05UUkFDVCwgU1RSSUNUIExJQUJJTElUWSwgT1IgVE9SVAogIChJTkNMVURJTkcg TkVHTElHRU5DRSBPUiBPVEhFUldJU0UpIEFSSVNJTkcgSU4gQU5ZIFdBWSBPVVQgT0YgVEhFIFVT RQotIE9GIFRISVMgU09GVFdBUkUsIEVWRU4gSUYgQURWSVNFRCBPRiBUSEUgUE9TU0lCSUxJVFkg T0YgU1VDSCBEQU1BR0UuIAorIE9GIFRISVMgU09GVFdBUkUsIEVWRU4gSUYgQURWSVNFRCBPRiBU SEUgUE9TU0lCSUxJVFkgT0YgU1VDSCBEQU1BR0UuCiAtLT4KIAogPHRpdGxlPlN1blNwaWRlciAx LjAuMiBKYXZhU2NyaXB0IEJlbmNobWFyayAoQFNVSVRFQCB0ZXN0IHN1aXRlIC0gSW4gUHJvZ3Jl c3MuLi4pPC90aXRsZT4KQEAgLTU0LDcgKzU0LDcgQEAgZm9yICh2YXIgaSA9IDA7IGkgPCBvdXRw dXQubGVuZ3RoOyBpKyspIHsKIAogZnVuY3Rpb24gd2FybXVwKCkKIHsKLSAgICBmb3IgKHZhciBz dGFydCA9IG5ldyBEYXRlOyBuZXcgRGF0ZSAtIHN0YXJ0IDwgd2FybXVwTVM7ICkgeworICAgIGZv ciAodmFyIHN0YXJ0ID0gbmV3IERhdGU7IG5ldyBEYXRlIC0gc3RhcnQgPCB3YXJtdXBNUzspIHsK ICAgICAgICAgZm9yICh2YXIgaSA9IDA7IGkgPCAxMDA7ICsraSkgewogICAgICAgICAgICAgaWYg KE1hdGguYXRhbihNYXRoLmFjb3MoTWF0aC5hc2luKE1hdGgucmFuZG9tKCkpKSkgPiA0KSAvLyBB bHdheXMgZmFsc2UuCiAgICAgICAgICAgICAgICAgcmV0dXJuOwpAQCAtNjIsNyArNjIsNyBAQCBm dW5jdGlvbiB3YXJtdXAoKQogICAgIH0KIH0KIAotZnVuY3Rpb24gc3RhcnQoKSAKK2Z1bmN0aW9u IHN0YXJ0KCkKIHsKICAgICB3aW5kb3cuc2V0VGltZW91dChuZXh0LCAxMjgpOwogfQpAQCAtODMs OSArODMsNyBAQCBmdW5jdGlvbiBuZXh0KCkKICAgICAgICAgdGVzdEZyYW1lLmNvbnRlbnREb2N1 bWVudC5vcGVuKCk7CiAgICAgICAgIHRlc3RGcmFtZS5jb250ZW50RG9jdW1lbnQud3JpdGUodGVz dENvbnRlbnRzW3Rlc3RJbmRleF0pOwogICAgICAgICB0ZXN0RnJhbWUuY29udGVudERvY3VtZW50 LmNsb3NlKCk7Ci0KLSAgICAgICAgd2luZG93LnNldFRpbWVvdXQobmV4dCwgMCk7Ci0gICAgfSBl bHNlIGlmICgrK2N1cnJlbnRSZXBlYXQgPCByZXBlYXRDb3VudCkgeyAKKyAgICB9IGVsc2UgaWYg KCsrY3VycmVudFJlcGVhdCA8IHJlcGVhdENvdW50KSB7CiAgICAgICAgIGRvY3VtZW50LmdldEVs ZW1lbnRCeUlkKCJjb3VudGRvd24iKS5pbm5lckhUTUwgPSByZXBlYXRDb3VudCAtIGN1cnJlbnRS ZXBlYXQ7CiAgICAgICAgIHRlc3RJbmRleCA9IC0xOwogCkBAIC05OSw2ICs5Nyw4IEBAIGZ1bmN0 aW9uIHJlY29yZFJlc3VsdCh0aW1lKQogewogICAgIGlmIChjdXJyZW50UmVwZWF0ID49IDApIC8v IG5lZ2F0aXZlIHJlcGVhdHMgYXJlIHdhcm11cHMKICAgICAgICAgb3V0cHV0W2N1cnJlbnRSZXBl YXRdW3Rlc3RzW3Rlc3RJbmRleF1dID0gdGltZTsKKworICAgIHdpbmRvdy5zZXRUaW1lb3V0KG5l eHQsIDApOwogfQogCiBmdW5jdGlvbiBmaW5pc2goKQo=