Bug 185134 – Fix bad use of RunLoop::main().dispatch() in MessagePort::dispatchMessages()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 339116 [details]
Patch

[Geoffrey Garen]

Comment on attachment 339116 [details]
Patch

r=me

[Daniel Bates]

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.

[Chris Dumez]

(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.

[WebKit Commit Bot]

Comment on attachment 339116 [details]
Patch

Clearing flags on attachment: 339116

Committed r231161: <https://trac.webkit.org/changeset/231161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39843751>

[Daniel Bates]

(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()?

[Chris Dumez]

(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.