+csharrison again, who somehow didn't get added?

I did some archaeology and found https://chromium.googlesource.com/chromium/src/+/f77d8b2c285461230adcb15f80ab6803caf52a55/components/scheduler/child/webthread_base.cc#82 and https://chromium.googlesource.com/chromium/src/+/f77d8b2c285461230adcb15f80ab6803caf52a55/components/scheduler/child/web_task_runner_impl.cc#20.

WebThreadBase::postIdleTask() is safe because it binds |idle_task| with |base::Passed|: ownership of |idle_task| is passed to the trampoline function that actually runs the Blink task. Thus, even if thread 1 holds the last reference to the base::BindState, there are no problems because the bound state has already been consumed, so destroying it is a no-op.

Contrast this with WebTaskRunnerImpl::postTask, which is *unsafe*. It binds |task| with |base::Owned|, which means that ownership of |task| is *not* passed to the trampoline helper. Instead, if thread 1 holds the last reference to the base::BindState, |task| will end up destroyed on the source thread.

As discussed offline, the issue here is primarily the passing of WTFString across thread boundaries, which relies upon the sender creating an "isolated" WTFString and *passing* its one-and-only reference into the Task, to be passed to the recipient.

That isolated WTFString should really be base::Passed() into the Callback inside the Task, so that even if the Callback's BindState is released on the sending thread, the unique reference to the WTFString's impl will have been safely passed to the recipient when the Callback ran, and the BindState's reference cleared.

... Though, as per dcheng@'s original argument, it's a fundamental change in the semantics of WebTaskRunner::postTask() which exposes the issue, so perhaps that's the best avenue for a fix. :)

I can take a stab at this one.

Looks like TSAN is complaining in  issue 680042  about this code. It seems almost too good to be true to get a clusterfuzz report about this just a day after we noticed it?

I'll see if I can repro locally at TOT and if my fix [1] fixes the race.

[1] https://codereview.chromium.org/2626883003/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9b559c6994582ca25952810057dc778e82685ede

commit 9b559c6994582ca25952810057dc778e82685ede
Author: csharrison <csharrison@chromium.org>
Date: Wed Jan 11 22:46:45 2017

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}

[modify] https://crrev.com/9b559c6994582ca25952810057dc778e82685ede/third_party/WebKit/Source/platform/WebTaskRunner.cpp

Let's mark as fixed. Followups to cleanup the API surface can use a different bug.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot