170391 2017-04-02 10:04:35 -0700 Implement PromiseDeferredTimer for non CF based ports 2017-04-05 00:58:15 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Local Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=169187 Gtk P2 Normal --- 170390 1 cgarcia webkit-unassigned andersca buildbot darin fpizlo ggaren jfbastien keith_miller mark.lam mcatanzaro msaboff saam sam ysuzuki oldest_to_newest 1293535 0 cgarcia 2017-04-02 10:04:35 -0700 RunLoop handling is only implemented for CF causing several wasm tests to fail for other ports: wasm.yaml/wasm/js-api/Module-compile.js.default-wasm: ERROR: Unexpected exit code: 3 wasm.yaml/wasm/js-api/web-assembly-compile-parallel.js.default-wasm: ERROR: Unexpected exit code: 3 wasm.yaml/wasm/js-api/web-assembly-instantiate.js.default-wasm: ERROR: Unexpected exit code: 3 wasm.yaml/wasm/js-api/web-assembly-instantiate-parallel.js.default-wasm: ERROR: Unexpected exit code: 3 1293536 1 306055 cgarcia 2017-04-02 10:13:45 -0700 Created attachment 306055 Patch 1293543 2 306055 mcatanzaro 2017-04-02 11:05:58 -0700 Comment on attachment 306055 Patch It looks OK to me, but I wonder if the separate implementation for CF is really necessary. If it really is, then I'd expect RunLoop to be implemented in terms of CFRunLoop.... 1293567 3 306055 ysuzuki 2017-04-02 17:40:30 -0700 Comment on attachment 306055 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306055&action=review > Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp:-85 > +#if USE(CF) > CFRunLoopRun(); > -} It's unnecessary since we already have the assertion that CFRunLoopGetCurrent() == m_runLoop.get(). So, RunLoop::run() drives current thread RunLoop and it is OK for all the ports. 1293571 4 306055 mcatanzaro 2017-04-02 18:16:57 -0700 Comment on attachment 306055 Patch OK then, please rewrite to get rid of CFRunLoop. 1293572 5 ysuzuki 2017-04-02 18:20:49 -0700 (In reply to Michael Catanzaro from comment #4) > Comment on attachment 306055 [details] > Patch > > OK then, please rewrite to get rid of CFRunLoop. A bit worried thing is that, current RunLoop & RunLoop::Timer do not have the full features of CFRunLoop. For example, RunLoop is thread-safe, functions can be called from the other thread. But RunLoop::Timer is only available from the created thread. And current Heap's timers are touched from the other thread. So, I think we still need to use CFRunLoop right now. But at least, in runRunLoop, we can just use RunLoop::run(). But for doWork function, we still need CFRunLoop thing. 1293577 6 cgarcia 2017-04-02 22:34:47 -0700 (In reply to Yusuke Suzuki from comment #5) > (In reply to Michael Catanzaro from comment #4) > > Comment on attachment 306055 [details] > > Patch > > > > OK then, please rewrite to get rid of CFRunLoop. > > A bit worried thing is that, current RunLoop & RunLoop::Timer do not have > the full features of CFRunLoop. For example, RunLoop is thread-safe, > functions can be called from the other thread. But RunLoop::Timer is only > available from the created thread. And current Heap's timers are touched > from the other thread. > > So, I think we still need to use CFRunLoop right now. But at least, in > runRunLoop, we can just use RunLoop::run(). But for doWork function, we > still need CFRunLoop thing. Also note that iOS calls setRunLoop. Since CF timers already have a m_runLoop member I thought it was safer to use the member. I agree we could use RunLoop::run(), but since we need to keep the CF ifdefs for the stop, I think it's less confusing to have ifdefs for the run too. 1293579 7 ysuzuki 2017-04-02 22:38:56 -0700 (In reply to Carlos Garcia Campos from comment #6) > (In reply to Yusuke Suzuki from comment #5) > > (In reply to Michael Catanzaro from comment #4) > > > Comment on attachment 306055 [details] > > > Patch > > > > > > OK then, please rewrite to get rid of CFRunLoop. > > > > A bit worried thing is that, current RunLoop & RunLoop::Timer do not have > > the full features of CFRunLoop. For example, RunLoop is thread-safe, > > functions can be called from the other thread. But RunLoop::Timer is only > > available from the created thread. And current Heap's timers are touched > > from the other thread. > > > > So, I think we still need to use CFRunLoop right now. But at least, in > > runRunLoop, we can just use RunLoop::run(). But for doWork function, we > > still need CFRunLoop thing. > > Also note that iOS calls setRunLoop. Since CF timers already have a > m_runLoop member I thought it was safer to use the member. I agree we could > use RunLoop::run(), but since we need to keep the CF ifdefs for the stop, I > think it's less confusing to have ifdefs for the run too. OK, we should change & drop all the CF things when we use RunLoop & RunLoop::Timer thing exclusively. Eventually, we should change current JSRunLoopTimer to accept RunLoop instead of CFRunLoopRef thing. 1294429 8 cgarcia 2017-04-05 00:58:15 -0700 Committed r214936: <http://trac.webkit.org/changeset/214936> 306055 2017-04-02 10:13:45 -0700 2017-04-02 22:39:04 -0700 Patch jsc-promise-timer.diff text/plain 3518 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IGM5YTk2ZjVjNzJiLi5mOTgwNTgyNjhhNyAxMDA2 NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjEgQEAKIDIwMTctMDQtMDIgIENhcmxv cyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgogCisgICAgICAgIEltcGxlbWVu dCBQcm9taXNlRGVmZXJyZWRUaW1lciBmb3Igbm9uIENGIGJhc2VkIHBvcnRzCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzAzOTEKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBSdW5Mb29wIGhhbmRsaW5nIGlz IG9ubHkgaW1wbGVtZW50ZWQgZm9yIENGIGNhdXNpbmcgc2V2ZXJhbCB3YXNtIHRlc3RzIHRvIGZh aWwgZm9yIG90aGVyIHBvcnRzLgorCisgICAgICAgICoganNjLmNwcDoKKyAgICAgICAgKHJ1bkpT Qyk6IFJlbW92ZSBDRiBpZmRlZnMuCisgICAgICAgICogcnVudGltZS9Qcm9taXNlRGVmZXJyZWRU aW1lci5jcHA6CisgICAgICAgIChKU0M6OlByb21pc2VEZWZlcnJlZFRpbWVyOjpkb1dvcmspOiBB ZGQgbm9uIENGIGltcGxlbWVudGF0aW9uIHVzaW5nIFdURiBSdW5Mb29wLgorICAgICAgICAoSlND OjpQcm9taXNlRGVmZXJyZWRUaW1lcjo6cnVuUnVuTG9vcCk6IERpdHRvLgorICAgICAgICAqIHJ1 bnRpbWUvUHJvbWlzZURlZmVycmVkVGltZXIuaDoKKworMjAxNy0wNC0wMiAgQ2FybG9zIEdhcmNp YSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CisKICAgICAgICAgV2ViQXNzZW1ibHk6IHNl dmVyYWwgdGVzdHMgYWRkZWQgaW4gcjIxNDUwNCBjcmFzaCB3aGVuIGJ1aWxkaW5nIHdpdGggR0ND CiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzAzOTAK IApkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2pzYy5jcHAgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvanNjLmNwcAppbmRleCA1NTIyZDgyMWY1Zi4uYmM1MWUwMGU5YjEgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qc2MuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9qc2MuY3BwCkBAIC0zODEyLDkgKzM4MTIsNyBAQCBpbnQgcnVuSlNDKENvbW1hbmRM aW5lIG9wdGlvbnMsIGNvbnN0IEZ1bmMmIGZ1bmMpCiAKICAgICAgICAgdm0uZHJhaW5NaWNyb3Rh c2tzKCk7CiAgICAgfQotI2lmIFVTRShDRikKICAgICB2bS5wcm9taXNlRGVmZXJyZWRUaW1lci0+ cnVuUnVuTG9vcCgpOwotI2VuZGlmCiAKICAgICByZXN1bHQgPSBzdWNjZXNzICYmIChhc3luY1Rl c3RFeHBlY3RlZFBhc3NlcyA9PSBhc3luY1Rlc3RQYXNzZXMpID8gMCA6IDM7CiAKZGlmZiAtLWdp dCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1Byb21pc2VEZWZlcnJlZFRpbWVyLmNw cCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1Byb21pc2VEZWZlcnJlZFRpbWVyLmNw cAppbmRleCA3NGFmNjkxNzlhMi4uYTRjYzBmZjYyYTAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9ydW50aW1lL1Byb21pc2VEZWZlcnJlZFRpbWVyLmNwcAorKysgYi9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvcnVudGltZS9Qcm9taXNlRGVmZXJyZWRUaW1lci5jcHAKQEAgLTI5LDgg KzI5LDggQEAKICNpbmNsdWRlICJKU1Byb21pc2VEZWZlcnJlZC5oIgogI2luY2x1ZGUgIlN0cm9u Z0lubGluZXMuaCIKICNpbmNsdWRlICJWTS5oIgotCiAjaW5jbHVkZSA8d3RmL0xvY2tlci5oPgor I2luY2x1ZGUgPHd0Zi9SdW5Mb29wLmg+CiAKIG5hbWVzcGFjZSBKU0MgewogCkBAIC02OCwyMiAr NjgsMjggQEAgdm9pZCBQcm9taXNlRGVmZXJyZWRUaW1lcjo6ZG9Xb3JrKCkKICAgICAgICAgfQog ICAgIH0KIAotI2lmIFVTRShDRikKICAgICBpZiAobV9wZW5kaW5nUHJvbWlzZXMuaXNFbXB0eSgp ICYmIG1fc2hvdWxkU3RvcFJ1bkxvb3BXaGVuQWxsUHJvbWlzZXNGaW5pc2gpCisjaWYgVVNFKENG KQogICAgICAgICBDRlJ1bkxvb3BTdG9wKG1fcnVuTG9vcC5nZXQoKSk7CisjZWxzZQorICAgICAg ICBSdW5Mb29wOjpjdXJyZW50KCkuc3RvcCgpOwogI2VuZGlmCiB9CiAKLSNpZiBVU0UoQ0YpCiB2 b2lkIFByb21pc2VEZWZlcnJlZFRpbWVyOjpydW5SdW5Mb29wKCkKIHsKICAgICBBU1NFUlQoIW1f dm0tPmN1cnJlbnRUaHJlYWRJc0hvbGRpbmdBUElMb2NrKCkpOworI2lmIFVTRShDRikKICAgICBB U1NFUlQoQ0ZSdW5Mb29wR2V0Q3VycmVudCgpID09IG1fcnVuTG9vcC5nZXQoKSk7CisjZW5kaWYK ICAgICBtX3Nob3VsZFN0b3BSdW5Mb29wV2hlbkFsbFByb21pc2VzRmluaXNoID0gdHJ1ZTsKICAg ICBpZiAobV9wZW5kaW5nUHJvbWlzZXMuc2l6ZSgpKQorI2lmIFVTRShDRikKICAgICAgICAgQ0ZS dW5Mb29wUnVuKCk7Ci19CisjZWxzZQorICAgICAgICBSdW5Mb29wOjpydW4oKTsKICNlbmRpZgor fQogCiB2b2lkIFByb21pc2VEZWZlcnJlZFRpbWVyOjphZGRQZW5kaW5nUHJvbWlzZShKU1Byb21p c2VEZWZlcnJlZCogdGlja2V0LCBWZWN0b3I8U3Ryb25nPEpTQ2VsbD4+JiYgZGVwZW5kZW5jaWVz KQogewpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvUHJvbWlzZURl ZmVycmVkVGltZXIuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1Byb21pc2VEZWZl cnJlZFRpbWVyLmgKaW5kZXggY2MzYzMzZWI4NzIuLjk4MWU3NTM1YzcwIDEwMDY0NAotLS0gYS9T b3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9Qcm9taXNlRGVmZXJyZWRUaW1lci5oCisrKyBi L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1Byb21pc2VEZWZlcnJlZFRpbWVyLmgKQEAg LTYxLDE3ICs2MSwxNCBAQCBwdWJsaWM6CiAgICAgdm9pZCBzY2hlZHVsZUJsb2NrZWRUYXNrKEpT UHJvbWlzZURlZmVycmVkKiwgVGFzayYmKTsKIAogICAgIHZvaWQgc3RvcFJ1bm5pbmdUYXNrcygp IHsgbV9ydW5UYXNrcyA9IGZhbHNlOyB9Ci0jaWYgVVNFKENGKQorCiAgICAgSlNfRVhQT1JUX1BS SVZBVEUgdm9pZCBydW5SdW5Mb29wKCk7Ci0jZW5kaWYKIAogcHJpdmF0ZToKICAgICBIYXNoTWFw PEpTUHJvbWlzZURlZmVycmVkKiwgVmVjdG9yPFN0cm9uZzxKU0NlbGw+Pj4gbV9wZW5kaW5nUHJv bWlzZXM7CiAgICAgTG9jayBtX3Rhc2tMb2NrOwogICAgIGJvb2wgbV9ydW5UYXNrcyB7IHRydWUg fTsKLSNpZiBVU0UoQ0YpCiAgICAgYm9vbCBtX3Nob3VsZFN0b3BSdW5Mb29wV2hlbkFsbFByb21p c2VzRmluaXNoIHsgZmFsc2UgfTsKLSNlbmRpZgogICAgIFZlY3RvcjxzdGQ6OnR1cGxlPEpTUHJv bWlzZURlZmVycmVkKiwgVGFzaz4+IG1fdGFza3M7CiAgICAgSGFzaE1hcDxKU1Byb21pc2VEZWZl cnJlZCosIFZlY3RvcjxUYXNrPj4gbV9ibG9ja2VkVGFza3M7CiB9Owo=