Bug 183619 - MessagePort is not always destroyed on the right thread
Summary: MessagePort is not always destroyed on the right thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 183602
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-13 17:04 PDT by youenn fablet
Modified: 2018-03-15 11:21 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.31 KB, patch)
2018-03-13 17:06 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (10.46 KB, patch)
2018-03-14 17:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Version without postTaskTo (12.01 KB, patch)
2018-03-14 17:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing log (12.16 KB, patch)
2018-03-14 21:19 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (12.14 KB, patch)
2018-03-15 10:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.