Bug 88823 – WorkerMessagingProxy lifetime is too complex

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore JavaScript

[Mark Hahnenberg]

Currently WorkerMessagingProxy is responsible for its own lifetime throughout a series of callbacks and asynchronous tasks posted across threads. This complexity has made it difficult to make modifications to how Worker instances can be torn down. To simplify our lives a bit, we should ref count these WorkerMessagingProxy objects so that we don't accidentally try to use them after they've already been destroyed.

[Alexey Proskuryakov]

The task passing design was meant to carefully avoid locking, in particular making it possible to implement workers as separate processes.

[David Levin]

It is unclear what the proposal is here. If it is to make WorkerMessagingProxy ThreadSafeRefCounted, then it won't work (as is) because there are a number of objects that aren't safe to delete on either thread (like the ScriptExecutionObject).

Perhaps it could be done by dividing the class in two though.

The current interfaces that it derives from my give an indiciation about how that division could take place: WorkerContextProxy, public WorkerObjectProxy, public WorkerLoaderProxy

It will still likely need to shut down using a complicated dance of message passing (but this could allow for per thread RefCounting if desired).

[Mark Hahnenberg]

(In reply to comment #2)
> It is unclear what the proposal is here. If it is to make WorkerMessagingProxy ThreadSafeRefCounted, then it won't work (as is) because there are a number of objects that aren't safe to delete on either thread (like the ScriptExecutionObject).

> Perhaps it could be done by dividing the class in two though.
> 
> The current interfaces that it derives from my give an indiciation about how that division could take place: WorkerContextProxy, public WorkerObjectProxy, public WorkerLoaderProxy
> 
> It will still likely need to shut down using a complicated dance of message passing (but this could allow for per thread RefCounting if desired).

My primary goal is to make destroying Workers during garbage collection safe. Currently, when tearing down a Worker we can start executing more JS code, which is unsafe if it occurs during a garbage collection. 

The easiest way that I could see to fix this issue would be to asynchronously call terminateWorkerContext() or workerContextDestroyedInternal() in WorkerMessagingProxy::workerObjectDestroyed() using workerContextClosed() and workerContextDestroyed() respectively. In order to do this without committing suicide too early and ending up with a poisonous task in the task queue, WorkerMessagingProxy would be ThreadSafeRefCounted and each task, along with the Worker, would have a RefPtr to the proxy. Thus, we'd still do our complicated message passing dance, but WorkerMessagingProxy wouldn't have to know exactly the right moment to commit suicide. Obviously if it's not safe to destroy some objects on either thread then this won't work.

How would splitting the class up help with this situation?

[Geoffrey Garen]

> The task passing design was meant to carefully avoid locking, in particular making it possible to implement workers as separate processes.

Reference counting does not require locking.

> If it is to make WorkerMessagingProxy ThreadSafeRefCounted

Yes. In particular, pending tasks must ref a proxy alive. (Currently, a proxy may delete itself even if it has pending tasks.)

> then it won't work

What symptoms of not working will we observe? Perhaps Mark can test his patch against those potential symptoms.

> Perhaps it could be done by dividing the class in two though.

I don't think it's safe to delete any portion of the object while it has a pending task, due to the simple fact that the task could pertain to any portion of the object.

Mark, if you have a patch in progress, can you post it to clarify the discussion here?

[David Levin]

(In reply to comment #4)
> > The task passing design was meant to carefully avoid locking, in particular making it possible to 
> 
> > then it won't work
> 
> What symptoms of not working will we observe? Perhaps Mark can test his patch against those potential symptoms.

If you treat non-threadsafe classes as if they were threadsafe, you will get random memory corruptions. You can't really test this. You have to carefully examine the code.

I specifically mentioned one case above: ScriptExecutionContext. It simply isn't safe to do deref a ScriptExecutionContext on any thread because it isn't ThreadSafeRefCounted.

[Alexey Proskuryakov]

> Reference counting does not require locking.

I guess one can disagree here, but atomic operations are not all that much different from locks internally, and don't work across processes either.

[David Levin]

I decided to look one last time for tonight to try to understand the problem here.

(In reply to comment #3) 
> My primary goal is to make destroying Workers during garbage collection safe. Currently, when tearing down a Worker we can start executing more JS code, which is unsafe if it occurs during a garbage collection. 

It sounds like this is the problem: that we can execute js code while tearing down a worker (during garbage collection).  Perhaps that can be avoided.

> How would splitting the class up help with this situation?
My primary concern with threadsafe refcounting is that implies you don't know what thread will finally delete the object and that is a problem. 

If you could separate the object into two pieces and have each piece be ref counted only on a single thread, then there isn't a problem with objects being destroyed (or deref'ed) on the wrong thread.

[Geoffrey Garen]

> My primary concern with threadsafe refcounting is that implies you don't know what thread will finally delete the object and that is a problem. 

What mechanism currently guarantees which thread will delete a WorkerMessagingProxy?

[David Levin]

(In reply to comment #8)
> > My primary concern with threadsafe refcounting is that implies you don't know what thread will finally delete the object and that is a problem. 
> 
> What mechanism currently guarantees which thread will delete a WorkerMessagingProxy?

There is only one place where "delete this" is called and that method is always executed on the Worker Object's (The Document's) thread.

[Mark Hahnenberg]

> There is only one place where "delete this" is called and that method is always executed on the Worker Object's (The Document's) thread.

And that is the same thread that Tasks posted to the ScriptExecutionContext are executed?

[David Levin]

(In reply to comment #10)
> > There is only one place where "delete this" is called and that method is always executed on the Worker Object's (The Document's) thread.
> 
> And that is the same thread that Tasks posted to the ScriptExecutionContext are executed?

Yes (the ScriptExecutionContext in the WorkerMessagingProxy is always the document right now since nested workers aren't supported at the present time).

[Mark Hahnenberg]

I guess we'll leave this bug for another day. It's a much more difficult task to make this code simpler than I initially thought.