Bug 185134 - Fix bad use of RunLoop::main().dispatch() in MessagePort::dispatchMessages()
Summary: Fix bad use of RunLoop::main().dispatch() in MessagePort::dispatchMessages()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-30 09:44 PDT by Chris Dumez
Modified: 2018-04-30 16:28 PDT (History)
11 users (show)

See Also:


Attachments
Patch (1.78 KB, patch)
2018-04-30 09:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-04-30 09:44:44 PDT
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.
Comment 1 Chris Dumez 2018-04-30 09:45:42 PDT
Created attachment 339116 [details]
Patch
Comment 2 Geoffrey Garen 2018-04-30 09:51:44 PDT
Comment on attachment 339116 [details]
Patch

r=me
Comment 3 Daniel Bates 2018-04-30 10:59:25 PDT
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.
Comment 4 Chris Dumez 2018-04-30 11:50:35 PDT
(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 5 WebKit Commit Bot 2018-04-30 12:13:38 PDT
Comment on attachment 339116 [details]
Patch

Clearing flags on attachment: 339116

Committed r231161: <https://trac.webkit.org/changeset/231161>
Comment 6 WebKit Commit Bot 2018-04-30 12:13:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2018-04-30 12:14:31 PDT
<rdar://problem/39843751>
Comment 8 Daniel Bates 2018-04-30 14:16:25 PDT
(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()?
Comment 9 Chris Dumez 2018-04-30 16:28:10 PDT
(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.