Bug 183619

Summary: MessagePort is not always destroyed on the right thread
Product: WebKit Reporter: youenn fablet <youennf>
Component: Service WorkersAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, cdumez, commit-queue, dbates, ddkilzer, esprehn+autocc, ews-watchlist, kangil.han, rniwa
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 183602    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Version without postTaskTo
none
Fixing log
none
Patch for landing none

Description youenn fablet 2018-03-13 17:04:14 PDT
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.
Comment 1 youenn fablet 2018-03-13 17:04:32 PDT
<rdar://problem/38204711>
Comment 2 youenn fablet 2018-03-13 17:06:18 PDT
Created attachment 335748 [details]
Patch
Comment 3 youenn fablet 2018-03-14 17:10:08 PDT
Created attachment 335812 [details]
Patch
Comment 4 youenn fablet 2018-03-14 17:10:51 PDT
I wonder whether we should not add this same thread for construction and destruction to all ActiveDOMObjects.
Comment 5 youenn fablet 2018-03-14 17:46:16 PDT
Created attachment 335816 [details]
Version without postTaskTo
Comment 6 youenn fablet 2018-03-14 21:19:45 PDT
Created attachment 335829 [details]
Fixing log
Comment 7 Chris Dumez 2018-03-15 09:33:22 PDT
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.
Comment 8 youenn fablet 2018-03-15 09:59:22 PDT
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
Comment 9 youenn fablet 2018-03-15 10:03:27 PDT
Created attachment 335856 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2018-03-15 11:21:16 PDT
Comment on attachment 335856 [details]
Patch for landing

Clearing flags on attachment: 335856

Committed r229628: <https://trac.webkit.org/changeset/229628>
Comment 11 WebKit Commit Bot 2018-03-15 11:21:18 PDT
All reviewed patches have been landed.  Closing bug.