Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2657603004:
Clear PostTaskAndReply task on the destination thread (3) (Closed)
DescriptionClear PostTaskAndReply task on the destination thread
This is the behavior-changing part of base::TaskRunner migration from
base::Callback to base::OnceCallback.
In the previous code, PostTaskAndReply and its family guarantee that
|task| is destroyed on the origin thread. If that is not possible, PTAR
leaks |task|. However, it's hard to keep that with OnceCallback, and the
behavior makes less sense on Blink classes, since most of its types are
thread-unsafe and need special handling for thread hop.
Destroying the bound object on the original thread requires extra
thread hop on the bound object just.
On the new code, |task| is destroyed on the target thread unless the
caller holds a reference on the origin thread.
BUG=680359
Review-Url: https://codereview.chromium.org/2657603004
Cr-Commit-Position: refs/heads/master@{#460821}
Committed: https://chromium.googlesource.com/chromium/src/+/e5de8d551e80115b21e0e3ca7d5451d3cee6f3ba
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : +WithResult. ios, cancelable, quota. #Patch Set 4 : +disk_cache::File fix #Patch Set 5 : rebase #
Total comments: 16
Patch Set 6 : rebase #Patch Set 7 : rebase on split CLs #Patch Set 8 : rename a test case. +comment #Patch Set 9 : rebase #Patch Set 10 : rebase #Patch Set 11 : rebase on DCHECK in RefCount CL #Patch Set 12 : rebase #Patch Set 13 : rebase #Patch Set 14 : rebase onto master without dcheck_in_ref_counted #
Total comments: 2
Patch Set 15 : test #Patch Set 16 : -#include #
Total comments: 2
Patch Set 17 : mod test #
Total comments: 2
Patch Set 18 : reorder delete_flag check. +comment #
Messages
Total messages: 118 (97 generated)
The CQ bit was checked by tzik@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.
Description was changed from ========== Clear PostTaskAndReply task on the destination thread BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all other TaskRunner::PostTaskAndReply caller. No one depends on the destruction thread of the posted task. BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all other TaskRunner::PostTaskAndReply caller. No one depends on the destruction thread of the posted task. BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all other TaskRunner::PostTaskAndReply caller. No one depends on the destruction thread of the posted task. * Checking BrowserThread::PostTaskAndReply. BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all other TaskRunner::PostTaskAndReply caller. No one depends on the destruction thread of the posted task. * Checking BrowserThread::PostTaskAndReply. BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * Checking BrowserThread::PostTaskAndReply. BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * Checking BrowserThread::PostTaskAndReply. BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. * Checking BrowserThread::PostBlockingPoolTaskAndReply. BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. * Checking BrowserThread::PostBlockingPoolTaskAndReply. BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked all 28 BrowserThread::PostBlockingPoolTaskAndReply callers. BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked all 28 BrowserThread::PostBlockingPoolTaskAndReply callers. BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked all 28 BrowserThread::PostBlockingPoolTaskAndReply callers. BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked all 28 BrowserThread::PostBlockingPoolTaskAndReply callers. BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked all 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ==========
The CQ bit was checked by tzik@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 ========== Clear PostTaskAndReply task on the destination thread TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked all 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked all 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked all 165 TaskRunner::PostTaskAndReply callers. No one depends on the destruction thread of the posted task, except for NSSCertDatabaseChromeOS. * [Done] Checked all 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked all 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * Looking into PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * Looking into PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * Looking into PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * Looking into PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * Should we change the signature of PostTaskAndReply not to leave a reference in the original thread? * Should we change other variants, e.g. PostTaskAndReplyWithResult? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread 123456789012345678901234567890123456789012345678901234567890123456789012 This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. BUG= ==========
The CQ bit was checked by tzik@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 ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * disk_cache::File BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. TODO: Check if it's safe. * [Done] PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * ActivityIconLoader::OnIconsReady is suspicious. -> fixing separately. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * disk_cache::File BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. TODO: Check if it's safe. * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * ActivityIconLoader::OnIconsReady is suspicious. * Fixing separately. * disk_cache::File -> fixing BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. TODO: Check if it's safe. * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * ActivityIconLoader::OnIconsReady is suspicious. * Fixing separately. * disk_cache::File -> fixing BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. TODO: Check if it's safe. * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * ActivityIconLoader::OnIconsReady is suspicious. * Fixed separately. https://codereview.chromium.org/2655213004/ * disk_cache::File * Fixing. https://codereview.chromium.org/2657003003/ BUG= ==========
The CQ bit was checked by tzik@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 tzik@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 ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. TODO: Check if it's safe. * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * ActivityIconLoader::OnIconsReady is suspicious. * Fixed separately. https://codereview.chromium.org/2655213004/ * disk_cache::File * Fixing. https://codereview.chromium.org/2657003003/ BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. TODO: Check if it's safe. * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * ActivityIconLoader::OnIconsReady is suspicious. * Fixed separately. https://codereview.chromium.org/2655213004/ * disk_cache::File * Fixing. https://codereview.chromium.org/2657003003/ * needs PSA? BUG= ==========
Description was changed from ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. TODO: Check if it's safe. * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * ActivityIconLoader::OnIconsReady is suspicious. * Fixed separately. https://codereview.chromium.org/2655213004/ * disk_cache::File * Fixing. https://codereview.chromium.org/2657003003/ * needs PSA? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. On the new code, |task| is destroyed on the target thread unless the caller holds a reference on the origin thread. TODO: Check if it's safe. * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * ActivityIconLoader::OnIconsReady is suspicious. * Fixed separately. https://codereview.chromium.org/2655213004/ * disk_cache::File * Fixing. https://codereview.chromium.org/2657003003/ * needs PSA? BUG= ==========
tzik@chromium.org changed reviewers: + dcheng@chromium.org
Description was changed from ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. On the new code, |task| is destroyed on the target thread unless the caller holds a reference on the origin thread. TODO: Check if it's safe. * [Done] Checked 165 TaskRunner::PostTaskAndReply callers. * [Done] Checked 96 BrowserThread::PostTaskAndReply callers. * [Done] Checked 28 BrowserThread::PostBlockingPoolTaskAndReply callers. * [Done] Checked 29 CancelableTaskTracker::PostTaskAndReply callers. * [Done] Checked 2 CancelableTaskTracker::PostTaskAndReplyWithResult callers. * [Done] Checked 338 PostTaskAndReplyWithResult callers. * [Done] Checked 22 PostTaskWithTraitsAndReply callers. * [Done] Checked 5 WorkerPool::PostTaskAndReply callers. * [Done] Checked 12 QuotaManager::PostTaskAndReplyWithResultForDBThread callers. * PostTaskAndReply in NSSCertDatabaseChromeOS::ListCerts looks suspicious. Is PK11_FreeSymKey thread safe? * OK, it's thread-safe. https://dxr.mozilla.org/mozilla-beta/source/security/nss/lib/pk11wrap/pk11ske... * ActivityIconLoader::OnIconsReady is suspicious. * Fixed separately. https://codereview.chromium.org/2655213004/ * disk_cache::File * Fixing. https://codereview.chromium.org/2657003003/ * needs PSA? BUG= ========== to ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. On the new code, |task| is destroyed on the target thread unless the caller holds a reference on the origin thread. ==========
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2017/01/30 21:07:47, tzik wrote: > PTAL Nit: I think the CL description might be incomplete: > Destroying the bound object on the original thread requires extra > thread hop on the bound object just. Also, mind adding BUG=680359 to the CL description? In general, I'm a little nervous about this change. I'm not convinced that we don't have anything that secretly depends on this behavior (for example, see https://groups.google.com/a/chromium.org/forum/#!topicsearchin/chromium-dev/R... for a past discussion of RefCounted and thread safety). Just to throw ideas out there... is it possible to preserve the existing behavior for RepeatingCallback by just adding an overload for OnceCallback instead? Unrelated note: it also seems like OnceCallback isn't a perfect solution for this either--if a refcounted thread-unsafe object has more than one reference, or the reference is not moved, then we still get explosions. I wonder if we should be DCHECKing that thread-unsafe
On Mon, Jan 30, 2017 at 8:55 PM, <dcheng@chromium.org> wrote: > On 2017/01/30 21:07:47, tzik wrote: > > PTAL > > Nit: I think the CL description might be incomplete: > > Destroying the bound object on the original thread requires extra > > thread hop on the bound object just. > > Also, mind adding BUG=680359 to the CL description? > > In general, I'm a little nervous about this change. I'm not convinced that > we > don't have anything that secretly depends on this behavior (for example, > see > https://groups.google.com/a/chromium.org/forum/#! > topicsearchin/chromium-dev/RefCountedThreadSafe/chromium-dev/8LqVoXQ_2bo > for a past discussion of RefCounted and thread safety). > > Just to throw ideas out there... is it possible to preserve the existing > behavior for RepeatingCallback by just adding an overload for OnceCallback > instead? > > Unrelated note: it also seems like OnceCallback isn't a perfect solution > for > this either--if a refcounted thread-unsafe object has more than one > reference, > or the reference is not moved, then we still get explosions. I wonder if we > should be DCHECKing that thread-unsafe > This is the same problem WTF::String has. It would be great to react if refcounted-not-threadsafe is passed to another thread with > 1 ref. > > https://codereview.chromium.org/2657603004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
gab@chromium.org changed reviewers: + gab@chromium.org
On 2017/01/31 15:37:08, danakj (slow) wrote: > On Mon, Jan 30, 2017 at 8:55 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dcheng@chromium.org> wrote: > > > On 2017/01/30 21:07:47, tzik wrote: > > > PTAL > > > > Nit: I think the CL description might be incomplete: > > > Destroying the bound object on the original thread requires extra > > > thread hop on the bound object just. > > > > Also, mind adding BUG=680359 to the CL description? > > > > In general, I'm a little nervous about this change. I'm not convinced that > > we > > don't have anything that secretly depends on this behavior (for example, > > see > > https://groups.google.com/a/chromium.org/forum/#! > > topicsearchin/chromium-dev/RefCountedThreadSafe/chromium-dev/8LqVoXQ_2bo > > for a past discussion of RefCounted and thread safety). > > > > Just to throw ideas out there... is it possible to preserve the existing > > behavior for RepeatingCallback by just adding an overload for OnceCallback > > instead? > > > > Unrelated note: it also seems like OnceCallback isn't a perfect solution > > for > > this either--if a refcounted thread-unsafe object has more than one > > reference, > > or the reference is not moved, then we still get explosions. I wonder if we > > should be DCHECKing that thread-unsafe > > > > This is the same problem WTF::String has. It would be great to react if > refcounted-not-threadsafe is passed to another thread with > 1 ref. Definitely, we should DCHECK if RefCounted's refcount is tweaked in a non-thread safe manner (can use same logic as WeakPtr's WeakReference::Flag checks which do handle the one-ref case?). I'm in favor of this behavior if we can pull it off. For one it will help use cases where |task| is an iOS "critical closure" which holds shutdown while its alive (and as such currently makes the reply shutdown blocking), e.g.: https://cs.chromium.org/chromium/src/base/files/important_file_writer.cc?sq=p... https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... File base/message_loop/message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... base/message_loop/message_loop_task_runner_unittest.cc:203: TEST_F(MessageLoopTaskRunnerTest, PostTaskAndReply_DeadReplyLoopDoesNotDelete) { Rename https://codereview.chromium.org/2657603004/diff/80001/base/task_runner.h File base/task_runner.h (right): https://codereview.chromium.org/2657603004/diff/80001/base/task_runner.h#newc... base/task_runner.h:11: #include "base/callback_forward.h" This need to be callback.h since taking by value. (ditto in other headers) https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... File base/threading/post_task_and_reply_impl.cc (left): https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... base/threading/post_task_and_reply_impl.cc:31: const Closure& task, Can this CL be focused on this change only? (i.e. move the mechanical renames and uncontroversial std::move changes to a precursor CL?)
Description was changed from ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. On the new code, |task| is destroyed on the target thread unless the caller holds a reference on the origin thread. ========== to ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. On the new code, |task| is destroyed on the target thread unless the caller holds a reference on the origin thread. BUG=680359 ==========
On 2017/01/31 19:34:59, gab (travel until 5th) wrote: > On 2017/01/31 15:37:08, danakj (slow) wrote: > > On Mon, Jan 30, 2017 at 8:55 PM, > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dcheng@chromium.org> wrote: > > > > > On 2017/01/30 21:07:47, tzik wrote: > > > > PTAL > > > > > > Nit: I think the CL description might be incomplete: > > > > Destroying the bound object on the original thread requires extra > > > > thread hop on the bound object just. > > > > > > Also, mind adding BUG=680359 to the CL description? > > > > > > In general, I'm a little nervous about this change. I'm not convinced that > > > we > > > don't have anything that secretly depends on this behavior (for example, > > > see > > > https://groups.google.com/a/chromium.org/forum/#! > > > topicsearchin/chromium-dev/RefCountedThreadSafe/chromium-dev/8LqVoXQ_2bo > > > for a past discussion of RefCounted and thread safety). > > > > > > Just to throw ideas out there... is it possible to preserve the existing > > > behavior for RepeatingCallback by just adding an overload for OnceCallback > > > instead? > > > > > > Unrelated note: it also seems like OnceCallback isn't a perfect solution > > > for > > > this either--if a refcounted thread-unsafe object has more than one > > > reference, > > > or the reference is not moved, then we still get explosions. I wonder if we > > > should be DCHECKing that thread-unsafe > > > > > > > This is the same problem WTF::String has. It would be great to react if > > refcounted-not-threadsafe is passed to another thread with > 1 ref. > > Definitely, we should DCHECK if RefCounted's refcount is tweaked in a non-thread > safe manner (can use same logic as WeakPtr's WeakReference::Flag checks which do > handle the one-ref case?). Note that we can't do anything meaningful if the thread-unsafe object is hidden inside another struct though. Though I guess we could have a ScopedEnsureThreadSafety object, and have RefCounted consult it in debug mode... > > I'm in favor of this behavior if we can pull it off. For one it will help use > cases where |task| is an iOS "critical closure" which holds shutdown while its > alive (and as such currently makes the reply shutdown blocking), e.g.: > https://cs.chromium.org/chromium/src/base/files/important_file_writer.cc?sq=p... > > https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... > File base/message_loop/message_loop_task_runner_unittest.cc (right): > > https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... > base/message_loop/message_loop_task_runner_unittest.cc:203: > TEST_F(MessageLoopTaskRunnerTest, PostTaskAndReply_DeadReplyLoopDoesNotDelete) { > Rename > > https://codereview.chromium.org/2657603004/diff/80001/base/task_runner.h > File base/task_runner.h (right): > > https://codereview.chromium.org/2657603004/diff/80001/base/task_runner.h#newc... > base/task_runner.h:11: #include "base/callback_forward.h" > This need to be callback.h since taking by value. > > (ditto in other headers) > > https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... > File base/threading/post_task_and_reply_impl.cc (left): > > https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... > base/threading/post_task_and_reply_impl.cc:31: const Closure& task, > Can this CL be focused on this change only? (i.e. move the mechanical renames > and uncontroversial std::move changes to a precursor CL?)
On 2017/01/31 01:55:40, dcheng wrote: > On 2017/01/30 21:07:47, tzik wrote: > > PTAL > > Nit: I think the CL description might be incomplete: > > Destroying the bound object on the original thread requires extra > > thread hop on the bound object just. > > Also, mind adding BUG=680359 to the CL description? Done > > In general, I'm a little nervous about this change. I'm not convinced that we > don't have anything that secretly depends on this behavior (for example, see > https://groups.google.com/a/chromium.org/forum/#!topicsearchin/chromium-dev/R... > for a past discussion of RefCounted and thread safety). I looked through all 697 callers of PostTaskAndReplyWithResult, and found only a few number of callers that depends on the thread |task| is destroyed on (and I already fixed them). Most of bound arguments are just primitives, pointers, Unretained, std::string, or ThreadSafeRefCounted object. So I believe this doesn't break existing code. > > Just to throw ideas out there... is it possible to preserve the existing > behavior for RepeatingCallback by just adding an overload for OnceCallback > instead? Yeah, it's an option. If OnceCallback is unacceptable, we should go that way. > > Unrelated note: it also seems like OnceCallback isn't a perfect solution for > this either--if a refcounted thread-unsafe object has more than one reference, > or the reference is not moved, then we still get explosions. I wonder if we > should be DCHECKing that thread-unsafe Trying out to add a DCHECK to the RefCountedBase. It mostly works, and detects a few thread unsafe usage of RefCount. However, some of the DCHECK failures seem false positive. E.g. //cc touches main thread objects from the impl thread while main thread is blocked. We need some mechanism to detect the blocked thread, or maybe just a suppression like ScopedDisableSequenceAffinityCheckOfNonThreadSafeRefCounted. It's the next step of the callback overhaul to add thread safety check as Blink's crossThreadBind does, but it's currently out of scope of OnceCallback.
LGTM with nits https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... File base/message_loop/message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... base/message_loop/message_loop_task_runner_unittest.cc:203: TEST_F(MessageLoopTaskRunnerTest, PostTaskAndReply_DeadReplyLoopDoesNotDelete) { On 2017/01/31 19:34:59, gab wrote: > Rename Per gab's comment, how about naming this: PostTaskAndReply_DeadReplyTaskRunnerBehavior https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... base/message_loop/message_loop_task_runner_unittest.cc:240: EXPECT_EQ(task_loop, task_run_on); And add some comments here to explain the new behavior. "Even if the reply task runner is already gone, the original task should already be deleted. However, the reply which hasn't executed yet should leak to avoid thread-safety issues." https://codereview.chromium.org/2657603004/diff/80001/base/task_runner.h File base/task_runner.h (right): https://codereview.chromium.org/2657603004/diff/80001/base/task_runner.h#newc... base/task_runner.h:11: #include "base/callback_forward.h" On 2017/01/31 19:34:59, gab wrote: > This need to be callback.h since taking by value. > > (ditto in other headers) +1 to this https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... base/threading/post_task_and_reply_impl.cc:55: // Ensure |task_| to be released before |reply_| is to ensure that no one Nit: reword to fix some minor typos in the rest of this comment // Ensure |task_| has already been released before |reply_| to ensure // that no one accidentally depends on |task_| keeping one of its // arguments alive while |reply_| is executing.
https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... File base/message_loop/message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... base/message_loop/message_loop_task_runner_unittest.cc:203: TEST_F(MessageLoopTaskRunnerTest, PostTaskAndReply_DeadReplyLoopDoesNotDelete) { On 2017/02/07 07:28:44, dcheng wrote: > On 2017/01/31 19:34:59, gab wrote: > > Rename > > Per gab's comment, how about naming this: > PostTaskAndReply_DeadReplyTaskRunnerBehavior Done. https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... base/message_loop/message_loop_task_runner_unittest.cc:203: TEST_F(MessageLoopTaskRunnerTest, PostTaskAndReply_DeadReplyLoopDoesNotDelete) { On 2017/02/07 07:28:44, dcheng wrote: > On 2017/01/31 19:34:59, gab wrote: > > Rename > > Per gab's comment, how about naming this: > PostTaskAndReply_DeadReplyTaskRunnerBehavior Acknowledged. https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... base/message_loop/message_loop_task_runner_unittest.cc:240: EXPECT_EQ(task_loop, task_run_on); On 2017/02/07 07:28:44, dcheng wrote: > And add some comments here to explain the new behavior. > > "Even if the reply task runner is already gone, the original task should already > be deleted. However, the reply which hasn't executed yet should leak to avoid > thread-safety issues." Acknowledged. https://codereview.chromium.org/2657603004/diff/80001/base/task_runner.h File base/task_runner.h (right): https://codereview.chromium.org/2657603004/diff/80001/base/task_runner.h#newc... base/task_runner.h:11: #include "base/callback_forward.h" On 2017/01/31 19:34:59, gab wrote: > This need to be callback.h since taking by value. > > (ditto in other headers) Done in the other CL. https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... File base/threading/post_task_and_reply_impl.cc (left): https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... base/threading/post_task_and_reply_impl.cc:31: const Closure& task, On 2017/01/31 19:34:59, gab wrote: > Can this CL be focused on this change only? (i.e. move the mechanical renames > and uncontroversial std::move changes to a precursor CL?) Sure. Let me split them to a separate CL. https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... base/threading/post_task_and_reply_impl.cc:55: // Ensure |task_| to be released before |reply_| is to ensure that no one On 2017/02/07 07:28:44, dcheng wrote: > Nit: reword to fix some minor typos in the rest of this comment > > // Ensure |task_| has already been released before |reply_| to ensure > // that no one accidentally depends on |task_| keeping one of its > // arguments alive while |reply_| is executing. Acknowledged.
The CQ bit was checked by tzik@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 checked by tzik@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...
https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... File base/message_loop/message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... base/message_loop/message_loop_task_runner_unittest.cc:203: TEST_F(MessageLoopTaskRunnerTest, PostTaskAndReply_DeadReplyLoopDoesNotDelete) { On 2017/02/07 07:28:44, dcheng wrote: > On 2017/01/31 19:34:59, gab wrote: > > Rename > > Per gab's comment, how about naming this: > PostTaskAndReply_DeadReplyTaskRunnerBehavior Done. https://codereview.chromium.org/2657603004/diff/80001/base/message_loop/messa... base/message_loop/message_loop_task_runner_unittest.cc:240: EXPECT_EQ(task_loop, task_run_on); On 2017/02/07 07:28:44, dcheng wrote: > And add some comments here to explain the new behavior. > > "Even if the reply task runner is already gone, the original task should already > be deleted. However, the reply which hasn't executed yet should leak to avoid > thread-safety issues." Done. https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2657603004/diff/80001/base/threading/post_tas... base/threading/post_task_and_reply_impl.cc:55: // Ensure |task_| to be released before |reply_| is to ensure that no one On 2017/02/07 07:28:44, dcheng wrote: > Nit: reword to fix some minor typos in the rest of this comment > > // Ensure |task_| has already been released before |reply_| to ensure > // that no one accidentally depends on |task_| keeping one of its > // arguments alive while |reply_| is executing. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/07 07:07:45, tzik wrote: > On 2017/01/31 01:55:40, dcheng wrote: > > On 2017/01/30 21:07:47, tzik wrote: > > > PTAL > > > > Nit: I think the CL description might be incomplete: > > > Destroying the bound object on the original thread requires extra > > > thread hop on the bound object just. > > > > Also, mind adding BUG=680359 to the CL description? > > Done > > > > > In general, I'm a little nervous about this change. I'm not convinced that we > > don't have anything that secretly depends on this behavior (for example, see > > > https://groups.google.com/a/chromium.org/forum/#!topicsearchin/chromium-dev/R... > > for a past discussion of RefCounted and thread safety). > > I looked through all 697 callers of PostTaskAndReplyWithResult, and found only a > few number of callers that depends on the thread |task| is destroyed on (and I > already fixed them). Most of bound arguments are just primitives, pointers, > Unretained, std::string, or ThreadSafeRefCounted object. > So I believe this doesn't break existing code. > > > > > Just to throw ideas out there... is it possible to preserve the existing > > behavior for RepeatingCallback by just adding an overload for OnceCallback > > instead? > > Yeah, it's an option. If OnceCallback is unacceptable, we should go that way. > > > > > Unrelated note: it also seems like OnceCallback isn't a perfect solution for > > this either--if a refcounted thread-unsafe object has more than one reference, > > or the reference is not moved, then we still get explosions. I wonder if we > > should be DCHECKing that thread-unsafe > > Trying out to add a DCHECK to the RefCountedBase. It mostly works, and detects a > few thread unsafe usage of RefCount. > However, some of the DCHECK failures seem false positive. E.g. //cc touches main > thread objects from the impl thread while main thread is blocked. > We need some mechanism to detect the blocked thread, or maybe just a suppression > like ScopedDisableSequenceAffinityCheckOfNonThreadSafeRefCounted. ~ how many such callsites are there? I prefer having safety checks that require explicit action to bypass than to assume devs get thread-safety right. For base::Thread we had a use case where the caller was managing thread-safety on its own and hitting DCHECK(CalledOnValidSequence()). What we did is introduce Thread::DetachFromSequence() which allows the caller to explicitly state they know what they're doing. It's easier to review and find mistakes when non-standard callers are explicit about their intent. > It's the next step of the callback overhaul to add thread safety check as > Blink's crossThreadBind does, but it's currently out of scope of OnceCallback. Right, but I thought we said we weren't comfortable with this change without a safety check on RefCounted?
On 2017/02/07 16:01:06, gab wrote: > On 2017/02/07 07:07:45, tzik wrote: > > On 2017/01/31 01:55:40, dcheng wrote: > > > On 2017/01/30 21:07:47, tzik wrote: > > > > PTAL > > > > > > Nit: I think the CL description might be incomplete: > > > > Destroying the bound object on the original thread requires extra > > > > thread hop on the bound object just. > > > > > > Also, mind adding BUG=680359 to the CL description? > > > > Done > > > > > > > > In general, I'm a little nervous about this change. I'm not convinced that > we > > > don't have anything that secretly depends on this behavior (for example, see > > > > > > https://groups.google.com/a/chromium.org/forum/#!topicsearchin/chromium-dev/R... > > > for a past discussion of RefCounted and thread safety). > > > > I looked through all 697 callers of PostTaskAndReplyWithResult, and found only > a > > few number of callers that depends on the thread |task| is destroyed on (and I > > already fixed them). Most of bound arguments are just primitives, pointers, > > Unretained, std::string, or ThreadSafeRefCounted object. > > So I believe this doesn't break existing code. > > > > > > > > Just to throw ideas out there... is it possible to preserve the existing > > > behavior for RepeatingCallback by just adding an overload for OnceCallback > > > instead? > > > > Yeah, it's an option. If OnceCallback is unacceptable, we should go that way. > > > > > > > > Unrelated note: it also seems like OnceCallback isn't a perfect solution for > > > this either--if a refcounted thread-unsafe object has more than one > reference, > > > or the reference is not moved, then we still get explosions. I wonder if we > > > should be DCHECKing that thread-unsafe > > > > Trying out to add a DCHECK to the RefCountedBase. It mostly works, and detects > a > > few thread unsafe usage of RefCount. > > However, some of the DCHECK failures seem false positive. E.g. //cc touches > main > > thread objects from the impl thread while main thread is blocked. > > We need some mechanism to detect the blocked thread, or maybe just a > suppression > > like ScopedDisableSequenceAffinityCheckOfNonThreadSafeRefCounted. > > ~ how many such callsites are there? I prefer having safety checks that require > explicit action to bypass than to assume devs get thread-safety right. There were only two: In an ARC case https://codereview.chromium.org/2655213004/, and a disk_cache case https://codereview.chromium.org/2657003003/. > > For base::Thread we had a use case where the caller was managing thread-safety > on its own and hitting DCHECK(CalledOnValidSequence()). What we did is introduce > Thread::DetachFromSequence() which allows the caller to explicitly state they > know what they're doing. It's easier to review and find mistakes when > non-standard callers are explicit about their intent. > > > It's the next step of the callback overhaul to add thread safety check as > > Blink's crossThreadBind does, but it's currently out of scope of OnceCallback. > > Right, but I thought we said we weren't comfortable with this change without a > safety check on RefCounted? Hmm, OK. I'm trying to add a DCHECK there. Here is a WIP CL: https://codereview.chromium.org/2666423002/
The CQ bit was checked by tzik@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tzik@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/02/07 16:01:06, gab wrote: > On 2017/02/07 07:07:45, tzik wrote: > > On 2017/01/31 01:55:40, dcheng wrote: > > > On 2017/01/30 21:07:47, tzik wrote: > > > > PTAL > > > > > > Nit: I think the CL description might be incomplete: > > > > Destroying the bound object on the original thread requires extra > > > > thread hop on the bound object just. > > > > > > Also, mind adding BUG=680359 to the CL description? > > > > Done > > > > > > > > In general, I'm a little nervous about this change. I'm not convinced that > we > > > don't have anything that secretly depends on this behavior (for example, see > > > > > > https://groups.google.com/a/chromium.org/forum/#!topicsearchin/chromium-dev/R... > > > for a past discussion of RefCounted and thread safety). > > > > I looked through all 697 callers of PostTaskAndReplyWithResult, and found only > a > > few number of callers that depends on the thread |task| is destroyed on (and I > > already fixed them). Most of bound arguments are just primitives, pointers, > > Unretained, std::string, or ThreadSafeRefCounted object. > > So I believe this doesn't break existing code. > > > > > > > > Just to throw ideas out there... is it possible to preserve the existing > > > behavior for RepeatingCallback by just adding an overload for OnceCallback > > > instead? > > > > Yeah, it's an option. If OnceCallback is unacceptable, we should go that way. > > > > > > > > Unrelated note: it also seems like OnceCallback isn't a perfect solution for > > > this either--if a refcounted thread-unsafe object has more than one > reference, > > > or the reference is not moved, then we still get explosions. I wonder if we > > > should be DCHECKing that thread-unsafe > > > > Trying out to add a DCHECK to the RefCountedBase. It mostly works, and detects > a > > few thread unsafe usage of RefCount. > > However, some of the DCHECK failures seem false positive. E.g. //cc touches > main > > thread objects from the impl thread while main thread is blocked. > > We need some mechanism to detect the blocked thread, or maybe just a > suppression > > like ScopedDisableSequenceAffinityCheckOfNonThreadSafeRefCounted. > > ~ how many such callsites are there? I prefer having safety checks that require > explicit action to bypass than to assume devs get thread-safety right. > > For base::Thread we had a use case where the caller was managing thread-safety > on its own and hitting DCHECK(CalledOnValidSequence()). What we did is introduce > Thread::DetachFromSequence() which allows the caller to explicitly state they > know what they're doing. It's easier to review and find mistakes when > non-standard callers are explicit about their intent. > > > It's the next step of the callback overhaul to add thread safety check as > > Blink's crossThreadBind does, but it's currently out of scope of OnceCallback. > > Right, but I thought we said we weren't comfortable with this change without a > safety check on RefCounted? gab: Can we land this CL before adding DCHECK in RefCount? The DCHECK in RefCount detected many data races (http://crbug.com/688072). The bot got almost green even with the DCHECK [1], but the DCHECK-adding CL has several blocker [2,3,4,5]. I think we can resolve one of the blocker [3] cleanly with the TaskRunner migration [6], which is in the following CL of this. [1] https://codereview.chromium.org/2666423002/ [2] https://codereview.chromium.org/2707993005/ [3] https://codereview.chromium.org/2712603002/ [4] https://codereview.chromium.org/2703283005/ [5] A RefCount race in PPAPI, I currently looking into. [6] https://codereview.chromium.org/2637843002/
On 2017/02/23 10:06:49, tzik wrote: > On 2017/02/07 16:01:06, gab wrote: > > On 2017/02/07 07:07:45, tzik wrote: > > > On 2017/01/31 01:55:40, dcheng wrote: > > > > On 2017/01/30 21:07:47, tzik wrote: > > > > > PTAL > > > > > > > > Nit: I think the CL description might be incomplete: > > > > > Destroying the bound object on the original thread requires extra > > > > > thread hop on the bound object just. > > > > > > > > Also, mind adding BUG=680359 to the CL description? > > > > > > Done > > > > > > > > > > > In general, I'm a little nervous about this change. I'm not convinced that > > we > > > > don't have anything that secretly depends on this behavior (for example, > see > > > > > > > > > > https://groups.google.com/a/chromium.org/forum/#!topicsearchin/chromium-dev/R... > > > > for a past discussion of RefCounted and thread safety). > > > > > > I looked through all 697 callers of PostTaskAndReplyWithResult, and found > only > > a > > > few number of callers that depends on the thread |task| is destroyed on (and > I > > > already fixed them). Most of bound arguments are just primitives, pointers, > > > Unretained, std::string, or ThreadSafeRefCounted object. > > > So I believe this doesn't break existing code. > > > > > > > > > > > Just to throw ideas out there... is it possible to preserve the existing > > > > behavior for RepeatingCallback by just adding an overload for OnceCallback > > > > instead? > > > > > > Yeah, it's an option. If OnceCallback is unacceptable, we should go that > way. > > > > > > > > > > > Unrelated note: it also seems like OnceCallback isn't a perfect solution > for > > > > this either--if a refcounted thread-unsafe object has more than one > > reference, > > > > or the reference is not moved, then we still get explosions. I wonder if > we > > > > should be DCHECKing that thread-unsafe > > > > > > Trying out to add a DCHECK to the RefCountedBase. It mostly works, and > detects > > a > > > few thread unsafe usage of RefCount. > > > However, some of the DCHECK failures seem false positive. E.g. //cc touches > > main > > > thread objects from the impl thread while main thread is blocked. > > > We need some mechanism to detect the blocked thread, or maybe just a > > suppression > > > like ScopedDisableSequenceAffinityCheckOfNonThreadSafeRefCounted. > > > > ~ how many such callsites are there? I prefer having safety checks that > require > > explicit action to bypass than to assume devs get thread-safety right. > > > > For base::Thread we had a use case where the caller was managing thread-safety > > on its own and hitting DCHECK(CalledOnValidSequence()). What we did is > introduce > > Thread::DetachFromSequence() which allows the caller to explicitly state they > > know what they're doing. It's easier to review and find mistakes when > > non-standard callers are explicit about their intent. > > > > > It's the next step of the callback overhaul to add thread safety check as > > > Blink's crossThreadBind does, but it's currently out of scope of > OnceCallback. > > > > Right, but I thought we said we weren't comfortable with this change without a > > safety check on RefCounted? > > gab: > Can we land this CL before adding DCHECK in RefCount? > The DCHECK in RefCount detected many data races (http://crbug.com/688072). The > bot got almost green even with the DCHECK [1], but the DCHECK-adding CL has > several blocker [2,3,4,5]. > I think we can resolve one of the blocker [3] cleanly with the TaskRunner > migration [6], which is in the following CL of this. > > [1] https://codereview.chromium.org/2666423002/ > [2] https://codereview.chromium.org/2707993005/ > [3] https://codereview.chromium.org/2712603002/ > [4] https://codereview.chromium.org/2703283005/ > [5] A RefCount race in PPAPI, I currently looking into. > [6] https://codereview.chromium.org/2637843002/ What I'm worried about is that this CL results in more unsafe usage of RefCounted (e.g. which previously happened to be safe per bouncing the destruction back on origin sequence). Having the RefCounted checks in place first will verify this. If you want to upload a new patch set for this CL which is based on top of https://codereview.chromium.org/2666423002/ and you can confirm that a CQ dry-run doesn't result in failures not present in https://codereview.chromium.org/2666423002/ then maybe we can land this one first. Otherwise it seems like a small price to pay to have a TODO in [3] to be addressed when this unwinds. Thanks for doing all of this btw, this is awesome work and will be very beneficial to the codebase :)
The CQ bit was checked by tzik@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 tzik@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 tzik@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: linux_chromium_chromeos_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 tzik@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 tzik@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.
https://codereview.chromium.org/2657603004/diff/260001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2657603004/diff/260001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:64: EXPECT_TRUE(*delete_flag); This test is kind of outdated with this CL, it was previously verifying no deletion until just prior to invoking Reply(). Let's update it to verify deletion right after running task and before reply is scheduled.
The CQ bit was checked by tzik@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 checked by tzik@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...
https://codereview.chromium.org/2657603004/diff/260001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2657603004/diff/260001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:64: EXPECT_TRUE(*delete_flag); On 2017/03/29 16:33:42, gab wrote: > This test is kind of outdated with this CL, it was previously verifying no > deletion until just prior to invoking Reply(). Let's update it to verify > deletion right after running task and before reply is scheduled. Done.
https://codereview.chromium.org/2657603004/diff/300001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2657603004/diff/300001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:75: class WrappedTaskRunner : public SingleThreadTaskRunner { Hmmm sorry I wasn't clear, I didn't mean to make the test more complex, but rather that I think we can simplify it (no longer need to verify in MockObject::Reply()). We can simply do: EXPECT_TRUE(post_runner->HasPendingTask()); EXPECT_FALSE(reply_runner->HasPendingTask()); EXPECT_FALSE(delete_flag); post_runner->RunUntilIdle(); EXPECT_FALSE(post_runner->HasPendingTask()); EXPECT_TRUE(reply_runner->HasPendingTask()); EXPECT_TRUE(delete_flag); reply_runner->RunUntilIdle(); EXPECT_FALSE(post_runner->HasPendingTask()); EXPECT_FALSE(reply_runner->HasPendingTask()); EXPECT_TRUE(delete_flag); without needing MockObject nor WrappedTaskRunner (don't think it matters to verify the state when posting specifically).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by tzik@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...
https://codereview.chromium.org/2657603004/diff/300001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2657603004/diff/300001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:75: class WrappedTaskRunner : public SingleThreadTaskRunner { On 2017/03/29 18:21:35, gab wrote: > Hmmm sorry I wasn't clear, I didn't mean to make the test more complex, but > rather that I think we can simplify it (no longer need to verify in > MockObject::Reply()). > > We can simply do: > > EXPECT_TRUE(post_runner->HasPendingTask()); > EXPECT_FALSE(reply_runner->HasPendingTask()); > EXPECT_FALSE(delete_flag); > > post_runner->RunUntilIdle(); > > EXPECT_FALSE(post_runner->HasPendingTask()); > EXPECT_TRUE(reply_runner->HasPendingTask()); > EXPECT_TRUE(delete_flag); > > reply_runner->RunUntilIdle(); > > EXPECT_FALSE(post_runner->HasPendingTask()); > EXPECT_FALSE(reply_runner->HasPendingTask()); > EXPECT_TRUE(delete_flag); > > > without needing MockObject nor WrappedTaskRunner (don't think it matters to > verify the state when posting specifically). Ah, sorry for confusion. Updated the test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ comment https://codereview.chromium.org/2657603004/diff/320001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2657603004/diff/320001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:97: EXPECT_TRUE(delete_flag); Move to line 93, right after testing::Mock::VerifyAndClear(&mock_object). With a comment like: |task| should have been deleted right after being run.
https://codereview.chromium.org/2657603004/diff/320001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2657603004/diff/320001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:97: EXPECT_TRUE(delete_flag); On 2017/03/30 14:40:22, gab wrote: > Move to line 93, right after testing::Mock::VerifyAndClear(&mock_object). With a > comment like: > > |task| should have been deleted right after being run. Done.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2657603004/#ps340001 (title: "reorder delete_flag check. +comment")
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": 340001, "attempt_start_ts": 1490890943983850,
"parent_rev": "7be44a4d4b5fbbd8787a57a4ffb705dae78bf7e1", "commit_rev":
"e5de8d551e80115b21e0e3ca7d5451d3cee6f3ba"}
Message was sent while issue was closed.
Description was changed from ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. On the new code, |task| is destroyed on the target thread unless the caller holds a reference on the origin thread. BUG=680359 ========== to ========== Clear PostTaskAndReply task on the destination thread This is the behavior-changing part of base::TaskRunner migration from base::Callback to base::OnceCallback. In the previous code, PostTaskAndReply and its family guarantee that |task| is destroyed on the origin thread. If that is not possible, PTAR leaks |task|. However, it's hard to keep that with OnceCallback, and the behavior makes less sense on Blink classes, since most of its types are thread-unsafe and need special handling for thread hop. Destroying the bound object on the original thread requires extra thread hop on the bound object just. On the new code, |task| is destroyed on the target thread unless the caller holds a reference on the origin thread. BUG=680359 Review-Url: https://codereview.chromium.org/2657603004 Cr-Commit-Position: refs/heads/master@{#460821} Committed: https://chromium.googlesource.com/chromium/src/+/e5de8d551e80115b21e0e3ca7d54... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/e5de8d551e80115b21e0e3ca7d54... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
