Commit 9b559c69 authored by csharrison's avatar csharrison Committed by Commit bot

Make WebTaskRunner::postTask thread safe

Copying the summary from dcheng@ on the referenced bug:

The current implementation looks like this:
void WebTaskRunner::postTask(const WebTraceLocation& location,
                             std::unique_ptr<CrossThreadClosure> task) {
  toSingleThreadTaskRunner()->PostTask(location,

     convertToBaseCallback(std::move(task)));
}

However, base::TaskRunner::PostTask() takes a const base::Closure&.
When we convert the CrossThreadClosure to a base::Callback, we just
extract the internal callback object. However, this callback object is
never supposed to be copied, only moved.

Since the base APIs haven't been updated to use base::OnceCallback, now
 we have two copies of the callback referencing the same
base::BindState (which is holding the bound variables): one on the
posting thread (call it thread 1), and one on the posted thread (call
it thread 2).

If the copy on thread 2 is destroyed before the copy on thread 1,
base::BindState will be destroyed on thread 1. If base::BindState
contained WTF::String or other thread-unsafe objects, and thread 2
copied it, this means that the thread-unsafe objects will be ref'ed
and deref'ed on multiple threads, which is not good.

BUG=679915, 680042

Review-Url: https://codereview.chromium.org/2626883003
Cr-Commit-Position: refs/heads/master@{#443028}
parent c2d1f65f
......@@ -29,6 +29,14 @@ struct CallbackCancellationTraits<
namespace blink {
namespace {
void runCrossThreadClosure(std::unique_ptr<CrossThreadClosure> task) {
(*task)();
}
} // namespace
class TaskHandle::Runner : public WTF::ThreadSafeRefCounted<Runner> {
public:
explicit Runner(std::unique_ptr<WTF::Closure> task)
......@@ -103,17 +111,21 @@ TaskHandle::TaskHandle(RefPtr<Runner> runner) : m_runner(std::move(runner)) {
DCHECK(m_runner);
}
// Use a custom function for base::Bind instead of convertToBaseCallback to
// avoid copying the closure later in the call chain. Copying the bound state
// can lead to data races with ref counted objects like StringImpl. See
// crbug.com/679915 for more details.
void WebTaskRunner::postTask(const WebTraceLocation& location,
std::unique_ptr<CrossThreadClosure> task) {
toSingleThreadTaskRunner()->PostTask(location,
convertToBaseCallback(std::move(task)));
toSingleThreadTaskRunner()->PostTask(
location, base::Bind(&runCrossThreadClosure, base::Passed(&task)));
}
void WebTaskRunner::postDelayedTask(const WebTraceLocation& location,
std::unique_ptr<CrossThreadClosure> task,
long long delayMs) {
toSingleThreadTaskRunner()->PostDelayedTask(
location, convertToBaseCallback(std::move(task)),
location, base::Bind(&runCrossThreadClosure, base::Passed(&task)),
base::TimeDelta::FromMilliseconds(delayMs));
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment