Bug 30612 – Remove threadsafe refcounting from tasks used with WTF::MessageQueue.

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

[Dmitry Titov]

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.

[Dmitry Titov]

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.

[Dmitry Titov]

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.

[Dmitry Titov]

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

[David Levin]

Comment on attachment 42245 [details]
Proposed patch.

Just a few things to fix on landing.

> diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog
> +        * wtf/MessageQueue.h:
> +        (WTF::MessageQueue::alwaysTruePredicate):
> +        (WTF::::~MessageQueue):
> +        (WTF::::append):
> +        (WTF::::appendAndCheckEmpty):
> +        (WTF::::prepend):
> +        (WTF::::waitForMessage):
> +        (WTF::::waitForMessageFilteredWithTimeout):
> +        (WTF::::tryGetMessage):
> +        (WTF::::removeIf):
These functions declarations are messed up. Please fix them.


> diff --git a/JavaScriptCore/wtf/MessageQueue.h b/JavaScriptCore/wtf/MessageQueue.h
> +    inline void MessageQueue<DataType>::~MessageQueue()
> +    {
> +        MutexLocker lock(m_mutex);
> +
> +        while (!m_queue.isEmpty()) {
> +            DequeConstIterator<DataType*> found = m_queue.begin();
> +            DataType* ptr = *found;
> +            m_queue.remove(found);
> +            delete ptr;
> +        }
> +    }
Consider using "deleteAllValues" from Deque.h

> +
> +
Extra blank line.


> +    inline PassOwnPtr<DataType> MessageQueue<DataType>::waitForMessageFilteredWithTimeout(MessageQueueWaitResult& result, Predicate& predicate, double absoluteTime)
> +        DataType* ptr = *found;
ptr? (name) How about message or data.... ?


> +    inline PassOwnPtr<DataType> MessageQueue<DataType>::tryGetMessage()
> +        DataType* ptr = m_queue.first();
ptr? (name)


> @@ -157,9 +181,11 @@ namespace WTF {
>      inline void MessageQueue<DataType>::removeIf(Predicate& predicate)
> +            DataType* ptr = *found;

ptr? (name)

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
>  2009-10-30  Dmitry Titov  <[e-mail]>
>  
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Remove threadsafe refcounting from tasks used with WTF::MessageQueue.
> +        https://bugs.webkit.org/show_bug.cgi?id=30612
> +
> +        No new tests since no new functionality. Storage, MessagePorts and Workers tests cover this.
> +
> +        There is a lot of files but most changes are simply replace RefPtr and PassRefPtr to

/There is/There are/
/to/with/


> diff --git a/WebCore/dom/default/PlatformMessagePortChannel.cpp b/WebCore/dom/default/PlatformMessagePortChannel.cpp
> @@ -193,7 +193,7 @@ void PlatformMessagePortChannel::postMessageToRemote(PassOwnPtr<MessagePortChann
>  bool PlatformMessagePortChannel::tryGetMessageFromRemote(OwnPtr<MessagePortChannel::EventData>& result)
>  {
>      MutexLocker lock(m_mutex);
> +    return (result = m_incomingQueue->tryGetMessage());

It would be nice to not write this on one line, which can easily making it look like == was intended at first glance.
  result = m_incomingQueue->tryGetMessage();
  return result;


> diff --git a/WebCore/storage/LocalStorageThread.cpp b/WebCore/storage/LocalStorageThread.cpp
>      while (true) {
> -        RefPtr<LocalStorageTask> task;
> -        if (!m_queue.waitForMessage(task))
> +        OwnPtr<LocalStorageTask> task = m_queue.waitForMessage();
> +        if (!task)
>              break;

For similar code in WebCore/storage/DatabaseThread.cpp, you re-wrote this to be
  while (OwnPtr<LocalStorageTask> task = m_queue.waitForMessage()) {

(and if you do that I think the contents will be a single line so no {} around that.)


> diff --git a/WebCore/workers/WorkerRunLoop.cpp b/WebCore/workers/WorkerRunLoop.cpp
> +void WorkerRunLoop::Task::performTask(ScriptExecutionContext* context) { m_task->performTask(context); }

Should be multiple lines.

[Dmitry Titov]

Fixed all and landed: http://trac.webkit.org/changeset/50427