Bug 183053 – MessagePort is not always destroyed in the right thread

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Ran into the following crash when running WPT service worker tests:
ASSERTION FAILED: (is<Document>(*this) && isMainThread()) || (is<WorkerGlobalScope>(*this) && downcast<WorkerGlobalScope>(*this).thread().thread() == &Thread::current())
./dom/ScriptExecutionContext.cpp(174) : void WebCore::ScriptExecutionContext::destroyedMessagePort(WebCore::MessagePort &)
1   0x395bf54ad WTFCrash
2   0x3870f9304 WebCore::ScriptExecutionContext::destroyedMessagePort(WebCore::MessagePort&)
3   0x3870770ad WebCore::MessagePort::~MessagePort()
4   0x387077145 WebCore::MessagePort::~MessagePort()
5   0x387077189 WebCore::MessagePort::~MessagePort()
6   0x3870769f2 WebCore::MessagePort::deref() const
7   0x380a0dd6e void WTF::derefIfNotNull<WebCore::MessagePort>(WebCore::MessagePort*)
8   0x380a0dd39 WTF::RefPtr<WebCore::MessagePort, WTF::DumbPtrTraits<WebCore::MessagePort> >::~RefPtr()
9   0x380a06395 WTF::RefPtr<WebCore::MessagePort, WTF::DumbPtrTraits<WebCore::MessagePort> >::~RefPtr()
10  0x380a05d3f WebKit::WebMessagePortChannelProvider::checkProcessLocalPortForActivity(WebCore::MessagePortIdentifier const&, unsigned long long)
11  0x380c9cea9 WebKit::WebProcess::checkProcessLocalPortForActivity(WebCore::MessagePortIdentifier const&, unsigned long long)
12  0x380cf6890 void IPC::callMemberFunctionImpl<WebKit::WebProcess, void (WebKit::WebProcess::*)(WebCore::MessagePortIdentifier const&, unsigned long long), std::__1::tuple<WebCore::MessagePortIdentifier, unsigned long long>, 0ul, 1ul>(WebKit::WebProcess*, void (WebKit::WebProcess::*)(WebCore::MessagePortIdentifier const&, unsigned long long), std::__1::tuple<WebCore::MessagePortIdentifier, unsigned long long>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>)
13  0x380cf65b0 void IPC::callMemberFunction<WebKit::WebProcess, void (WebKit::WebProcess::*)(WebCore::MessagePortIdentifier const&, unsigned long long), std::__1::tuple<WebCore::MessagePortIdentifier, unsigned long long>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WebCore::MessagePortIdentifier, unsigned long long>&&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(WebCore::MessagePortIdentifier const&, unsigned long long))
14  0x380ceb9b5 void IPC::handleMessage<Messages::WebProcess::CheckProcessLocalPortForActivity, WebKit::WebProcess, void (WebKit::WebProcess::*)(WebCore::MessagePortIdentifier const&, unsigned long long)>(IPC::Decoder&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(WebCore::MessagePortIdentifier const&, unsigned long long))
15  0x380ce6c36 WebKit::WebProcess::didReceiveWebProcessMessage(IPC::Connection&, IPC::Decoder&)
16  0x380c9757b WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
17  0x3801468b3 IPC::Connection::dispatchMessage(IPC::Decoder&)
18  0x38013be98 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
19  0x380146eba IPC::Connection::dispatchOneMessage()
20  0x38015f37d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()()
21  0x38015f2d9 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call()
22  0x395c115cb WTF::Function<void ()>::operator()() const
23  0x395c56473 WTF::RunLoop::performWork()
24  0x395c56d14 WTF::RunLoop::performWork(void*)
25  0x7fff36d102d1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
26  0x7fff36dc9c9c __CFRunLoopDoSource0
27  0x7fff36cf2e60 __CFRunLoopDoSources0
28  0x7fff36cf22dd __CFRunLoopRun
29  0x7fff36cf1b43 CFRunLoopRunSpecific
30  0x7fff35fe2f16 RunCurrentEventLoopInMode
31  0x7fff35fe2c86 ReceiveNextEventCommon

[youenn fablet]

Looking at MessagePort::existingMessagePortForIdentifier implementation, it returns a RefPtr<MessagePort>.
Since it can be called from any thread, we could for instance retain the lock until the MessagePort is used.

[youenn fablet]

Created attachment 334467 [details]
Patch

[Chris Dumez]

Comment on attachment 334467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334467&action=review

> Source/WebCore/ChangeLog:3
> +        MessagePort is not always destroyed in the right thread

Shouldn't we fix this? The patch does not seem address this particular issue.

It seems bad that a MessagePort would be used on one thread but could be destroyed on another thread while being used. Brady is more familiar with this code however.

> Source/WebCore/dom/MessagePort.cpp:80
> +    callback(allMessagePorts().get(identifier));

This could in theory cause deadlocks if the callback calls existingMessagePortForIdentifier().

[youenn fablet]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 334467 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334467&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        MessagePort is not always destroyed in the right thread
> 
> Shouldn't we fix this? The patch does not seem address this particular issue.

As I read the code, as long as we hold the lock, no MessagePort can be destroyed.
Since existingMessagePortForIdentifier() is not refing anymore a MessagePort, it cannot cause it to be destroyed in the wrong thread.

> It seems bad that a MessagePort would be used on one thread but could be
> destroyed on another thread while being used. Brady is more familiar with
> this code however.

A MessagePort has no thread-safe members.
But we would be touching ScriptExecutionContext message port hashset from various threads which probably has implications.

> > Source/WebCore/dom/MessagePort.cpp:80
> > +    callback(allMessagePorts().get(identifier));
> 
> This could in theory cause deadlocks if the callback calls
> existingMessagePortForIdentifier().

Yes, the other approach would be to somehow take the lock from the caller site.
Not a problem right now but might be in the future.
Maybe there is a better way to do this?

[Radar WebKit Bug Importer]

<rdar://problem/37805437>

[youenn fablet]

I will update the patch to add the various getters/setter methods with a lock and remove the need for the existingXX method.
Will do that tonight

[youenn fablet]

Created attachment 334586 [details]
Patch

[youenn fablet]

I added two methods isExistingMessagePortLocallyReachable and notifyMessageAvailable.
I wonder whether we could simplify the MessagePort model a bit.

Maybe something like having a MainThread map that allows getting from a message port identifier the context identifier and whether it is locally reachable.
That may allow removing the need of the lock and making MessagePort a regular RefCounted object.

[Chris Dumez]

Comment on attachment 334586 [details]
Patch

r=me, nice.

[WebKit Commit Bot]

Comment on attachment 334586 [details]
Patch

Clearing flags on attachment: 334586

Committed r229028: <https://trac.webkit.org/changeset/229028>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.