Since we ref MessagePort from a worker thread, and pass this ref to the main thread, a MessagePort might sometimes be destroyed on the main thread.
<rdar://problem/38204711>
Created attachment 335748 [details] Patch
Created attachment 335812 [details] Patch
I wonder whether we should not add this same thread for construction and destruction to all ActiveDOMObjects.
Created attachment 335816 [details] Version without postTaskTo
Created attachment 335829 [details] Fixing log
Comment on attachment 335829 [details] Fixing log View in context: https://bugs.webkit.org/attachment.cgi?id=335829&action=review > Source/WebCore/dom/MessagePort.cpp:279 > innerHandler(WTFMove(messages)); Could you please ASSERT(isMainThread()); in here? > Source/WebCore/dom/MessagePort.cpp:333 > + if (weakThis) Could you please ASSERT(isMainThread()); in here? > Source/WebCore/dom/MessagePort.h:113 > + WeakPtr<MessagePort> createWeakPtr() { return m_weakFactory.createWeakPtr(*this); } My understanding is that the modern way is to not have this method and use makeWeakPtr() to constructor your WeakPtr. You already have the weakPtrFactory() getter above so it should work fine.
Thanks for the review. (In reply to Chris Dumez from comment #7) > Comment on attachment 335829 [details] > Fixing log > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335829&action=review > > > Source/WebCore/dom/MessagePort.cpp:279 > > innerHandler(WTFMove(messages)); > > Could you please ASSERT(isMainThread()); in here? It might be best to add it upfront in the callback. Since we are refing workerThread, we really want the callback to be destroyed in the main thread whether workerThread is null or not. > > Source/WebCore/dom/MessagePort.cpp:333 > > + if (weakThis) > > Could you please ASSERT(isMainThread()); in here? Ditto. > > Source/WebCore/dom/MessagePort.h:113 > > + WeakPtr<MessagePort> createWeakPtr() { return m_weakFactory.createWeakPtr(*this); } > > My understanding is that the modern way is to not have this method and use > makeWeakPtr() to constructor your WeakPtr. You already have the > weakPtrFactory() getter above so it should work fine. OK
Created attachment 335856 [details] Patch for landing
Comment on attachment 335856 [details] Patch for landing Clearing flags on attachment: 335856 Committed r229628: <https://trac.webkit.org/changeset/229628>
All reviewed patches have been landed. Closing bug.