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.
Created attachment 339116 [details] Patch
Comment on attachment 339116 [details] Patch r=me
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.
(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.
Comment on attachment 339116 [details] Patch Clearing flags on attachment: 339116 Committed r231161: <https://trac.webkit.org/changeset/231161>
All reviewed patches have been landed. Closing bug.
<rdar://problem/39843751>
(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()?
(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.