|Summary:||Remove threadsafe refcounting from tasks used with WTF::MessageQueue.|
|Product:||WebKit||Reporter:||Dmitry Titov <dimich>|
|Severity:||Normal||CC:||ap, dimich, levin|
|Version:||528+ (Nightly build)|
|Bug Depends on:||30805, 30941|
Description Dmitry Titov 2009-10-20 18:01:51 PDT
The MessageQueue keeps RefPtr<WorkerRunLoop::Task> objects. Unfortunately, WorkerRunLoop::Task is RefCounted, not ThreadSafeShared. Because of this, when the task is added to the queue, another thread can pick it up right away while the RefPtr on the callstack is still to be deconstructed. This can cause race condition.
Comment 1 Dmitry Titov 2009-10-23 10:58:16 PDT
So the easy fix would be to make WorkerRunLoop::Task ThreadSafeShared. However, the pattern of usage of MessageQueue in all places (workers, localstore, database) is that we create a task, append it to the MQ, which signals to other thread. The signaled thread takes the task from the queue, runs it and discards it. This is straight scenario for OwnPtr/PassOwnPtr because at no moment in time there are multiple owners of the tasks, the ownership is passed very cleanly from one thread to another. So we can stop deriving all kind of task classes from ThreadSafeShared and change MessageQueue to take ownership of a task on append and pass it to the fetching thread later. This will remove quite a lot of unnecessary thread-safe refcounting. Patch is forthcoming.
Comment 2 Dmitry Titov 2009-10-23 22:26:36 PDT
Created attachment 41777 [details] Work in progress - not for actual review yet. This seems to be the change. Posting here not for review yet, but to get feedback on overall direction. It's on a biggish side, so I think I'll split it in 3 parts after cleanup: - split DatabaseTask into a task and a synchronizer that allows to wait for synchronous completion of the task without having an actual task pointer in the originating thread. - adding MessageQueue::removeWithPredicate(...) - making tasks not refcounted.
Comment 3 Dmitry Titov 2009-10-30 16:53:36 PDT
Created attachment 42245 [details] Proposed patch. Now that the preparatory patches are in, this change is simple: - Change MessageQueue to act as a queue of OwnPtr<DataType> - Remove ThreadSafeShared from various Task types. - Replace a bunch of RefPtrs with OwnPtrs when dealing with those Tasks
Comment 4 David Levin 2009-11-02 08:31:43 PST