174421 2017-07-12 06:38:37 -0700 Web Automation: evaluateJavaScriptFunction should start the callback timeout after the function is applied 2017-07-13 09:22:13 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 cgarcia webkit-unassigned bburg joepeck oldest_to_newest 1328023 0 cgarcia 2017-07-12 06:38:37 -0700 This is causing selenium test testShouldNotTimeoutIfScriptCallsbackInsideAZeroTimeout to fail, because JavaScriptTimeout error is generated unexpectedly. When no script timeout is specified, 0 is used by default, which means we do a setTimeout with 0 and then the script does another setTimeout with 0, but ours is dispatched before and reportTimeoutError is called. We should start our timeout after applying the function, and only if the result hasn't been reported yet. FAIL test/selenium/webdriver/common/executing_async_javascript_tests.py::testShouldNotTimeoutIfScriptCallsbackInsideAZeroTimeout[WebKitGTK] ========================================================================================== FAILURES ========================================================================================== _____________________________________________________________ testShouldNotTimeoutIfScriptCallsbackInsideAZeroTimeout[WebKitGTK] _____________________________________________________________ driver = <selenium.webdriver.webkitgtk.webdriver.WebDriver (session="0ebfd066-81e2-4b84-9012-23eced4eed4c")>, pages = <conftest.Pages object at 0x7f5b9e19bfd0> def testShouldNotTimeoutIfScriptCallsbackInsideAZeroTimeout(driver, pages): pages.load("ajaxy_page.html") driver.execute_async_script( """var callback = arguments[arguments.length - 1]; > window.setTimeout(function() { callback(123); }, 0)""") E TimeoutException: Message: selenium/webdriver/remote/errorhandler.py:193: TimeoutException 1328024 1 315235 cgarcia 2017-07-12 06:40:30 -0700 Created attachment 315235 Patch 1328113 2 315235 bburg 2017-07-12 10:28:28 -0700 Comment on attachment 315235 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315235&action=review r=me For some reason, this test passes in safaridriver without the patch. I agree that this is a valid fix though. > Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:54 > + let reportResult = (result) => { clearTimeout(timeoutIdentifier); resultCallback(frameID, callbackID, this._jsonStringify(result), false); resultReported = true; }; This is getting unwieldy, can you add newlines to the function? The clearTimeout won't do anything if the function being evaluated calls the callback/completion handler synchronously. I guess that's okay. 1328385 3 cgarcia 2017-07-12 23:22:17 -0700 (In reply to Brian Burg from comment #2) > Comment on attachment 315235 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315235&action=review > > r=me > > For some reason, this test passes in safaridriver without the patch. I agree > that this is a valid fix though. Could it be that in mac Timers implementation two timers scheduled in the same run loop cycle with the same time interval (0) could be dispatched in any order? > > Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:54 > > + let reportResult = (result) => { clearTimeout(timeoutIdentifier); resultCallback(frameID, callbackID, this._jsonStringify(result), false); resultReported = true; }; > > This is getting unwieldy, can you add newlines to the function? Sure. > The clearTimeout won't do anything if the function being evaluated calls the > callback/completion handler synchronously. I guess that's okay. I'm assuming clearTimeout(0) does nothing, but maybe it's better to simply check it before calling it. 1328387 4 cgarcia 2017-07-12 23:34:31 -0700 Committed r219442: <http://trac.webkit.org/changeset/219442> 1328453 5 bburg 2017-07-13 09:22:13 -0700 (In reply to Carlos Garcia Campos from comment #3) > (In reply to Brian Burg from comment #2) > > Comment on attachment 315235 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=315235&action=review > > > > r=me > > > > For some reason, this test passes in safaridriver without the patch. I agree > > that this is a valid fix though. > > Could it be that in mac Timers implementation two timers scheduled in the > same run loop cycle with the same time interval (0) could be dispatched in > any order? No, DOMTimers are scheduled FIFO. 315235 2017-07-12 06:40:30 -0700 2017-07-12 10:28:28 -0700 Patch wk2-automation-script-timeout.diff text/plain 2787 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA2NThmNjE4NjEwMi4uMDIwNTBlN2M5MTkgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJLaXQyL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKQEAgLTEs NSArMSwyMSBAQAogMjAxNy0wNy0xMiAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGln YWxpYS5jb20+CiAKKyAgICAgICAgV2ViIEF1dG9tYXRpb246IGV2YWx1YXRlSmF2YVNjcmlwdEZ1 bmN0aW9uIHNob3VsZCBzdGFydCB0aGUgY2FsbGJhY2sgdGltZW91dCBhZnRlciB0aGUgZnVuY3Rp b24gaXMgYXBwbGllZAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTc0NDIxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgVGhpcyBpcyBjYXVzaW5nIHNlbGVuaXVtIHRlc3QgdGVzdFNob3VsZE5vdFRpbWVvdXRJ ZlNjcmlwdENhbGxzYmFja0luc2lkZUFaZXJvVGltZW91dCB0byBmYWlsLCBiZWNhdXNlCisgICAg ICAgIEphdmFTY3JpcHRUaW1lb3V0IGVycm9yIGlzIGdlbmVyYXRlZCB1bmV4cGVjdGVkbHkuIFdo ZW4gbm8gc2NyaXB0IHRpbWVvdXQgaXMgc3BlY2lmaWVkLCAwIGlzIHVzZWQgYnkgZGVmYXVsdCwK KyAgICAgICAgd2hpY2ggbWVhbnMgd2UgZG8gYSBzZXRUaW1lb3V0IHdpdGggMCBhbmQgdGhlbiB0 aGUgc2NyaXB0IGRvZXMgYW5vdGhlciBzZXRUaW1lb3V0IHdpdGggMCwgYnV0IG91cnMgaXMgZGlz cGF0Y2hlZAorICAgICAgICBiZWZvcmUgYW5kIHJlcG9ydFRpbWVvdXRFcnJvciBpcyBjYWxsZWQu IFdlIHNob3VsZCBzdGFydCBvdXIgdGltZW91dCBhZnRlciBhcHBseWluZyB0aGUgZnVuY3Rpb24s IGFuZCBvbmx5IGlmCisgICAgICAgIHRoZSByZXN1bHQgaGFzbid0IGJlZW4gcmVwb3J0ZWQgeWV0 LgorCisgICAgICAgICogV2ViUHJvY2Vzcy9BdXRvbWF0aW9uL1dlYkF1dG9tYXRpb25TZXNzaW9u UHJveHkuanM6CisgICAgICAgIChsZXQuQXV0b21hdGlvblNlc3Npb25Qcm94eS5wcm90b3R5cGUu ZXZhbHVhdGVKYXZhU2NyaXB0RnVuY3Rpb24pOgorCisyMDE3LTA3LTEyICBDYXJsb3MgR2FyY2lh IENhbXBvcyAgPGNnYXJjaWFAaWdhbGlhLmNvbT4KKwogICAgICAgICBXZWIgQXV0b21hdGlvbjog dXBzdHJlYW0gc2FmYXJpZHJpdmVyJ3MgSmF2YVNjcmlwdCBhdG9tIGltcGxlbWVudGF0aW9ucwog ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTcyMDYwCiAg ICAgICAgIDxyZGFyOi8vcHJvYmxlbS8zMjE2ODE4Nz4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJL aXQyL1dlYlByb2Nlc3MvQXV0b21hdGlvbi9XZWJBdXRvbWF0aW9uU2Vzc2lvblByb3h5LmpzIGIv U291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9BdXRvbWF0aW9uL1dlYkF1dG9tYXRpb25TZXNzaW9u UHJveHkuanMKaW5kZXggYjFlYTk3Mjk5OTEuLjA4NWMzOTBhODViIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViS2l0Mi9XZWJQcm9jZXNzL0F1dG9tYXRpb24vV2ViQXV0b21hdGlvblNlc3Npb25Qcm94 eS5qcworKysgYi9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL0F1dG9tYXRpb24vV2ViQXV0b21h dGlvblNlc3Npb25Qcm94eS5qcwpAQCAtNDksMTUgKzQ5LDE2IEBAIGxldCBBdXRvbWF0aW9uU2Vz c2lvblByb3h5ID0gY2xhc3MgQXV0b21hdGlvblNlc3Npb25Qcm94eQogICAgICAgICBsZXQgYXJn dW1lbnRWYWx1ZXMgPSBhcmd1bWVudFN0cmluZ3MubWFwKHRoaXMuX2pzb25QYXJzZSwgdGhpcyk7 CiAKICAgICAgICAgbGV0IHRpbWVvdXRJZGVudGlmaWVyID0gMDsKKyAgICAgICAgbGV0IHJlc3Vs dFJlcG9ydGVkID0gZmFsc2U7CiAKLSAgICAgICAgbGV0IHJlcG9ydFJlc3VsdCA9IChyZXN1bHQp ID0+IHsgY2xlYXJUaW1lb3V0KHRpbWVvdXRJZGVudGlmaWVyKTsgcmVzdWx0Q2FsbGJhY2soZnJh bWVJRCwgY2FsbGJhY2tJRCwgdGhpcy5fanNvblN0cmluZ2lmeShyZXN1bHQpLCBmYWxzZSk7IH07 CisgICAgICAgIGxldCByZXBvcnRSZXN1bHQgPSAocmVzdWx0KSA9PiB7IGNsZWFyVGltZW91dCh0 aW1lb3V0SWRlbnRpZmllcik7IHJlc3VsdENhbGxiYWNrKGZyYW1lSUQsIGNhbGxiYWNrSUQsIHRo aXMuX2pzb25TdHJpbmdpZnkocmVzdWx0KSwgZmFsc2UpOyByZXN1bHRSZXBvcnRlZCA9IHRydWU7 IH07CiAgICAgICAgIGxldCByZXBvcnRUaW1lb3V0RXJyb3IgPSAoKSA9PiB7IHJlc3VsdENhbGxi YWNrKGZyYW1lSUQsIGNhbGxiYWNrSUQsICJKYXZhU2NyaXB0VGltZW91dCIsIHRydWUpOyB9Owog CiAgICAgICAgIGlmIChleHBlY3RzSW1wbGljaXRDYWxsYmFja0FyZ3VtZW50KSB7Ci0gICAgICAg ICAgICB0aW1lb3V0SWRlbnRpZmllciA9IHNldFRpbWVvdXQocmVwb3J0VGltZW91dEVycm9yLCBj YWxsYmFja1RpbWVvdXQpOwotCiAgICAgICAgICAgICBhcmd1bWVudFZhbHVlcy5wdXNoKHJlcG9y dFJlc3VsdCk7CiAgICAgICAgICAgICBmdW5jdGlvblZhbHVlLmFwcGx5KG51bGwsIGFyZ3VtZW50 VmFsdWVzKTsKKyAgICAgICAgICAgIGlmICghcmVzdWx0UmVwb3J0ZWQpCisgICAgICAgICAgICAg ICAgdGltZW91dElkZW50aWZpZXIgPSBzZXRUaW1lb3V0KHJlcG9ydFRpbWVvdXRFcnJvciwgY2Fs bGJhY2tUaW1lb3V0KTsKICAgICAgICAgfSBlbHNlCiAgICAgICAgICAgICByZXBvcnRSZXN1bHQo ZnVuY3Rpb25WYWx1ZS5hcHBseShudWxsLCBhcmd1bWVudFZhbHVlcykpOwogICAgIH0K