Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2626883003:
Make WebTaskRunner::postTask thread safe (Closed)
|
Created:
3 years, 11 months ago by Charlie Harrison Modified:
3 years, 11 months ago CC:
blink-reviews, chromium-reviews, kinuko+watch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake 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}
Committed: https://chromium.googlesource.com/chromium/src/+/9b559c6994582ca25952810057dc778e82685ede
Patch Set 1 #Patch Set 2 : all cross thread closures #
Total comments: 2
Patch Set 3 : dcheng review #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 33 (23 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make WebTaskRunner::postTask thread safe BUG=679915 ========== to ========== Make WebTaskRunner::postTask thread safe BUG=679915, 680042 ==========
Description was changed from
==========
Make WebTaskRunner::postTask thread safe
BUG=679915, 680042
==========
to
==========
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
==========
csharrison@chromium.org changed reviewers: + dcheng@chromium.org
dcheng, ptal? I had trouble reproing this with tsan with a custom SimTest or the clusterfuzz repo. Should we just ship this and see if it fixes the existing clusterfuzz bug? I only did used the custom Bind for CrossThreadClosures.
LGTM, but I'm not an OWNER in platform. I assume that the background html parser tests no longer fail with the thread verifier enabled with this patch? https://codereview.chromium.org/2626883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2626883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebTaskRunner.cpp:122: base::Bind(&runCrossThreadClosure, base::Passed(std::move(task)))); Nit: base::Passed(&task) here and below.
https://codereview.chromium.org/2624443003 Is looking good with this patch applied to it. PS4 has a completed webkit_tests on Linux without the parser failures. There are 3 test timeouts but I think they are unrelated to this change. https://codereview.chromium.org/2626883003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2626883003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebTaskRunner.cpp:122: base::Bind(&runCrossThreadClosure, base::Passed(std::move(task)))); On 2017/01/11 18:19:16, dcheng wrote: > Nit: base::Passed(&task) here and below. Done.
csharrison@chromium.org changed reviewers: + esprehn@chromium.org
+esprehn would you review this as platform owner?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wez@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/2626883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2626883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebTaskRunner.cpp:117: // crbug.com/679915 for more details. Wouldn't it be better to "fix" convertToBaseCallback() to do this wrapping of the |task| into a Callback? This issue is caused by moving from a Task with move semantics to a Callback with copy semantics, so it will also affect other sites where convertToBaseCallback() is used, e.g. the WTF::Closure postTask() overload below, right?
On 2017/01/11 21:21:49, Wez wrote: > https://codereview.chromium.org/2626883003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): > > https://codereview.chromium.org/2626883003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/WebTaskRunner.cpp:117: // crbug.com/679915 > for more details. > Wouldn't it be better to "fix" convertToBaseCallback() to do this wrapping of > the |task| into a Callback? > > This issue is caused by moving from a Task with move semantics to a Callback > with copy semantics, so it will also affect other sites where > convertToBaseCallback() is used, e.g. the WTF::Closure postTask() overload > below, right? We should absolutely do that... in a followup. =) It would be nice to avoid more indirections than we really need -- we do need it here, but for things like Mojo, it's actually "OK". The right fix is to just plumb OnceCallback through so we don't accidentally blow our foot off.
We should probably rename convertToBaseCallback() to convertToSameThreadCallback(), but we can do that in a follow up once we fix this.
lgtm
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2626883003/#ps40001 (title: "dcheng review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484170421920630,
"parent_rev": "c2d1f65f2eaf620936ed56c42fe3e6235674600b", "commit_rev":
"9b559c6994582ca25952810057dc778e82685ede"}
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b559c6994582ca25952810057dc...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9b559c6994582ca25952810057dc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
