Bug 118879 – SerializedScriptValue should be ThreadSafeRefCounted when ENABLE(WORKERS) guard is on

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Kwang Yul Seo]

Because SerializedScriptValue is passed across threads using WorkerMessagingProxy, it should be ThreadSafeRefCounted when ENABLE(WORKERS) guard is on.

[Kwang Yul Seo]

Created attachment 207048 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 207048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207048&action=review

I don't see why this is necessary. But please feel free to mark for review again with additional explanation.

> Source/WebCore/ChangeLog:10
> +        SerializedScriptValue should be ThreadSafeRefCounted when
> +        ENABLE(WORKERS) guard is on because WorkerMessagingProxy passes
> +        SerializedScriptValue across threads.

I don't think that this is a sufficient explanation. An object only needs to be ThreadSafeRefCounted if multiple threads reference/dereference it. But if ownership is cleanly passed from one thread to another, that should not be necessary.

[Kwang Yul Seo]

(In reply to comment #2)
> I don't think that this is a sufficient explanation. An object only needs to be ThreadSafeRefCounted if multiple threads reference/dereference it. But if ownership is cleanly passed from one thread to another, that should not be necessary.

I spent more time digging where these SerializedScriptValue instances are created and where they are actually ref/deref-ed.

And you are right. Currently, all SerializedScriptValue instances that are passed to WorkerMessagingProxy are created in handlePostMessage and the ownership is cleanly passed to a worker thread.

BTW, it seems quite tricky because anyone can break this if he is not careful enough to find the underlying assumption.

I think we need a more systematic way to express the idiom of "create in one thread and pass the ownership to another thread".

Closing this bug as invalid.

[Alexey Proskuryakov]

I think that generally and ideally, out approach should be that no objects are used from multiple threads (and thus nearly nothing is ThreadSafeRefCounted).