Bug 183619 – MessagePort is not always destroyed on the right thread

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

<rdar://problem/38204711>

[youenn fablet]

Created attachment 335748 [details]
Patch

[youenn fablet]

Created attachment 335812 [details]
Patch

[youenn fablet]

I wonder whether we should not add this same thread for construction and destruction to all ActiveDOMObjects.

[youenn fablet]

Created attachment 335816 [details]
Version without postTaskTo

[youenn fablet]

Created attachment 335829 [details]
Fixing log

[Chris Dumez]

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.

[youenn fablet]

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

[youenn fablet]

Created attachment 335856 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 335856 [details]
Patch for landing

Clearing flags on attachment: 335856

Committed r229628: <https://trac.webkit.org/changeset/229628>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.