185134 2018-04-30 09:44:44 -0700 Fix bad use of RunLoop::main().dispatch() in MessagePort::dispatchMessages() 2018-04-30 16:28:10 -0700 1 1 1 Unclassified WebKit DOM WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=185073 InRadar P2 Normal --- 1 cdumez cdumez ap beidson cdumez commit-queue dbates esprehn+autocc ews-watchlist ggaren kangil.han webkit-bug-importer youennf oldest_to_newest 1419003 0 cdumez 2018-04-30 09:44:44 -0700 Fix bad use of RunLoop::main().dispatch() in MessagePort::dispatchMessages(). This code runs on iOS WebKitLegacy and it is therefore unsafe to use RunLoop::main() here. We want to use callOnMainThread() instead to run code on the WebThread. 1419005 1 339116 cdumez 2018-04-30 09:45:42 -0700 Created attachment 339116 Patch 1419009 2 339116 ggaren 2018-04-30 09:51:44 -0700 Comment on attachment 339116 Patch r=me 1419047 3 339116 dbates 2018-04-30 10:59:25 -0700 Comment on attachment 339116 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339116&action=review > Source/WebCore/dom/MessagePort.cpp:-287 > - RunLoop::main().dispatch([completionCallback = WTFMove(completionCallback)] { Is there anything that we can do to prevent this kind of mistake from the getgo? It seems to be repeatedly made. 1419060 4 cdumez 2018-04-30 11:50:35 -0700 (In reply to Daniel Bates from comment #3) > Comment on attachment 339116 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339116&action=review > > > Source/WebCore/dom/MessagePort.cpp:-287 > > - RunLoop::main().dispatch([completionCallback = WTFMove(completionCallback)] { > > Is there anything that we can do to prevent this kind of mistake from the > getgo? It seems to be repeatedly made. I agree it would be nice. Maybe we could introduce a rule enforced by the style checker that RunLoop::main().dispatch() should never be used in WebCore? I don't have a better idea at the moment. 1419067 5 339116 commit-queue 2018-04-30 12:13:38 -0700 Comment on attachment 339116 Patch Clearing flags on attachment: 339116 Committed r231161: <https://trac.webkit.org/changeset/231161> 1419068 6 commit-queue 2018-04-30 12:13:40 -0700 All reviewed patches have been landed. Closing bug. 1419069 7 webkit-bug-importer 2018-04-30 12:14:31 -0700 <rdar://problem/39843751> 1419113 8 dbates 2018-04-30 14:16:25 -0700 (In reply to Chris Dumez from comment #4) > (In reply to Daniel Bates from comment #3) > > Comment on attachment 339116 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=339116&action=review > > > > > Source/WebCore/dom/MessagePort.cpp:-287 > > > - RunLoop::main().dispatch([completionCallback = WTFMove(completionCallback)] { > > > > Is there anything that we can do to prevent this kind of mistake from the > > getgo? It seems to be repeatedly made. > > I agree it would be nice. Maybe we could introduce a rule enforced by the > style checker that RunLoop::main().dispatch() should never be used in > WebCore? I don't have a better idea at the moment. Notice that the WebThread code is guarded by USE(WEB_THREAD) and we only enable this feature on iOS. Can we take advantage of this together with a static_assert() to cause a compile-time failure with a helpful message whenever someone makes use unsafe or potentially unsafe RunLoop functionality, including RunLoop::main().dispatch()? 1419165 9 cdumez 2018-04-30 16:28:10 -0700 (In reply to Daniel Bates from comment #8) > (In reply to Chris Dumez from comment #4) > > (In reply to Daniel Bates from comment #3) > > > Comment on attachment 339116 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=339116&action=review > > > > > > > Source/WebCore/dom/MessagePort.cpp:-287 > > > > - RunLoop::main().dispatch([completionCallback = WTFMove(completionCallback)] { > > > > > > Is there anything that we can do to prevent this kind of mistake from the > > > getgo? It seems to be repeatedly made. > > > > I agree it would be nice. Maybe we could introduce a rule enforced by the > > style checker that RunLoop::main().dispatch() should never be used in > > WebCore? I don't have a better idea at the moment. > > Notice that the WebThread code is guarded by USE(WEB_THREAD) and we only > enable this feature on iOS. Can we take advantage of this together with a > static_assert() to cause a compile-time failure with a helpful message > whenever someone makes use unsafe or potentially unsafe RunLoop > functionality, including RunLoop::main().dispatch()? RunLoop is in WTF and USE_WEB_THREAD is set to 1 when building WTF on iOS. Therefore, the static_assert(!USE_WEB_THREAD, "") would cause a build error on iOS, even though it is perfectly fine to call RunLoop::main().dispatch() from iOS WebKit2 code. I might be missing something. 339116 2018-04-30 09:45:42 -0700 2018-04-30 12:13:38 -0700 Patch bug-185134-20180430094541.patch text/plain 1818 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjMxMTU2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOTRhZDIwNzBhNGVhMzQ4 ZGM0MjAyNmYxOGI3NzdkODc1OTU3NTNkNi4uN2NkMTFlZGNmM2Y1ZjE1MWJhZTI1ODdkZTZiMzRk MmM1NmU3ZThlYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDE4LTA0LTMwICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgRml4IGJhZCB1c2Ugb2YgUnVu TG9vcDo6bWFpbigpLmRpc3BhdGNoKCkgaW4gTWVzc2FnZVBvcnQ6OmRpc3BhdGNoTWVzc2FnZXMo KQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTg1MTM0 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRml4IGJh ZCB1c2Ugb2YgUnVuTG9vcDo6bWFpbigpLmRpc3BhdGNoKCkgaW4gTWVzc2FnZVBvcnQ6OmRpc3Bh dGNoTWVzc2FnZXMoKS4gVGhpcyBjb2RlIHJ1bnMgb24gaU9TIFdlYktpdExlZ2FjeQorICAgICAg ICBhbmQgaXQgaXMgdGhlcmVmb3JlIHVuc2FmZSB0byB1c2UgUnVuTG9vcDo6bWFpbigpIGhlcmUu IFdlIHdhbnQgdG8gdXNlIGNhbGxPbk1haW5UaHJlYWQoKSBpbnN0ZWFkIHRvIHJ1biBjb2RlIG9u CisgICAgICAgIHRoZSBXZWJUaHJlYWQuCisKKyAgICAgICAgKiBkb20vTWVzc2FnZVBvcnQuY3Bw OgorICAgICAgICAoV2ViQ29yZTo6TWVzc2FnZVBvcnQ6OmRpc3BhdGNoTWVzc2FnZXMpOgorCiAy MDE4LTA0LTMwICBNaWNoYWVsIENhdGFuemFybyAgPG1jYXRhbnphcm9AaWdhbGlhLmNvbT4KIAog ICAgICAgICBbR1RLXSBXZWJraXQgc2hvdWxkIHNwb29mIGFzIFNhZmFyaSBvbiBhIE1hYyB3aGVu IG9uIENoYXNlLmNvbQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvZG9tL01lc3NhZ2VQb3J0 LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9NZXNzYWdlUG9ydC5jcHAKaW5kZXggY2YyODk3NjY0 Y2MwNTFlNmUzZjFlMDIzMzRhYzg2MTNmZTU3Mzg5Ni4uNmY3ZmQzNmJlM2RlMjFmYTY2Mjg0MTAy YWY2YzU2MDdlNGIwNzBkMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvZG9tL01lc3NhZ2VQ b3J0LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9kb20vTWVzc2FnZVBvcnQuY3BwCkBAIC0yODQs NyArMjg0LDcgQEAgdm9pZCBNZXNzYWdlUG9ydDo6ZGlzcGF0Y2hNZXNzYWdlcygpCiAgICAgICAg IH0KICAgICAgICAgd29ya2VyVGhyZWFkLT5ydW5Mb29wKCkucG9zdFRhc2tGb3JNb2RlKFtpbm5l ckhhbmRsZXIgPSBXVEZNb3ZlKGlubmVySGFuZGxlciksIG1lc3NhZ2VzID0gV1RGTW92ZShtZXNz YWdlcyksIGNvbXBsZXRpb25DYWxsYmFjayA9IFdURk1vdmUoY29tcGxldGlvbkNhbGxiYWNrKV0o YXV0byYpIG11dGFibGUgewogICAgICAgICAgICAgaW5uZXJIYW5kbGVyKFdURk1vdmUobWVzc2Fn ZXMpKTsKLSAgICAgICAgICAgIFJ1bkxvb3A6Om1haW4oKS5kaXNwYXRjaChbY29tcGxldGlvbkNh bGxiYWNrID0gV1RGTW92ZShjb21wbGV0aW9uQ2FsbGJhY2spXSB7CisgICAgICAgICAgICBjYWxs T25NYWluVGhyZWFkKFtjb21wbGV0aW9uQ2FsbGJhY2sgPSBXVEZNb3ZlKGNvbXBsZXRpb25DYWxs YmFjayldIHsKICAgICAgICAgICAgICAgICBjb21wbGV0aW9uQ2FsbGJhY2soKTsKICAgICAgICAg ICAgIH0pOwogICAgICAgICB9LCBXb3JrZXJSdW5Mb29wOjpkZWZhdWx0TW9kZSgpKTsK