Summary: | MessagePort is not always destroyed in the right thread | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||
Component: | Service Workers | Assignee: | youenn fablet <youennf> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | beidson, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, kangil.han, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
youenn fablet
2018-02-22 12:20:37 PST
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. Created attachment 334467 [details]
Patch
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(). (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? 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 Created attachment 334586 [details]
Patch
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. Comment on attachment 334586 [details]
Patch
r=me, nice.
Comment on attachment 334586 [details] Patch Clearing flags on attachment: 334586 Committed r229028: <https://trac.webkit.org/changeset/229028> All reviewed patches have been landed. Closing bug. |