Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2666423002:
Assert sequence validity on non-thread-safe RefCount manipulations (2) (Closed)
DescriptionAssert sequence validity on non-thread-safe RefCount manipulations
This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy
ref count manipulations. A ref counted object is considered to be bound
to a sequence if the count is more than one. After this CL, ref count
manipulations to a sequenced-bound one cause DCHECK failure.
This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out,
and applies the opt-out as needed.
BUG=688072
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2666423002
Cr-Original-Commit-Position: refs/heads/master@{#460773}
Committed: https://chromium.googlesource.com/chromium/src/+/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068
Review-Url: https://codereview.chromium.org/2666423002
Cr-Commit-Position: refs/heads/master@{#461235}
Committed: https://chromium.googlesource.com/chromium/src/+/3f4231f979090ad23fd89622b3b36ed564650645
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : history fix #Patch Set 4 : fix net_unittests #Patch Set 5 : +suppressor #Patch Set 6 : +SW fix #Patch Set 7 : suppress ppapi false positive #Patch Set 8 : fix devtools race #Patch Set 9 : suppress GpuDataManagerImpl #Patch Set 10 : suppress ResourceBundle and AtExitManager #Patch Set 11 : fix #Patch Set 12 : rebase #Patch Set 13 : +loop_runner_fix. +page_capture_fix #Patch Set 14 : suppress mojo #Patch Set 15 : clean up #Patch Set 16 : fix #Patch Set 17 : rebase #Patch Set 18 : rebase #Patch Set 19 : rebase #Patch Set 20 : font fix & add_to_home_fix #Patch Set 21 : rebase on fonts_fix CL #
Total comments: 24
Patch Set 22 : rebase on snapshot_fix. suppress TextureGroup failure #Patch Set 23 : . #Patch Set 24 : rebase on ImageStorage fix #
Total comments: 8
Patch Set 25 : rebase #Patch Set 26 : comment & variable name fix #Patch Set 27 : . #Patch Set 28 : rebase #Patch Set 29 : rebase #
Total comments: 1
Patch Set 30 : rebase #Patch Set 31 : per-instance opt-out #
Total comments: 1
Patch Set 32 : rebase #Patch Set 33 : rebase #
Total comments: 6
Patch Set 34 : +comment #Patch Set 35 : remove DisableSequenceConsistencyAssertions #Depends on Patchset: Messages
Total messages: 211 (158 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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Description was changed from ========== Assert sequence validity on non-threadsafe RefCount bump ========== to ========== Assert sequence validity on non-threadsafe RefCount bump CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
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: 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
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_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
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: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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...
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Patchset #12 (id:220001) has been deleted
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
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...)
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: 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...
Description was changed from ========== Assert sequence validity on non-threadsafe RefCount bump CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Assert sequence validity on non-threadsafe RefCount manipulations CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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 ========== Assert sequence validity on non-threadsafe RefCount manipulations CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. ==========
tzik@chromium.org changed reviewers: + gab@chromium.org
I think all depending CLs and fixes have landed now. PTAL.
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/16 11:43:49, tzik wrote: > I think all depending CLs and fixes have landed now. > PTAL. Hmm, there seem to be several leftover around gfx::FontList and AddToHomescreenDataFetcher :< I'll fix it separately before landing this CL.
Description was changed from ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2017/02/16 12:48:52, tzik wrote: > On 2017/02/16 11:43:49, tzik wrote: > > I think all depending CLs and fixes have landed now. > > PTAL. > > Hmm, there seem to be several leftover around gfx::FontList and > AddToHomescreenDataFetcher :< > I'll fix it separately before landing this CL. Fixing them on a separate CL. But, there seem yet other failures on Android bots. I'll take a look tomorrow.
Description was changed from ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Took a peak at ref_counted.h/cc, this is very nice, thanks for doing this and the associated cleanups :)! https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... File base/memory/ref_counted.cc (right): https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.cc:67: } Keep these inline (they're no-ops in non-dcheck builds) https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.cc:79: return sequence_checker_.CalledOnValidSequence(); Merge if into this call: return CalledOnValidSequenced() || !AtomicRefCountIsZero(); https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:44: DCHECK(CalledOnValidSequence()); I found this whole comparing with "2" confusing (thought it should be 1) until I realized you're comparing *after add* and *before release*. I'd prefer this to be the other way around, check w.r.t to 1 and Add() after / Release() before the check. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:71: void DetachFromSequence() const; Invoked sequence_checker_.DetachFromSequence() directly and remove the DetachFromSequence() private method. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:72: bool CalledOnValidSequence() const; Wrap method itself in DCHECK_IS_ON() since it's already never called outside those blocks. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:74: mutable size_t ref_count_; Tangential cleanup while you're here = 0; here and drop initialization list altogether in constructor :) https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:109: class BASE_EXPORT ScopedAllowCrossThreadRefCountAccess final { // See RefCounted class for documentation. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:135: // The ref count manipulation to RefCounted is NOT thread safe, and has DCHECKs rm comma https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:136: // to trap unsafe cross thread usages. A subclass instance of RefCounted can be s/usages/usage/ https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:138: // ref count is more than 1, the ref count checks if it's on the same thread or s/checks if it's on the same thread or sequence to the previous ones/verifies the ref updates are made on the same execution sequence as the previous ones/ https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:139: // sequence to the previous ones. s/thread or sequence/execution sequence/ https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:141: // ScopedAllowCrossThreadRefCountAccess to disable the DCHECK. For this last paragraph: // In rare cases where thread-safety is guaranteed through other means (e.g. locks or explicit sequencing of calls across execution sequences): ScopedAllowCrossThreadRefCountAccess can be used to explicitly indicate this and disable the aforementioned check.
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 ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. TODO: fix chrome/browser/web_applications/web_app.h failure. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
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/2666423002/diff/420001/base/memory/ref_counte... File base/memory/ref_counted.cc (right): https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.cc:67: } On 2017/02/16 21:38:46, gab wrote: > Keep these inline (they're no-ops in non-dcheck builds) Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.cc:79: return sequence_checker_.CalledOnValidSequence(); On 2017/02/16 21:38:46, gab wrote: > Merge if into this call: > > return CalledOnValidSequenced() || !AtomicRefCountIsZero(); Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:44: DCHECK(CalledOnValidSequence()); On 2017/02/16 21:38:47, gab wrote: > I found this whole comparing with "2" confusing (thought it should be 1) until I > realized you're comparing *after add* and *before release*. > > I'd prefer this to be the other way around, check w.r.t to 1 and Add() after / > Release() before the check. Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:71: void DetachFromSequence() const; On 2017/02/16 21:38:46, gab wrote: > Invoked sequence_checker_.DetachFromSequence() directly and remove the > DetachFromSequence() private method. Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:72: bool CalledOnValidSequence() const; On 2017/02/16 21:38:47, gab wrote: > Wrap method itself in DCHECK_IS_ON() since it's already never called outside > those blocks. Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:74: mutable size_t ref_count_; On 2017/02/16 21:38:47, gab wrote: > Tangential cleanup while you're here > > = 0; > > here and drop initialization list altogether in constructor :) Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:109: class BASE_EXPORT ScopedAllowCrossThreadRefCountAccess final { On 2017/02/16 21:38:46, gab wrote: > // See RefCounted class for documentation. Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:135: // The ref count manipulation to RefCounted is NOT thread safe, and has DCHECKs On 2017/02/16 21:38:47, gab wrote: > rm comma Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:136: // to trap unsafe cross thread usages. A subclass instance of RefCounted can be On 2017/02/16 21:38:47, gab wrote: > s/usages/usage/ Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:138: // ref count is more than 1, the ref count checks if it's on the same thread or On 2017/02/16 21:38:47, gab wrote: > s/checks if it's on the same thread or sequence to the previous ones/verifies > the ref updates are made on the same execution sequence as the previous ones/ Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:139: // sequence to the previous ones. On 2017/02/16 21:38:47, gab wrote: > s/thread or sequence/execution sequence/ Done. https://codereview.chromium.org/2666423002/diff/420001/base/memory/ref_counte... base/memory/ref_counted.h:141: // ScopedAllowCrossThreadRefCountAccess to disable the DCHECK. On 2017/02/16 21:38:47, gab wrote: > For this last paragraph: > > // In rare cases where thread-safety is guaranteed through other means (e.g. > locks or explicit sequencing of calls across execution sequences): > ScopedAllowCrossThreadRefCountAccess can be used to explicitly indicate this and > disable the aforementioned check. Done.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Description was changed from ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. TODO: fix chrome/browser/web_applications/web_app.h failure. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
//base looks great :) I'm don't think I like the ScopedAllow when behind locks though. Locks are at least as expensive as atomics (in the best case implemented with atomics) and as such these use cases could just use RefCountedThreadSafe for "free". WDYT? I'd suggest changing those to RefCountedThreadSafe and document in that in ref_counted.h instead of suggestion to use ScopedAllow when behind lock. The fewer use cases of ScopedAllow there are the better as it doesn't set precedent for doing this by default. https://codereview.chromium.org/2666423002/diff/480001/base/memory/ref_counte... File base/memory/ref_counted.cc (right): https://codereview.chromium.org/2666423002/diff/480001/base/memory/ref_counte... base/memory/ref_counted.cc:62: sequence_checker_.CalledOnValidSequence(); Flip these (i.e. sequence check || exception). https://codereview.chromium.org/2666423002/diff/480001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/2666423002/diff/480001/base/memory/ref_counte... base/memory/ref_counted.h:149: // passed to the other thread or sequence only when its ref count is 1. If the s/the other/another/ https://codereview.chromium.org/2666423002/diff/480001/base/memory/ref_counte... base/memory/ref_counted.h:150: // ref count is more than 1, the ref count verifies the ref updates are made on s/the ref count/the RefCounted class/ https://codereview.chromium.org/2666423002/diff/480001/gpu/command_buffer/ser... File gpu/command_buffer/service/mailbox_manager_sync.cc (right): https://codereview.chromium.org/2666423002/diff/480001/gpu/command_buffer/ser... gpu/command_buffer/service/mailbox_manager_sync.cc:321: base::ScopedAllowCrossThreadRefCountAccess a; Comment and full variable name everywhere.
Description was changed from ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
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/2666423002/diff/480001/base/memory/ref_counte... File base/memory/ref_counted.cc (right): https://codereview.chromium.org/2666423002/diff/480001/base/memory/ref_counte... base/memory/ref_counted.cc:62: sequence_checker_.CalledOnValidSequence(); On 2017/02/17 21:10:40, gab wrote: > Flip these (i.e. sequence check || exception). Done. https://codereview.chromium.org/2666423002/diff/480001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/2666423002/diff/480001/base/memory/ref_counte... base/memory/ref_counted.h:149: // passed to the other thread or sequence only when its ref count is 1. If the On 2017/02/17 21:10:41, gab wrote: > s/the other/another/ Done. https://codereview.chromium.org/2666423002/diff/480001/base/memory/ref_counte... base/memory/ref_counted.h:150: // ref count is more than 1, the ref count verifies the ref updates are made on On 2017/02/17 21:10:40, gab wrote: > s/the ref count/the RefCounted class/ Done. https://codereview.chromium.org/2666423002/diff/480001/gpu/command_buffer/ser... File gpu/command_buffer/service/mailbox_manager_sync.cc (right): https://codereview.chromium.org/2666423002/diff/480001/gpu/command_buffer/ser... gpu/command_buffer/service/mailbox_manager_sync.cc:321: base::ScopedAllowCrossThreadRefCountAccess a; On 2017/02/17 21:10:41, gab wrote: > Comment and full variable name everywhere. Done.
On 2017/02/17 21:10:41, gab wrote: > //base looks great :) > > I'm don't think I like the ScopedAllow when behind locks though. Locks are at > least as expensive as atomics (in the best case implemented with atomics) and as > such these use cases could just use RefCountedThreadSafe for "free". I agree we should use RefCountedThreadSafe on simple case, but I doubt we can remove these locks since they protects other data too. I prefer RefCountedThreadSafe even behind locks, since, IIUC, it's easy to leak a reference temporarily to another thread, and atomic ops are generally cheap enough when we use them on single thread, and it's easy to leak the reference to a separate thread. > > WDYT? I'd suggest changing those to RefCountedThreadSafe and document in that in > ref_counted.h instead of suggestion to use ScopedAllow when behind lock. > > The fewer use cases of ScopedAllow there are the better as it doesn't set > precedent for doing this by default. >
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/21 07:06:26, tzik wrote: > On 2017/02/17 21:10:41, gab wrote: > > //base looks great :) > > > > I'm don't think I like the ScopedAllow when behind locks though. Locks are at > > least as expensive as atomics (in the best case implemented with atomics) and > as > > such these use cases could just use RefCountedThreadSafe for "free". > > I agree we should use RefCountedThreadSafe on simple case, but I doubt we can > remove these locks since they protects other data too. > I prefer RefCountedThreadSafe even behind locks, since, IIUC, it's easy to leak > a reference temporarily to another thread, and atomic ops are generally cheap > enough when we use them on single thread, and it's easy to leak the reference to > a separate thread. Right, I'm not saying we should remove the usage of those locks, but that we shouldn't ScopedAllow just because we're behind locks (instead those should use RefCountedThreadSafe as the performance gain of the non-atomic RefCounted is already irrelevant when surrounding usage of a Lock -- and it's also error-prone as not all refs modifications may properly be done behind the lock in which case even an "on sequence" modification can race with an "off sequence" modification if the former isn't behind the lock while the latter is...). > > > > > WDYT? I'd suggest changing those to RefCountedThreadSafe and document in that > in > > ref_counted.h instead of suggestion to use ScopedAllow when behind lock. > > > > The fewer use cases of ScopedAllow there are the better as it doesn't set > > precedent for doing this by default. > >
On 2017/02/21 07:06:26, tzik wrote: > On 2017/02/17 21:10:41, gab wrote: > > //base looks great :) > > > > I'm don't think I like the ScopedAllow when behind locks though. Locks are at > > least as expensive as atomics (in the best case implemented with atomics) and > as > > such these use cases could just use RefCountedThreadSafe for "free". > > I agree we should use RefCountedThreadSafe on simple case, but I doubt we can > remove these locks since they protects other data too. > I prefer RefCountedThreadSafe even behind locks, since, IIUC, it's easy to leak > a reference temporarily to another thread, and atomic ops are generally cheap > enough when we use them on single thread, and it's easy to leak the reference to > a separate thread. Right, I'm not saying we should remove the usage of those locks, but that we shouldn't ScopedAllow just because we're behind locks (instead those should use RefCountedThreadSafe as the performance gain of the non-atomic RefCounted is already irrelevant when surrounding usage of a Lock -- and it's also error-prone as not all refs modifications may properly be done behind the lock in which case even an "on sequence" modification can race with an "off sequence" modification if the former isn't behind the lock while the latter is...). > > > > > WDYT? I'd suggest changing those to RefCountedThreadSafe and document in that > in > > ref_counted.h instead of suggestion to use ScopedAllow when behind lock. > > > > The fewer use cases of ScopedAllow there are the better as it doesn't set > > precedent for doing this by default. > >
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.
Description was changed from ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
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.
tzik@chromium.org changed reviewers: + danakj@chromium.org, raymes@chromium.org, yzshen@chromium.org, zmo@chromium.org
All race fixes have landed. Let's resume this to prevent further race. On 2017/02/21 21:45:38, gab (OOO until Mar 13) wrote: > On 2017/02/21 07:06:26, tzik wrote: > > On 2017/02/17 21:10:41, gab wrote: > > > //base looks great :) > > > > > > I'm don't think I like the ScopedAllow when behind locks though. Locks are > at > > > least as expensive as atomics (in the best case implemented with atomics) > and > > as > > > such these use cases could just use RefCountedThreadSafe for "free". > > > > I agree we should use RefCountedThreadSafe on simple case, but I doubt we can > > remove these locks since they protects other data too. > > I prefer RefCountedThreadSafe even behind locks, since, IIUC, it's easy to > leak > > a reference temporarily to another thread, and atomic ops are generally cheap > > enough when we use them on single thread, and it's easy to leak the reference > to > > a separate thread. > > Right, I'm not saying we should remove the usage of those locks, but that we > shouldn't ScopedAllow just because we're behind locks (instead those should use > RefCountedThreadSafe as the performance gain of the non-atomic RefCounted is > already irrelevant when surrounding usage of a Lock -- and it's also error-prone > as not all refs modifications may properly be done behind the lock in which case > even an "on sequence" modification can race with an "off sequence" modification > if the former isn't behind the lock while the latter is...). I'd defer that to the component owners. raymes, zmo, yzshen, danakj: Does it make sense to convert these ref counted classes to RefCountedThreadSafe? +raymes, zmo, yzshen and danakj. PTAL to raymes: //ppapi zmo: //gpu and //content/browser/gpu yzshen: //mojo danakj: //cc
On 2017/03/03 12:05:49, tzik wrote: > All race fixes have landed. Let's resume this to prevent further race. > > On 2017/02/21 21:45:38, gab (OOO until Mar 13) wrote: > > On 2017/02/21 07:06:26, tzik wrote: > > > On 2017/02/17 21:10:41, gab wrote: > > > > //base looks great :) > > > > > > > > I'm don't think I like the ScopedAllow when behind locks though. Locks are > > at > > > > least as expensive as atomics (in the best case implemented with atomics) > > and > > > as > > > > such these use cases could just use RefCountedThreadSafe for "free". > > > > > > I agree we should use RefCountedThreadSafe on simple case, but I doubt we > can > > > remove these locks since they protects other data too. > > > I prefer RefCountedThreadSafe even behind locks, since, IIUC, it's easy to > > leak > > > a reference temporarily to another thread, and atomic ops are generally > cheap > > > enough when we use them on single thread, and it's easy to leak the > reference > > to > > > a separate thread. > > > > Right, I'm not saying we should remove the usage of those locks, but that we > > shouldn't ScopedAllow just because we're behind locks (instead those should > use > > RefCountedThreadSafe as the performance gain of the non-atomic RefCounted is > > already irrelevant when surrounding usage of a Lock -- and it's also > error-prone > > as not all refs modifications may properly be done behind the lock in which > case > > even an "on sequence" modification can race with an "off sequence" > modification > > if the former isn't behind the lock while the latter is...). > > I'd defer that to the component owners. > raymes, zmo, yzshen, danakj: Does it make sense to convert these ref counted > classes to RefCountedThreadSafe? > > +raymes, zmo, yzshen and danakj. PTAL to > raymes: //ppapi > zmo: //gpu and //content/browser/gpu > yzshen: //mojo > danakj: //cc cc LGTM
On 2017/03/03 17:32:14, danakj wrote: > On 2017/03/03 12:05:49, tzik wrote: > > All race fixes have landed. Let's resume this to prevent further race. > > > > On 2017/02/21 21:45:38, gab (OOO until Mar 13) wrote: > > > On 2017/02/21 07:06:26, tzik wrote: > > > > On 2017/02/17 21:10:41, gab wrote: > > > > > //base looks great :) > > > > > > > > > > I'm don't think I like the ScopedAllow when behind locks though. Locks > are > > > at > > > > > least as expensive as atomics (in the best case implemented with > atomics) > > > and > > > > as > > > > > such these use cases could just use RefCountedThreadSafe for "free". > > > > > > > > I agree we should use RefCountedThreadSafe on simple case, but I doubt we > > can > > > > remove these locks since they protects other data too. > > > > I prefer RefCountedThreadSafe even behind locks, since, IIUC, it's easy to > > > leak > > > > a reference temporarily to another thread, and atomic ops are generally > > cheap > > > > enough when we use them on single thread, and it's easy to leak the > > reference > > > to > > > > a separate thread. > > > > > > Right, I'm not saying we should remove the usage of those locks, but that we > > > shouldn't ScopedAllow just because we're behind locks (instead those should > > use > > > RefCountedThreadSafe as the performance gain of the non-atomic RefCounted is > > > already irrelevant when surrounding usage of a Lock -- and it's also > > error-prone > > > as not all refs modifications may properly be done behind the lock in which > > case > > > even an "on sequence" modification can race with an "off sequence" > > modification > > > if the former isn't behind the lock while the latter is...). > > > > I'd defer that to the component owners. > > raymes, zmo, yzshen, danakj: Does it make sense to convert these ref counted > > classes to RefCountedThreadSafe? > > > > +raymes, zmo, yzshen and danakj. PTAL to > > raymes: //ppapi > > zmo: //gpu and //content/browser/gpu > > yzshen: //mojo > > danakj: //cc > > cc LGTM I'm not sure if I understand the question fully. Is the question whether the performance of RefCountedThreadSafe would be problematic for these classes in ppapi? I don't think it would be given all the other overheads involved.
gpu side lgtm
https://codereview.chromium.org/2666423002/diff/580001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): https://codereview.chromium.org/2666423002/diff/580001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/lib/multiplex_router.cc:436: base::ScopedAllowCrossThreadRefCountAccess Can this opt-out be configured with a type (in this case MultiplexRouter::Endpoint), instead on each callsite? That would be less verbose.
On 2017/03/06 17:53:52, yzshen1 wrote: > https://codereview.chromium.org/2666423002/diff/580001/mojo/public/cpp/bindin... > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > https://codereview.chromium.org/2666423002/diff/580001/mojo/public/cpp/bindin... > mojo/public/cpp/bindings/lib/multiplex_router.cc:436: > base::ScopedAllowCrossThreadRefCountAccess > Can this opt-out be configured with a type (in this case > MultiplexRouter::Endpoint), instead on each callsite? That would be less > verbose. Since the type itself doesn't have the protection and the call sites do as the lock, I think the opt-out should be done at the call site around the lock. Can we keep it there?
On 2017/03/07 13:50:27, tzik wrote: > On 2017/03/06 17:53:52, yzshen1 wrote: > > > https://codereview.chromium.org/2666423002/diff/580001/mojo/public/cpp/bindin... > > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > > > > https://codereview.chromium.org/2666423002/diff/580001/mojo/public/cpp/bindin... > > mojo/public/cpp/bindings/lib/multiplex_router.cc:436: > > base::ScopedAllowCrossThreadRefCountAccess > > Can this opt-out be configured with a type (in this case > > MultiplexRouter::Endpoint), instead on each callsite? That would be less > > verbose. > > Since the type itself doesn't have the protection and the call sites do as the > lock, I think the opt-out should be done at the call site around the lock. Can > we keep it there? IMO, if a developer intentionally takes responsibility to protect multi-threaded access to a non-thread ref-counted object, it is a promise for that entire object. That means he/she needs to consistently protect the ref-count under the exact same lock. On the other hand, SACTRCA disables the check for all ref-counted objects within its scope (IIUC). Looking at the SACTRCA calls, it doesn't tell what objects would like to opt-out of the check, and the opt-out may not be applied consistently across all usage of those objects. That is why I think having SACTRCA at each callsite seems a little verbose, and also seems to be the wrong level. WDYT? Thanks! IIUC, with the current implementation, if the object is accessed on the thread that it is bound to, there is no need to have SACTRCA. In this case, it can still race with the
On 2017/03/07 16:49:31, yzshen1 wrote: > On 2017/03/07 13:50:27, tzik wrote: > > On 2017/03/06 17:53:52, yzshen1 wrote: > > > > > > https://codereview.chromium.org/2666423002/diff/580001/mojo/public/cpp/bindin... > > > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > > > > > > > > https://codereview.chromium.org/2666423002/diff/580001/mojo/public/cpp/bindin... > > > mojo/public/cpp/bindings/lib/multiplex_router.cc:436: > > > base::ScopedAllowCrossThreadRefCountAccess > > > Can this opt-out be configured with a type (in this case > > > MultiplexRouter::Endpoint), instead on each callsite? That would be less > > > verbose. > > > > Since the type itself doesn't have the protection and the call sites do as the > > lock, I think the opt-out should be done at the call site around the lock. Can > > we keep it there? > > IMO, if a developer intentionally takes responsibility to protect multi-threaded > access to a non-thread ref-counted object, it is a promise for that entire > object. That means he/she needs to consistently protect the ref-count under the > exact same lock. > > On the other hand, SACTRCA disables the check for all ref-counted objects within > its scope (IIUC). Looking at the SACTRCA calls, it doesn't tell what objects > would like to opt-out of the check, and the opt-out may not be applied > consistently across all usage of those objects. > > That is why I think having SACTRCA at each callsite seems a little verbose, and > also seems to be the wrong level. > > WDYT? Thanks! > > > IIUC, with the current implementation, if the object is accessed on the thread > that it is bound to, there is no need to have SACTRCA. In this case, it can > still race with the OK. Can we do it in a separate follow-up CL? Adding per-type whitelist needs additional template trick around RefCounted, and I want to avoid blocking following CLs with this.
On Mon, Mar 13, 2017 at 2:48 AM, <tzik@chromium.org> wrote: > On 2017/03/07 16:49:31, yzshen1 wrote: > > On 2017/03/07 13:50:27, tzik wrote: > > > On 2017/03/06 17:53:52, yzshen1 wrote: > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/580001/ > mojo/public/cpp/bindings/lib/multiplex_router.cc > > > > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/580001/ > mojo/public/cpp/bindings/lib/multiplex_router.cc#newcode436 > > > > mojo/public/cpp/bindings/lib/multiplex_router.cc:436: > > > > base::ScopedAllowCrossThreadRefCountAccess > > > > Can this opt-out be configured with a type (in this case > > > > MultiplexRouter::Endpoint), instead on each callsite? That would be > less > > > > verbose. > > > > > > Since the type itself doesn't have the protection and the call sites > do as > the > > > lock, I think the opt-out should be done at the call site around the > lock. > Can > > > we keep it there? > > > > IMO, if a developer intentionally takes responsibility to protect > multi-threaded > > access to a non-thread ref-counted object, it is a promise for that > entire > > object. That means he/she needs to consistently protect the ref-count > under > the > > exact same lock. > > > > On the other hand, SACTRCA disables the check for all ref-counted objects > within > > its scope (IIUC). Looking at the SACTRCA calls, it doesn't tell what > objects > > would like to opt-out of the check, and the opt-out may not be applied > > consistently across all usage of those objects. > > > > That is why I think having SACTRCA at each callsite seems a little > verbose, > and > > also seems to be the wrong level. > > > > WDYT? Thanks! > > > > > > IIUC, with the current implementation, if the object is accessed on the > thread > > that it is bound to, there is no need to have SACTRCA. In this case, it > can > > still race with the > > OK. Can we do it in a separate follow-up CL? Adding per-type whitelist > needs > additional template trick around RefCounted, and I want to avoid blocking > following CLs with this. > Hm, this is tricky because there also may be types that are used cross-thread safely in one place, but that doesn't mean every use is safe and it could hide other uses to tie it to the type globally. > > https://codereview.chromium.org/2666423002/ > -- 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.
Description was changed from ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
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_...)
On 2017/03/13 14:41:39, danakj wrote: > On Mon, Mar 13, 2017 at 2:48 AM, <mailto:tzik@chromium.org> wrote: > > > On 2017/03/07 16:49:31, yzshen1 wrote: > > > On 2017/03/07 13:50:27, tzik wrote: > > > > On 2017/03/06 17:53:52, yzshen1 wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/580001/ > > mojo/public/cpp/bindings/lib/multiplex_router.cc > > > > > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/580001/ > > mojo/public/cpp/bindings/lib/multiplex_router.cc#newcode436 > > > > > mojo/public/cpp/bindings/lib/multiplex_router.cc:436: > > > > > base::ScopedAllowCrossThreadRefCountAccess > > > > > Can this opt-out be configured with a type (in this case > > > > > MultiplexRouter::Endpoint), instead on each callsite? That would be > > less > > > > > verbose. > > > > > > > > Since the type itself doesn't have the protection and the call sites > > do as > > the > > > > lock, I think the opt-out should be done at the call site around the > > lock. > > Can > > > > we keep it there? > > > > > > IMO, if a developer intentionally takes responsibility to protect > > multi-threaded > > > access to a non-thread ref-counted object, it is a promise for that > > entire > > > object. That means he/she needs to consistently protect the ref-count > > under > > the > > > exact same lock. > > > > > > On the other hand, SACTRCA disables the check for all ref-counted objects > > within > > > its scope (IIUC). Looking at the SACTRCA calls, it doesn't tell what > > objects > > > would like to opt-out of the check, and the opt-out may not be applied > > > consistently across all usage of those objects. > > > > > > That is why I think having SACTRCA at each callsite seems a little > > verbose, > > and > > > also seems to be the wrong level. > > > > > > WDYT? Thanks! > > > > > > > > > IIUC, with the current implementation, if the object is accessed on the > > thread > > > that it is bound to, there is no need to have SACTRCA. In this case, it > > can > > > still race with the > > > > OK. Can we do it in a separate follow-up CL? Adding per-type whitelist > > needs > > additional template trick around RefCounted, and I want to avoid blocking > > following CLs with this. > > > > Hm, this is tricky because there also may be types that are used > cross-thread safely in one place, but that doesn't mean every use is safe > and it could hide other uses to tie it to the type globally. So maybe it should be a policy for each individual instance (as opposed to all instances of a class)? Would you please give an example for me to better understand it? > > > > > > https://codereview.chromium.org/2666423002/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
On 2017/03/13 06:48:20, tzik wrote: > On 2017/03/07 16:49:31, yzshen1 wrote: > > On 2017/03/07 13:50:27, tzik wrote: > > > On 2017/03/06 17:53:52, yzshen1 wrote: > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/580001/mojo/public/cpp/bindin... > > > > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/580001/mojo/public/cpp/bindin... > > > > mojo/public/cpp/bindings/lib/multiplex_router.cc:436: > > > > base::ScopedAllowCrossThreadRefCountAccess > > > > Can this opt-out be configured with a type (in this case > > > > MultiplexRouter::Endpoint), instead on each callsite? That would be less > > > > verbose. > > > > > > Since the type itself doesn't have the protection and the call sites do as > the > > > lock, I think the opt-out should be done at the call site around the lock. > Can > > > we keep it there? > > > > IMO, if a developer intentionally takes responsibility to protect > multi-threaded > > access to a non-thread ref-counted object, it is a promise for that entire > > object. That means he/she needs to consistently protect the ref-count under > the > > exact same lock. > > > > On the other hand, SACTRCA disables the check for all ref-counted objects > within > > its scope (IIUC). Looking at the SACTRCA calls, it doesn't tell what objects > > would like to opt-out of the check, and the opt-out may not be applied > > consistently across all usage of those objects. > > > > That is why I think having SACTRCA at each callsite seems a little verbose, > and > > also seems to be the wrong level. > > > > WDYT? Thanks! > > > > > > IIUC, with the current implementation, if the object is accessed on the thread > > that it is bound to, there is no need to have SACTRCA. In this case, it can > > still race with the > > OK. Can we do it in a separate follow-up CL? Adding per-type whitelist needs > additional template trick around RefCounted, and I want to avoid blocking > following CLs with this. How about this: change the Endpoint class to use ThreadSafeRefCounted, if the perf bots are still happy (I guess they would), and add a TODO to change it back the non-thread-safe RefCounted when the support is ready? I think that is still better than using per-call-site opt-out.
On Mon, Mar 13, 2017 at 2:10 PM, <yzshen@chromium.org> wrote: > On 2017/03/13 14:41:39, danakj wrote: > > > On Mon, Mar 13, 2017 at 2:48 AM, <mailto:tzik@chromium.org> wrote: > > > > > On 2017/03/07 16:49:31, yzshen1 wrote: > > > > On 2017/03/07 13:50:27, tzik wrote: > > > > > On 2017/03/06 17:53:52, yzshen1 wrote: > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/580001/ > > > mojo/public/cpp/bindings/lib/multiplex_router.cc > > > > > > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/580001/ > > > mojo/public/cpp/bindings/lib/multiplex_router.cc#newcode436 > > > > > > mojo/public/cpp/bindings/lib/multiplex_router.cc:436: > > > > > > base::ScopedAllowCrossThreadRefCountAccess > > > > > > Can this opt-out be configured with a type (in this case > > > > > > MultiplexRouter::Endpoint), instead on each callsite? That would > be > > > less > > > > > > verbose. > > > > > > > > > > Since the type itself doesn't have the protection and the call > sites > > > do as > > > the > > > > > lock, I think the opt-out should be done at the call site around > the > > > lock. > > > Can > > > > > we keep it there? > > > > > > > > IMO, if a developer intentionally takes responsibility to protect > > > multi-threaded > > > > access to a non-thread ref-counted object, it is a promise for that > > > entire > > > > object. That means he/she needs to consistently protect the ref-count > > > under > > > the > > > > exact same lock. > > > > > > > > On the other hand, SACTRCA disables the check for all ref-counted > objects > > > within > > > > its scope (IIUC). Looking at the SACTRCA calls, it doesn't tell what > > > objects > > > > would like to opt-out of the check, and the opt-out may not be > applied > > > > consistently across all usage of those objects. > > > > > > > > That is why I think having SACTRCA at each callsite seems a little > > > verbose, > > > and > > > > also seems to be the wrong level. > > > > > > > > WDYT? Thanks! > > > > > > > > > > > > IIUC, with the current implementation, if the object is accessed on > the > > > thread > > > > that it is bound to, there is no need to have SACTRCA. In this case, > it > > > can > > > > still race with the > > > > > > OK. Can we do it in a separate follow-up CL? Adding per-type whitelist > > > needs > > > additional template trick around RefCounted, and I want to avoid > blocking > > > following CLs with this. > > > > > > > Hm, this is tricky because there also may be types that are used > > cross-thread safely in one place, but that doesn't mean every use is safe > > and it could hide other uses to tie it to the type globally. > > So maybe it should be a policy for each individual instance (as opposed to > all > instances of a class)? > Would you please give an example for me to better understand it? > Like, you can have a refcounted Widget, which is used in two classes. In class A, Widget is used across threads but its refcount is protected by A::lock_. In class B, it is not meant for use across threads, so there is no lock. If B's instance happened to end up refcounting across threads, this patch would catch that (there'd be no scoped silencing of DCHECKs outside of class A). But whitelisting Widget would not catch it. > > > > > > > > > > > > > > https://codereview.chromium.org/2666423002/ > > > > > > > -- > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/2666423002/ > -- 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.
On Mon, Mar 13, 2017 at 11:12 AM, <danakj@chromium.org> wrote: > On Mon, Mar 13, 2017 at 2:10 PM, <yzshen@chromium.org> wrote: > >> On 2017/03/13 14:41:39, danakj wrote: >> >> > On Mon, Mar 13, 2017 at 2:48 AM, <mailto:tzik@chromium.org> wrote: >> > >> > > On 2017/03/07 16:49:31, yzshen1 wrote: >> > > > On 2017/03/07 13:50:27, tzik wrote: >> > > > > On 2017/03/06 17:53:52, yzshen1 wrote: >> > > > > > >> > > > > >> > > > >> > > https://codereview.chromium.org/2666423002/diff/580001/ >> > > mojo/public/cpp/bindings/lib/multiplex_router.cc >> > > > > > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): >> > > > > > >> > > > > > >> > > > > >> > > > >> > > https://codereview.chromium.org/2666423002/diff/580001/ >> > > mojo/public/cpp/bindings/lib/multiplex_router.cc#newcode436 >> > > > > > mojo/public/cpp/bindings/lib/multiplex_router.cc:436: >> > > > > > base::ScopedAllowCrossThreadRefCountAccess >> > > > > > Can this opt-out be configured with a type (in this case >> > > > > > MultiplexRouter::Endpoint), instead on each callsite? That >> would be >> > > less >> > > > > > verbose. >> > > > > >> > > > > Since the type itself doesn't have the protection and the call >> sites >> > > do as >> > > the >> > > > > lock, I think the opt-out should be done at the call site around >> the >> > > lock. >> > > Can >> > > > > we keep it there? >> > > > >> > > > IMO, if a developer intentionally takes responsibility to protect >> > > multi-threaded >> > > > access to a non-thread ref-counted object, it is a promise for that >> > > entire >> > > > object. That means he/she needs to consistently protect the >> ref-count >> > > under >> > > the >> > > > exact same lock. >> > > > >> > > > On the other hand, SACTRCA disables the check for all ref-counted >> objects >> > > within >> > > > its scope (IIUC). Looking at the SACTRCA calls, it doesn't tell what >> > > objects >> > > > would like to opt-out of the check, and the opt-out may not be >> applied >> > > > consistently across all usage of those objects. >> > > > >> > > > That is why I think having SACTRCA at each callsite seems a little >> > > verbose, >> > > and >> > > > also seems to be the wrong level. >> > > > >> > > > WDYT? Thanks! >> > > > >> > > > >> > > > IIUC, with the current implementation, if the object is accessed on >> the >> > > thread >> > > > that it is bound to, there is no need to have SACTRCA. In this >> case, it >> > > can >> > > > still race with the >> > > >> > > OK. Can we do it in a separate follow-up CL? Adding per-type whitelist >> > > needs >> > > additional template trick around RefCounted, and I want to avoid >> blocking >> > > following CLs with this. >> > > >> > >> > Hm, this is tricky because there also may be types that are used >> > cross-thread safely in one place, but that doesn't mean every use is >> safe >> > and it could hide other uses to tie it to the type globally. >> >> So maybe it should be a policy for each individual instance (as opposed >> to all >> instances of a class)? >> Would you please give an example for me to better understand it? >> > > Like, you can have a refcounted Widget, which is used in two classes. In > class A, Widget is used across threads but its refcount is protected by > A::lock_. In class B, it is not meant for use across threads, so there is > no lock. If B's instance happened to end up refcounting across threads, > this patch would catch that (there'd be no scoped silencing of DCHECKs > outside of class A). But whitelisting Widget would not catch it. > Right. I think it makes sense to use per-instance policy in this case. We explicitly opt-out the check for the Widget instance in class A. > > >> >> >> >> >> > >> > >> > > >> > > https://codereview.chromium.org/2666423002/ >> > > >> > >> > -- >> > 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 mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> >> https://codereview.chromium.org/2666423002/ >> > > -- 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.
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...
Added per-instance opt-out and applied it to Mojo.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojo LGTM https://codereview.chromium.org/2666423002/diff/620001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/2666423002/diff/620001/base/memory/ref_counte... base/memory/ref_counted.h:165: // and disable the aforementioned check. Nit: Please also mention DisableSequenceConsistencyAssertions in this comment.
ppapi lgtm
Let me re-iterate this as I think owners started reviewing this CL not this question... we should change RefCounted usage that's behind locks to just use RefCountedThreadSafe -- the overhead of atomics is nil in the presence of the overhead of locks and avoiding the ScopedAllow(...) is better as it avoids complexity (if we can avoid introducing this scoped allowance altogether that's even better). > > we shouldn't ScopedAllow just because we're behind locks (instead those should > use > > RefCountedThreadSafe as the performance gain of the non-atomic RefCounted is > > already irrelevant when surrounding usage of a Lock -- and it's also > error-prone > > as not all refs modifications may properly be done behind the lock in which > case > > even an "on sequence" modification can race with an "off sequence" > modification > > if the former isn't behind the lock while the latter is...). > > I'd defer that to the component owners. > raymes, zmo, yzshen, danakj: Does it make sense to convert these ref counted > classes to RefCountedThreadSafe? > > +raymes, zmo, yzshen and danakj. PTAL to > raymes: //ppapi > zmo: //gpu and //content/browser/gpu > yzshen: //mojo > danakj: //cc
On Wed, Mar 15, 2017 at 4:34 PM, <gab@chromium.org> wrote: > Let me re-iterate this as I think owners started reviewing this CL not this > question... we should change RefCounted usage that's behind locks to just > use > RefCountedThreadSafe -- the overhead of atomics is nil in the presence of > the > overhead of locks and avoiding the ScopedAllow(...) is better as it avoids > complexity (if we can avoid introducing this scoped allowance altogether > that's > even better). > I can't speak to all the cases, but in cc, it's a refptr that is used on the compositor thread only when the main thread is blocked. This is a well-known state in the system called commit, where the compositor thread uses data structures from the blocked main thread to build structures for its own thread. There's no reason to use atomics, and there's no locks trying to replace atomics. > > > > > we shouldn't ScopedAllow just because we're behind locks (instead those > should > > use > > > RefCountedThreadSafe as the performance gain of the non-atomic > RefCounted is > > > already irrelevant when surrounding usage of a Lock -- and it's also > > error-prone > > > as not all refs modifications may properly be done behind the lock in > which > > case > > > even an "on sequence" modification can race with an "off sequence" > > modification > > > if the former isn't behind the lock while the latter is...). > > > > I'd defer that to the component owners. > > raymes, zmo, yzshen, danakj: Does it make sense to convert these ref > counted > > classes to RefCountedThreadSafe? > > > > +raymes, zmo, yzshen and danakj. PTAL to > > raymes: //ppapi > > zmo: //gpu and //content/browser/gpu > > yzshen: //mojo > > danakj: //cc > > > > https://codereview.chromium.org/2666423002/ > -- 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.
On 2017/03/15 20:34:34, gab wrote: > Let me re-iterate this as I think owners started reviewing this CL not this > question... we should change RefCounted usage that's behind locks to just use > RefCountedThreadSafe -- the overhead of atomics is nil in the presence of the > overhead of locks and avoiding the ScopedAllow(...) is better as it avoids > complexity (if we can avoid introducing this scoped allowance altogether that's > even better). In other words, I will not l.g.t.m. this CL with ScopedAllowCrossThreadRefCountAccess unless there's a true valid use case where RefCountedThreadSafe would be a detriment and allowing is clearly better. i.e. I'm imposing RefCountedThreadSafe on existing use cases, not asking owners' permissions, point them to me if you get push back, thanks! > > > > we shouldn't ScopedAllow just because we're behind locks (instead those > should > > use > > > RefCountedThreadSafe as the performance gain of the non-atomic RefCounted is > > > already irrelevant when surrounding usage of a Lock -- and it's also > > error-prone > > > as not all refs modifications may properly be done behind the lock in which > > case > > > even an "on sequence" modification can race with an "off sequence" > > modification > > > if the former isn't behind the lock while the latter is...). > > > > I'd defer that to the component owners. > > raymes, zmo, yzshen, danakj: Does it make sense to convert these ref counted > > classes to RefCountedThreadSafe? > > > > +raymes, zmo, yzshen and danakj. PTAL to > > raymes: //ppapi > > zmo: //gpu and //content/browser/gpu > > yzshen: //mojo > > danakj: //cc
On 2017/03/15 20:37:46, danakj wrote: > On Wed, Mar 15, 2017 at 4:34 PM, <mailto:gab@chromium.org> wrote: > > > Let me re-iterate this as I think owners started reviewing this CL not this > > question... we should change RefCounted usage that's behind locks to just > > use > > RefCountedThreadSafe -- the overhead of atomics is nil in the presence of > > the > > overhead of locks and avoiding the ScopedAllow(...) is better as it avoids > > complexity (if we can avoid introducing this scoped allowance altogether > > that's > > even better). > > > > I can't speak to all the cases, but in cc, it's a refptr that is used on > the compositor thread only when the main thread is blocked. This is a > well-known state in the system called commit, where the compositor thread > uses data structures from the blocked main thread to build structures for > its own thread. There's no reason to use atomics, and there's no locks > trying to replace atomics. Ah ok, yes in that case that makes sense. The question is, say that's the only remaining use case, it it worth having ScopedAllowCrossThreadRefCountAccess just for it? PostTask uses locks so once again maybe the atomics for refcount are no big deal..? > > > > > > > > > > we shouldn't ScopedAllow just because we're behind locks (instead those > > should > > > use > > > > RefCountedThreadSafe as the performance gain of the non-atomic > > RefCounted is > > > > already irrelevant when surrounding usage of a Lock -- and it's also > > > error-prone > > > > as not all refs modifications may properly be done behind the lock in > > which > > > case > > > > even an "on sequence" modification can race with an "off sequence" > > > modification > > > > if the former isn't behind the lock while the latter is...). > > > > > > I'd defer that to the component owners. > > > raymes, zmo, yzshen, danakj: Does it make sense to convert these ref > > counted > > > classes to RefCountedThreadSafe? > > > > > > +raymes, zmo, yzshen and danakj. PTAL to > > > raymes: //ppapi > > > zmo: //gpu and //content/browser/gpu > > > yzshen: //mojo > > > danakj: //cc > > > > > > > > https://codereview.chromium.org/2666423002/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
On Wed, Mar 15, 2017 at 4:40 PM, <gab@chromium.org> wrote: > On 2017/03/15 20:37:46, danakj wrote: > > On Wed, Mar 15, 2017 at 4:34 PM, <mailto:gab@chromium.org> wrote: > > > > > Let me re-iterate this as I think owners started reviewing this CL not > this > > > question... we should change RefCounted usage that's behind locks to > just > > > use > > > RefCountedThreadSafe -- the overhead of atomics is nil in the presence > of > > > the > > > overhead of locks and avoiding the ScopedAllow(...) is better as it > avoids > > > complexity (if we can avoid introducing this scoped allowance > altogether > > > that's > > > even better). > > > > > > > I can't speak to all the cases, but in cc, it's a refptr that is used on > > the compositor thread only when the main thread is blocked. This is a > > well-known state in the system called commit, where the compositor thread > > uses data structures from the blocked main thread to build structures for > > its own thread. There's no reason to use atomics, and there's no locks > > trying to replace atomics. > > Ah ok, yes in that case that makes sense. The question is, say that's the > only > remaining use case, it it worth having ScopedAllowCrossThreadRefCountAccess > just > for it? PostTask uses locks so once again maybe the atomics for refcount > are no > big deal..? > I'd need performance tests to show. This scoped object is at the top of commit. Commit may use the entire compositor data structures, all layers, pictures, things in skia, etc.. This isn't a scoped object for a single refptr instance or anything. *also* I think changing to ThreadSafeRefCounted would be extremely misleading and I would not be happy about it. ThreadSafeRefCounted means the references can change on any thread -> implies it can be destroyed on any thread and needs very different expectations. In this case even tho technically the compositor thread ends up running the code, conceptually it is single threaded and only needs to deal with the main thread. > > > > > > > > > > > > > > > we shouldn't ScopedAllow just because we're behind locks (instead > those > > > should > > > > use > > > > > RefCountedThreadSafe as the performance gain of the non-atomic > > > RefCounted is > > > > > already irrelevant when surrounding usage of a Lock -- and it's > also > > > > error-prone > > > > > as not all refs modifications may properly be done behind the lock > in > > > which > > > > case > > > > > even an "on sequence" modification can race with an "off sequence" > > > > modification > > > > > if the former isn't behind the lock while the latter is...). > > > > > > > > I'd defer that to the component owners. > > > > raymes, zmo, yzshen, danakj: Does it make sense to convert these ref > > > counted > > > > classes to RefCountedThreadSafe? > > > > > > > > +raymes, zmo, yzshen and danakj. PTAL to > > > > raymes: //ppapi > > > > zmo: //gpu and //content/browser/gpu > > > > yzshen: //mojo > > > > danakj: //cc > > > > > > > > > > > > https://codereview.chromium.org/2666423002/ > > > > > > > -- > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > > https://codereview.chromium.org/2666423002/ > -- 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.
Le mer. 15 mars 2017 16:46, <danakj@chromium.org> a écrit : > On Wed, Mar 15, 2017 at 4:40 PM, <gab@chromium.org> wrote: > > On 2017/03/15 20:37:46, danakj wrote: > > On Wed, Mar 15, 2017 at 4:34 PM, <mailto:gab@chromium.org> wrote: > > > > > Let me re-iterate this as I think owners started reviewing this CL not > this > > > question... we should change RefCounted usage that's behind locks to > just > > > use > > > RefCountedThreadSafe -- the overhead of atomics is nil in the presence > of > > > the > > > overhead of locks and avoiding the ScopedAllow(...) is better as it > avoids > > > complexity (if we can avoid introducing this scoped allowance > altogether > > > that's > > > even better). > > > > > > > I can't speak to all the cases, but in cc, it's a refptr that is used on > > the compositor thread only when the main thread is blocked. This is a > > well-known state in the system called commit, where the compositor thread > > uses data structures from the blocked main thread to build structures for > > its own thread. There's no reason to use atomics, and there's no locks > > trying to replace atomics. > > Ah ok, yes in that case that makes sense. The question is, say that's the > only > remaining use case, it it worth having > ScopedAllowCrossThreadRefCountAccess just > for it? PostTask uses locks so once again maybe the atomics for refcount > are no > big deal..? > > > I'd need performance tests to show. This scoped object is at the top of > commit. Commit may use the entire compositor data structures, all layers, > pictures, things in skia, etc.. This isn't a scoped object for a single > refptr instance or anything. > > *also* I think changing to ThreadSafeRefCounted would be extremely > misleading and I would not be happy about it. ThreadSafeRefCounted means > the references can change on any thread -> implies it can be destroyed on > any thread and needs very different expectations. In this case even tho > technically the compositor thread ends up running the code, conceptually it > is single threaded and only needs to deal with the main thread. > So why even pass the ref then? Pass it Unretained if the cc thread never owns? > > > > > > > > > > > > > > > > we shouldn't ScopedAllow just because we're behind locks (instead > those > > > should > > > > use > > > > > RefCountedThreadSafe as the performance gain of the non-atomic > > > RefCounted is > > > > > already irrelevant when surrounding usage of a Lock -- and it's > also > > > > error-prone > > > > > as not all refs modifications may properly be done behind the lock > in > > > which > > > > case > > > > > even an "on sequence" modification can race with an "off sequence" > > > > modification > > > > > if the former isn't behind the lock while the latter is...). > > > > > > > > I'd defer that to the component owners. > > > > raymes, zmo, yzshen, danakj: Does it make sense to convert these ref > > > counted > > > > classes to RefCountedThreadSafe? > > > > > > > > +raymes, zmo, yzshen and danakj. PTAL to > > > > raymes: //ppapi > > > > zmo: //gpu and //content/browser/gpu > > > > yzshen: //mojo > > > > danakj: //cc > > > > > > > > > > > > https://codereview.chromium.org/2666423002/ > > > > > > > -- > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > > https://codereview.chromium.org/2666423002/ > > -- 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.
On 2017/03/15 20:34:34, gab wrote: > Let me re-iterate this as I think owners started reviewing this CL not this > question... we should change RefCounted usage that's behind locks to just use > RefCountedThreadSafe -- the overhead of atomics is nil in the presence of the > overhead of locks and avoiding the ScopedAllow(...) is better as it avoids > complexity (if we can avoid introducing this scoped allowance altogether that's > even better). I did try to answer the question for ppapi: > I'm not sure if I understand the question fully. Is the question whether the > performance of RefCountedThreadSafe would be problematic for these classes in > ppapi? I don't think it would be given all the other overheads involved. I think either approach is ok for ppapi from a performance standpoint.
On Wed, Mar 15, 2017 at 6:25 PM, Gabriel Charette <gab@google.com> wrote: > > > Le mer. 15 mars 2017 16:46, <danakj@chromium.org> a écrit : > >> On Wed, Mar 15, 2017 at 4:40 PM, <gab@chromium.org> wrote: >> >> On 2017/03/15 20:37:46, danakj wrote: >> > On Wed, Mar 15, 2017 at 4:34 PM, <mailto:gab@chromium.org> wrote: >> > >> > > Let me re-iterate this as I think owners started reviewing this CL >> not this >> > > question... we should change RefCounted usage that's behind locks to >> just >> > > use >> > > RefCountedThreadSafe -- the overhead of atomics is nil in the >> presence of >> > > the >> > > overhead of locks and avoiding the ScopedAllow(...) is better as it >> avoids >> > > complexity (if we can avoid introducing this scoped allowance >> altogether >> > > that's >> > > even better). >> > > >> > >> > I can't speak to all the cases, but in cc, it's a refptr that is used on >> > the compositor thread only when the main thread is blocked. This is a >> > well-known state in the system called commit, where the compositor >> thread >> > uses data structures from the blocked main thread to build structures >> for >> > its own thread. There's no reason to use atomics, and there's no locks >> > trying to replace atomics. >> >> Ah ok, yes in that case that makes sense. The question is, say that's the >> only >> remaining use case, it it worth having ScopedAllowCrossThreadRefCountAccess >> just >> for it? PostTask uses locks so once again maybe the atomics for refcount >> are no >> big deal..? >> >> >> I'd need performance tests to show. This scoped object is at the top of >> commit. Commit may use the entire compositor data structures, all layers, >> pictures, things in skia, etc.. This isn't a scoped object for a single >> refptr instance or anything. >> >> *also* I think changing to ThreadSafeRefCounted would be extremely >> misleading and I would not be happy about it. ThreadSafeRefCounted means >> the references can change on any thread -> implies it can be destroyed on >> any thread and needs very different expectations. In this case even tho >> technically the compositor thread ends up running the code, conceptually it >> is single threaded and only needs to deal with the main thread. >> > > So why even pass the ref then? Pass it Unretained if the cc thread never > owns? > It's possible we should be move()ing refptrs that we are not, and temp objects are being created during the passing of ownership from the main thread to the compositor thread or something, or maybe ownership is moving within main-thread structures, in code run on the compositor thread. I don't know which pointers and which lines of code are violating the DCHECKs to say more. > > >> >> > >> > >> > > >> > > >> > > > > we shouldn't ScopedAllow just because we're behind locks (instead >> those >> > > should >> > > > use >> > > > > RefCountedThreadSafe as the performance gain of the non-atomic >> > > RefCounted is >> > > > > already irrelevant when surrounding usage of a Lock -- and it's >> also >> > > > error-prone >> > > > > as not all refs modifications may properly be done behind the >> lock in >> > > which >> > > > case >> > > > > even an "on sequence" modification can race with an "off sequence" >> > > > modification >> > > > > if the former isn't behind the lock while the latter is...). >> > > > >> > > > I'd defer that to the component owners. >> > > > raymes, zmo, yzshen, danakj: Does it make sense to convert these ref >> > > counted >> > > > classes to RefCountedThreadSafe? >> > > > >> > > > +raymes, zmo, yzshen and danakj. PTAL to >> > > > raymes: //ppapi >> > > > zmo: //gpu and //content/browser/gpu >> > > > yzshen: //mojo >> > > > danakj: //cc >> > > >> > > >> > > >> > > https://codereview.chromium.org/2666423002/ >> > > >> > >> > -- >> > 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 mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> https://codereview.chromium.org/2666423002/ >> >> -- 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.
On 2017/03/16 15:55:39, danakj wrote: > On Wed, Mar 15, 2017 at 6:25 PM, Gabriel Charette <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@google.com> wrote: > > > > > > > Le mer. 15 mars 2017 16:46, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=danakj@chromium.org> a écrit : > > > >> On Wed, Mar 15, 2017 at 4:40 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote: > >> > >> On 2017/03/15 20:37:46, danakj wrote: > >> > On Wed, Mar 15, 2017 at 4:34 PM, <mailto:gab@chromium.org> wrote: > >> > > >> > > Let me re-iterate this as I think owners started reviewing this CL > >> not this > >> > > question... we should change RefCounted usage that's behind locks to > >> just > >> > > use > >> > > RefCountedThreadSafe -- the overhead of atomics is nil in the > >> presence of > >> > > the > >> > > overhead of locks and avoiding the ScopedAllow(...) is better as it > >> avoids > >> > > complexity (if we can avoid introducing this scoped allowance > >> altogether > >> > > that's > >> > > even better). > >> > > > >> > > >> > I can't speak to all the cases, but in cc, it's a refptr that is used on > >> > the compositor thread only when the main thread is blocked. This is a > >> > well-known state in the system called commit, where the compositor > >> thread > >> > uses data structures from the blocked main thread to build structures > >> for > >> > its own thread. There's no reason to use atomics, and there's no locks > >> > trying to replace atomics. > >> > >> Ah ok, yes in that case that makes sense. The question is, say that's the > >> only > >> remaining use case, it it worth having ScopedAllowCrossThreadRefCountAccess > >> just > >> for it? PostTask uses locks so once again maybe the atomics for refcount > >> are no > >> big deal..? > >> > >> > >> I'd need performance tests to show. This scoped object is at the top of > >> commit. Commit may use the entire compositor data structures, all layers, > >> pictures, things in skia, etc.. This isn't a scoped object for a single > >> refptr instance or anything. > >> > >> *also* I think changing to ThreadSafeRefCounted would be extremely > >> misleading and I would not be happy about it. ThreadSafeRefCounted means > >> the references can change on any thread -> implies it can be destroyed on > >> any thread and needs very different expectations. In this case even tho > >> technically the compositor thread ends up running the code, conceptually it > >> is single threaded and only needs to deal with the main thread. > >> > > > > So why even pass the ref then? Pass it Unretained if the cc thread never > > owns? > > > > It's possible we should be move()ing refptrs that we are not, and temp > objects are being created during the passing of ownership from the main > thread to the compositor thread or something, or maybe ownership is moving > within main-thread structures, in code run on the compositor thread. I > don't know which pointers and which lines of code are violating the DCHECKs > to say more. Okay, would be great to figure that out but I no longer think it needs to block this CL. I'm fine with adding ScopedAllowCrossThreadRefCountAccess if we use it in the bare minimal set of places initially (it should have a big comment on it saying that it's there to allow pre-existing "safe" use cases to live in the presence of the check while we figure out why they hit the check -- missing move or whatnot -- and shouldn't be used for new use cases). And for those use cases that are solely safe per being behind locks I still think we should use RefCountedThreadSafe per my previous comments.
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
Description was changed from ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
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...
On 2017/03/20 16:50:43, gab wrote: > On 2017/03/16 15:55:39, danakj wrote: > > On Wed, Mar 15, 2017 at 6:25 PM, Gabriel Charette > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@google.com> wrote: > > > > > > > > > > > Le mer. 15 mars 2017 16:46, > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=danakj@chromium.org> a écrit > : > > > > > >> On Wed, Mar 15, 2017 at 4:40 PM, > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote: > > >> > > >> On 2017/03/15 20:37:46, danakj wrote: > > >> > On Wed, Mar 15, 2017 at 4:34 PM, <mailto:gab@chromium.org> wrote: > > >> > > > >> > > Let me re-iterate this as I think owners started reviewing this CL > > >> not this > > >> > > question... we should change RefCounted usage that's behind locks to > > >> just > > >> > > use > > >> > > RefCountedThreadSafe -- the overhead of atomics is nil in the > > >> presence of > > >> > > the > > >> > > overhead of locks and avoiding the ScopedAllow(...) is better as it > > >> avoids > > >> > > complexity (if we can avoid introducing this scoped allowance > > >> altogether > > >> > > that's > > >> > > even better). > > >> > > > > >> > > > >> > I can't speak to all the cases, but in cc, it's a refptr that is used on > > >> > the compositor thread only when the main thread is blocked. This is a > > >> > well-known state in the system called commit, where the compositor > > >> thread > > >> > uses data structures from the blocked main thread to build structures > > >> for > > >> > its own thread. There's no reason to use atomics, and there's no locks > > >> > trying to replace atomics. > > >> > > >> Ah ok, yes in that case that makes sense. The question is, say that's the > > >> only > > >> remaining use case, it it worth having ScopedAllowCrossThreadRefCountAccess > > >> just > > >> for it? PostTask uses locks so once again maybe the atomics for refcount > > >> are no > > >> big deal..? > > >> > > >> > > >> I'd need performance tests to show. This scoped object is at the top of > > >> commit. Commit may use the entire compositor data structures, all layers, > > >> pictures, things in skia, etc.. This isn't a scoped object for a single > > >> refptr instance or anything. > > >> > > >> *also* I think changing to ThreadSafeRefCounted would be extremely > > >> misleading and I would not be happy about it. ThreadSafeRefCounted means > > >> the references can change on any thread -> implies it can be destroyed on > > >> any thread and needs very different expectations. In this case even tho > > >> technically the compositor thread ends up running the code, conceptually it > > >> is single threaded and only needs to deal with the main thread. > > >> > > > > > > So why even pass the ref then? Pass it Unretained if the cc thread never > > > owns? > > > > > > > It's possible we should be move()ing refptrs that we are not, and temp > > objects are being created during the passing of ownership from the main > > thread to the compositor thread or something, or maybe ownership is moving > > within main-thread structures, in code run on the compositor thread. I > > don't know which pointers and which lines of code are violating the DCHECKs > > to say more. > > Okay, would be great to figure that out but I no longer think it needs to block > this CL. I'm fine with adding ScopedAllowCrossThreadRefCountAccess if we use it > in the bare minimal set of places initially (it should have a big comment on it > saying that it's there to allow pre-existing "safe" use cases to live in the > presence of the check while we figure out why they hit the check -- missing move > or whatnot -- and shouldn't be used for new use cases). And for those use cases > that are solely safe per being behind locks I still think we should use > RefCountedThreadSafe per my previous comments I think the number of ScopedAllowCrossThreadRefCountAccess is not important enough to stop adding the assertion itself. Can we handle these exception separately on following CLs?
https://codereview.chromium.org/2666423002/diff/660001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/2666423002/diff/660001/base/memory/ref_counte... base/memory/ref_counted.h:131: class BASE_EXPORT ScopedAllowCrossThreadRefCountAccess final { // ScopedAllowCrossThreadRefCountAccess disables the check documented on RefCounted below for rare pre-existing use cases where thread-safety was guaranteed through other means (e.g. explicit sequencing of calls across execution sequences when bouncing between threads in order). New callers should refrain from using this (callsites handling thread-safety through locks should use RefCountedThreadSafe per the overhead of its atomics being negligible compared to locks anyways and callsites doing explicit sequencing should properly std::move() the ref to avoid hitting this check). // TODO(tzik): Cleanup existing use cases and remove ScopedAllowCrossThreadRefCountAccess. https://codereview.chromium.org/2666423002/diff/660001/base/memory/ref_counte... base/memory/ref_counted.h:165: // and disable the aforementioned check. Remove last sentence so it's not part of main API (no new users of ScopedAllowCrossThreadRefCountAccess should be added) and move documentation above with comment as suggested. https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/lib/multiplex_router.cc:964: endpoint->DisableSequenceConsistencyAssertions(); That seems like a very large hammer, can we use the scoped object where relevant instead? Otherwise how do we know all callers are truly doing the right thing with the received pointer? I'd much rather not have DisableSequenceConsistencyAssertions() if we can help it (and otherwise it should have a large comment saying what it's there for and not to use it in new use cases). Also, outside the scope of this CL but why is this stored in a raw ptr and returned as a raw ptr if it's RefCounted..?
https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/lib/multiplex_router.cc:964: endpoint->DisableSequenceConsistencyAssertions(); On 2017/03/24 15:46:45, gab wrote: > That seems like a very large hammer, can we use the scoped object where relevant > instead? Otherwise how do we know all callers are truly doing the right thing > with the received pointer? I'd much rather not have > DisableSequenceConsistencyAssertions() if we can help it (and otherwise it > should have a large comment saying what it's there for and not to use it in new > use cases). I am the one who suggested that the check should be disabled at the object level, instead of using the scoped-allow object. :) That is because: - The scoped-allow object disables the check for *all* ref-counted objects in its scope. - When we want to disable the check for an object, it is a promise to behave correctly for all usage of that object across its lifetime. That is not a per-callsite choice. For example, all callsites should use the same lock to guard addref/release for that object. Considering these reasons, I don't think the scoped-allow approach is at the right level. > > Also, outside the scope of this CL but why is this stored in a raw ptr and > returned as a raw ptr if it's RefCounted..? (I think this is a question to me.) It saves one unnecessary add-ref for the return value of FindEndpoint(). I agree that it is not a big difference though.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think I have expressed my opinion clearly that ScopedAllowCrossThreadRefCountAccess is not a correct API. WRT the MultiplexRouter::Endpoint case, I don't think it would be a huge perf difference to change it to ThreadSafeRefCounted. Please feel free to do that. Then we have one less thing to argue about. :) On Fri, Mar 24, 2017 at 9:58 AM, <yzshen@chromium.org> wrote: > > https://codereview.chromium.org/2666423002/diff/660001/ > mojo/public/cpp/bindings/lib/multiplex_router.cc > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > https://codereview.chromium.org/2666423002/diff/660001/ > mojo/public/cpp/bindings/lib/multiplex_router.cc#newcode964 > mojo/public/cpp/bindings/lib/multiplex_router.cc:964: > endpoint->DisableSequenceConsistencyAssertions(); > On 2017/03/24 15:46:45, gab wrote: > > That seems like a very large hammer, can we use the scoped object > where relevant > > instead? Otherwise how do we know all callers are truly doing the > right thing > > with the received pointer? I'd much rather not have > > DisableSequenceConsistencyAssertions() if we can help it (and > otherwise it > > should have a large comment saying what it's there for and not to use > it in new > > use cases). > > > I am the one who suggested that the check should be disabled at the > object level, instead of using the scoped-allow object. :) > > That is because: > - The scoped-allow object disables the check for *all* ref-counted > objects in its scope. > - When we want to disable the check for an object, it is a promise to > behave correctly for all usage of that object across its lifetime. That > is not a per-callsite choice. For example, all callsites should use the > same lock to guard addref/release for that object. > > Considering these reasons, I don't think the scoped-allow approach is at > the right level. > > > > > > Also, outside the scope of this CL but why is this stored in a raw ptr > and > > returned as a raw ptr if it's RefCounted..? > > (I think this is a question to me.) > It saves one unnecessary add-ref for the return value of FindEndpoint(). > I agree that it is not a big difference though. > > https://codereview.chromium.org/2666423002/ > -- 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.
I tried to make these points earlier but just to reiterate why this doesn't seem one-size-fits-all.. On Fri, Mar 24, 2017 at 12:58 PM, <yzshen@chromium.org> wrote: > > https://codereview.chromium.org/2666423002/diff/660001/ > mojo/public/cpp/bindings/lib/multiplex_router.cc > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > https://codereview.chromium.org/2666423002/diff/660001/ > mojo/public/cpp/bindings/lib/multiplex_router.cc#newcode964 > mojo/public/cpp/bindings/lib/multiplex_router.cc:964: > endpoint->DisableSequenceConsistencyAssertions(); > On 2017/03/24 15:46:45, gab wrote: > > That seems like a very large hammer, can we use the scoped object > where relevant > > instead? Otherwise how do we know all callers are truly doing the > right thing > > with the received pointer? I'd much rather not have > > DisableSequenceConsistencyAssertions() if we can help it (and > otherwise it > > should have a large comment saying what it's there for and not to use > it in new > > use cases). > > > I am the one who suggested that the check should be disabled at the > object level, instead of using the scoped-allow object. :) > > That is because: > - The scoped-allow object disables the check for *all* ref-counted > objects in its scope. > If a thread is blocked then we want it to apply to all objects on that thread, but only while it is blocked. The timing matters not the objects. > - When we want to disable the check for an object, it is a promise to > behave correctly for all usage of that object across its lifetime. That > is not a per-callsite choice. For example, all callsites should use the > same lock to guard addref/release for that object. > Some objects are not actually threadsafe and we want to guard against bad behaviour outside of windows where it is safe. > > Considering these reasons, I don't think the scoped-allow approach is at > the right level. > > > > > > Also, outside the scope of this CL but why is this stored in a raw ptr > and > > returned as a raw ptr if it's RefCounted..? > > (I think this is a question to me.) > It saves one unnecessary add-ref for the return value of FindEndpoint(). > I agree that it is not a big difference though. > > https://codereview.chromium.org/2666423002/ > -- 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.
On Fri, Mar 24, 2017 at 10:32 AM, <danakj@chromium.org> wrote: > I tried to make these points earlier but just to reiterate why this > doesn't seem one-size-fits-all.. > > On Fri, Mar 24, 2017 at 12:58 PM, <yzshen@chromium.org> wrote: > >> >> https://codereview.chromium.org/2666423002/diff/660001/mojo/ >> public/cpp/bindings/lib/multiplex_router.cc >> File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): >> >> https://codereview.chromium.org/2666423002/diff/660001/mojo/ >> public/cpp/bindings/lib/multiplex_router.cc#newcode964 >> mojo/public/cpp/bindings/lib/multiplex_router.cc:964: >> endpoint->DisableSequenceConsistencyAssertions(); >> On 2017/03/24 15:46:45, gab wrote: >> > That seems like a very large hammer, can we use the scoped object >> where relevant >> > instead? Otherwise how do we know all callers are truly doing the >> right thing >> > with the received pointer? I'd much rather not have >> > DisableSequenceConsistencyAssertions() if we can help it (and >> otherwise it >> > should have a large comment saying what it's there for and not to use >> it in new >> > use cases). >> >> >> I am the one who suggested that the check should be disabled at the >> object level, instead of using the scoped-allow object. :) >> >> That is because: >> - The scoped-allow object disables the check for *all* ref-counted >> objects in its scope. >> > > If a thread is blocked then we want it to apply to all objects on that > thread, but only while it is blocked. The timing matters not the objects. > I agree that it could be useful in this particular case. That being said, it seems easy to misuse this helper in other cases. > > >> - When we want to disable the check for an object, it is a promise to >> behave correctly for all usage of that object across its lifetime. That >> is not a per-callsite choice. For example, all callsites should use the >> same lock to guard addref/release for that object. >> > > Some objects are not actually threadsafe and we want to guard against bad > behaviour outside of windows where it is safe. > Okay. I am persuaded that this class is useful in certain cases. :) > > Considering these reasons, I don't think the scoped-allow approach is at > the right level. > > > > > > Also, outside the scope of this CL but why is this stored in a raw ptr > and > > returned as a raw ptr if it's RefCounted..? > > (I think this is a question to me.) > It saves one unnecessary add-ref for the return value of FindEndpoint(). > I agree that it is not a big difference though. > > https://codereview.chromium.org/2666423002/ > -- 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.
Le ven. 24 mars 2017 12:58, <yzshen@chromium.org> a écrit : > > > https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > > https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... > mojo/public/cpp/bindings/lib/multiplex_router.cc:964: > endpoint->DisableSequenceConsistencyAssertions(); > On 2017/03/24 15:46:45, gab wrote: > > That seems like a very large hammer, can we use the scoped object > where relevant > > instead? Otherwise how do we know all callers are truly doing the > right thing > > with the received pointer? I'd much rather not have > > DisableSequenceConsistencyAssertions() if we can help it (and > otherwise it > > should have a large comment saying what it's there for and not to use > it in new > > use cases). > > > I am the one who suggested that the check should be disabled at the > object level, instead of using the scoped-allow object. :) > > That is because: > - The scoped-allow object disables the check for *all* ref-counted > objects in its scope. > - When we want to disable the check for an object, it is a promise to > behave correctly for all usage of that object across its lifetime. That > is not a per-callsite choice. For example, all callsites should use the > same lock to guard addref/release for that object. > > Considering these reasons, I don't think the scoped-allow approach is at > the right level. > Am I understanding correctly from your other reply that this object doesn't use RefCountedThreadSafe because it's always used behind a lock? If so I'd much prefer to make it RefCountedThreadSafe anyways (as I mentioned multiple times above) as the overhead of atomics is negligible compared to locks so the optimization of using the unsafe version isn't worth it while on the other hand not introducing this escape hatch is very much desired. > > > > > > Also, outside the scope of this CL but why is this stored in a raw ptr > and > > returned as a raw ptr if it's RefCounted..? > > (I think this is a question to me.) > It saves one unnecessary add-ref for the return value of FindEndpoint(). > I agree that it is not a big difference though. > I don't have link handy from mobile now but Chromium style is to always pass pointers through scoped_refptr/std::unique_ptr unless the caller merely uses the value without taking a ref. In this case this doesn't save an add-ref because C++11 move semantics would move the ref to the caller anyways. > https://codereview.chromium.org/2666423002/ > -- 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.
On Fri, Mar 24, 2017 at 3:06 PM, Gabriel Charette <gab@google.com> wrote: > > > Le ven. 24 mars 2017 12:58, <yzshen@chromium.org> a écrit : > >> >> https://codereview.chromium.org/2666423002/diff/660001/ >> mojo/public/cpp/bindings/lib/multiplex_router.cc >> File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): >> >> https://codereview.chromium.org/2666423002/diff/660001/ >> mojo/public/cpp/bindings/lib/multiplex_router.cc#newcode964 >> mojo/public/cpp/bindings/lib/multiplex_router.cc:964: >> endpoint->DisableSequenceConsistencyAssertions(); >> On 2017/03/24 15:46:45, gab wrote: >> > That seems like a very large hammer, can we use the scoped object >> where relevant >> > instead? Otherwise how do we know all callers are truly doing the >> right thing >> > with the received pointer? I'd much rather not have >> > DisableSequenceConsistencyAssertions() if we can help it (and >> otherwise it >> > should have a large comment saying what it's there for and not to use >> it in new >> > use cases). >> >> >> I am the one who suggested that the check should be disabled at the >> object level, instead of using the scoped-allow object. :) >> >> That is because: >> - The scoped-allow object disables the check for *all* ref-counted >> objects in its scope. >> - When we want to disable the check for an object, it is a promise to >> behave correctly for all usage of that object across its lifetime. That >> is not a per-callsite choice. For example, all callsites should use the >> same lock to guard addref/release for that object. >> >> Considering these reasons, I don't think the scoped-allow approach is at >> the right level. >> > > Am I understanding correctly from your other reply that this object > doesn't use RefCountedThreadSafe because it's always used behind a lock? If > so I'd much prefer to make it RefCountedThreadSafe anyways (as I mentioned > multiple times above) as the overhead of atomics is negligible compared to > locks so the optimization of using the unsafe version isn't worth it while > on the other hand not introducing this escape hatch is very much desired. > > >> >> >> > >> > Also, outside the scope of this CL but why is this stored in a raw ptr >> and >> > returned as a raw ptr if it's RefCounted..? >> >> (I think this is a question to me.) >> It saves one unnecessary add-ref for the return value of FindEndpoint(). >> I agree that it is not a big difference though. >> > > I don't have link handy from mobile now but Chromium style is to always > pass pointers through scoped_refptr/std::unique_ptr unless the caller > merely uses the value without taking a ref. > FindEndpoint() meets this criteria: users use it without taking a ref. That is why it doesn't return a scoped_refptr. > > In this case this doesn't save an add-ref because C++11 move semantics > would move the ref to the caller anyways. > > > >> https://codereview.chromium.org/2666423002/ >> > -- 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.
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/2666423002/diff/660001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/2666423002/diff/660001/base/memory/ref_counte... base/memory/ref_counted.h:131: class BASE_EXPORT ScopedAllowCrossThreadRefCountAccess final { On 2017/03/24 15:46:45, gab wrote: > // ScopedAllowCrossThreadRefCountAccess disables the check documented on > RefCounted below for rare pre-existing use cases where thread-safety was > guaranteed through other means (e.g. explicit sequencing of calls across > execution sequences when bouncing between threads in order). New callers should > refrain from using this (callsites handling thread-safety through locks should > use RefCountedThreadSafe per the overhead of its atomics being negligible > compared to locks anyways and callsites doing explicit sequencing should > properly std::move() the ref to avoid hitting this check). > // TODO(tzik): Cleanup existing use cases and remove > ScopedAllowCrossThreadRefCountAccess. Done. https://codereview.chromium.org/2666423002/diff/660001/base/memory/ref_counte... base/memory/ref_counted.h:165: // and disable the aforementioned check. On 2017/03/24 15:46:45, gab wrote: > Remove last sentence so it's not part of main API (no new users of > ScopedAllowCrossThreadRefCountAccess should be added) and move documentation > above with comment as suggested. Done.
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_...)
On 2017/03/24 22:06:46, chromium-reviews wrote: > Le ven. 24 mars 2017 12:58, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=yzshen@chromium.org> a écrit : > > > > > > > > https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... > > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > > > > > > https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... > > mojo/public/cpp/bindings/lib/multiplex_router.cc:964: > > endpoint->DisableSequenceConsistencyAssertions(); > > On 2017/03/24 15:46:45, gab wrote: > > > That seems like a very large hammer, can we use the scoped object > > where relevant > > > instead? Otherwise how do we know all callers are truly doing the > > right thing > > > with the received pointer? I'd much rather not have > > > DisableSequenceConsistencyAssertions() if we can help it (and > > otherwise it > > > should have a large comment saying what it's there for and not to use > > it in new > > > use cases). > > > > > > I am the one who suggested that the check should be disabled at the > > object level, instead of using the scoped-allow object. :) > > > > That is because: > > - The scoped-allow object disables the check for *all* ref-counted > > objects in its scope. > > - When we want to disable the check for an object, it is a promise to > > behave correctly for all usage of that object across its lifetime. That > > is not a per-callsite choice. For example, all callsites should use the > > same lock to guard addref/release for that object. > > > > Considering these reasons, I don't think the scoped-allow approach is at > > the right level. > > > > Am I understanding correctly from your other reply that this object doesn't > use RefCountedThreadSafe because it's always used behind a lock? If so I'd > much prefer to make it RefCountedThreadSafe anyways (as I mentioned > multiple times above) as the overhead of atomics is negligible compared to > locks so the optimization of using the unsafe version isn't worth it while > on the other hand not introducing this escape hatch is very much desired. Ping on this, looks like |endpoint|'s are refcounted from |endpoints_| and always used behind |lock_|, can we once again make these RefCountedThreadSafe and avoid introducing DisableSequenceConsistencyAssertions()? (keeping only ScopedAllow is already much better than having two of these escape hatches)
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...
On 2017/03/28 15:42:34, gab wrote: > On 2017/03/24 22:06:46, chromium-reviews wrote: > > Le ven. 24 mars 2017 12:58, > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=yzshen@chromium.org> a écrit > : > > > > > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... > > > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... > > > mojo/public/cpp/bindings/lib/multiplex_router.cc:964: > > > endpoint->DisableSequenceConsistencyAssertions(); > > > On 2017/03/24 15:46:45, gab wrote: > > > > That seems like a very large hammer, can we use the scoped object > > > where relevant > > > > instead? Otherwise how do we know all callers are truly doing the > > > right thing > > > > with the received pointer? I'd much rather not have > > > > DisableSequenceConsistencyAssertions() if we can help it (and > > > otherwise it > > > > should have a large comment saying what it's there for and not to use > > > it in new > > > > use cases). > > > > > > > > > I am the one who suggested that the check should be disabled at the > > > object level, instead of using the scoped-allow object. :) > > > > > > That is because: > > > - The scoped-allow object disables the check for *all* ref-counted > > > objects in its scope. > > > - When we want to disable the check for an object, it is a promise to > > > behave correctly for all usage of that object across its lifetime. That > > > is not a per-callsite choice. For example, all callsites should use the > > > same lock to guard addref/release for that object. > > > > > > Considering these reasons, I don't think the scoped-allow approach is at > > > the right level. > > > > > > > Am I understanding correctly from your other reply that this object doesn't > > use RefCountedThreadSafe because it's always used behind a lock? If so I'd > > much prefer to make it RefCountedThreadSafe anyways (as I mentioned > > multiple times above) as the overhead of atomics is negligible compared to > > locks so the optimization of using the unsafe version isn't worth it while > > on the other hand not introducing this escape hatch is very much desired. > > Ping on this, looks like |endpoint|'s are refcounted from |endpoints_| and > always used behind |lock_|, can we once again make these RefCountedThreadSafe > and avoid introducing DisableSequenceConsistencyAssertions()? (keeping only > ScopedAllow is already much better than having two of these escape hatches) OK, I converted InterfaceEndPoint to RefCountedThreadSafe in a separate CL, and removed the per-instance suppression.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/30 09:05:16, tzik wrote: > On 2017/03/28 15:42:34, gab wrote: > > On 2017/03/24 22:06:46, chromium-reviews wrote: > > > Le ven. 24 mars 2017 12:58, > > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=yzshen@chromium.org> a > écrit > > : > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... > > > > File mojo/public/cpp/bindings/lib/multiplex_router.cc (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2666423002/diff/660001/mojo/public/cpp/bindin... > > > > mojo/public/cpp/bindings/lib/multiplex_router.cc:964: > > > > endpoint->DisableSequenceConsistencyAssertions(); > > > > On 2017/03/24 15:46:45, gab wrote: > > > > > That seems like a very large hammer, can we use the scoped object > > > > where relevant > > > > > instead? Otherwise how do we know all callers are truly doing the > > > > right thing > > > > > with the received pointer? I'd much rather not have > > > > > DisableSequenceConsistencyAssertions() if we can help it (and > > > > otherwise it > > > > > should have a large comment saying what it's there for and not to use > > > > it in new > > > > > use cases). > > > > > > > > > > > > I am the one who suggested that the check should be disabled at the > > > > object level, instead of using the scoped-allow object. :) > > > > > > > > That is because: > > > > - The scoped-allow object disables the check for *all* ref-counted > > > > objects in its scope. > > > > - When we want to disable the check for an object, it is a promise to > > > > behave correctly for all usage of that object across its lifetime. That > > > > is not a per-callsite choice. For example, all callsites should use the > > > > same lock to guard addref/release for that object. > > > > > > > > Considering these reasons, I don't think the scoped-allow approach is at > > > > the right level. > > > > > > > > > > Am I understanding correctly from your other reply that this object doesn't > > > use RefCountedThreadSafe because it's always used behind a lock? If so I'd > > > much prefer to make it RefCountedThreadSafe anyways (as I mentioned > > > multiple times above) as the overhead of atomics is negligible compared to > > > locks so the optimization of using the unsafe version isn't worth it while > > > on the other hand not introducing this escape hatch is very much desired. > > > > Ping on this, looks like |endpoint|'s are refcounted from |endpoints_| and > > always used behind |lock_|, can we once again make these RefCountedThreadSafe > > and avoid introducing DisableSequenceConsistencyAssertions()? (keeping only > > ScopedAllow is already much better than having two of these escape hatches) > > OK, I converted InterfaceEndPoint to RefCountedThreadSafe in a separate CL, and > removed the per-instance suppression. Awesome! LGTM :)! Thanks!
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, zmo@chromium.org, raymes@chromium.org, yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2666423002/#ps700001 (title: "remove DisableSequenceConsistencyAssertions")
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": 700001, "attempt_start_ts": 1490887944343840,
"parent_rev": "e618b6971a699f5d25d9f1b8c5d3011b8118feab", "commit_rev":
"f9a212dec0fec22e21d7f66ad1aa9414ed1dc068"}
Message was sent while issue was closed.
Description was changed from ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2666423002 Cr-Commit-Position: refs/heads/master@{#460773} Committed: https://chromium.googlesource.com/chromium/src/+/f9a212dec0fec22e21d7f66ad1aa... ==========
Message was sent while issue was closed.
Committed patchset #35 (id:700001) as https://chromium.googlesource.com/chromium/src/+/f9a212dec0fec22e21d7f66ad1aa...
Message was sent while issue was closed.
A revert of this CL (patchset #35 id:700001) has been created in https://codereview.chromium.org/2783393002/ by wjmaclean@chromium.org. The reason for reverting is: Speculative revert to see if this CL is the cause of the RenderWidgetHostViewChildFrameTest.ForwardsBeginFrameAcks failures on https://uberchromegw.corp.google.com/i/chromium.win/builders/Win%207%20Tests%....
Message was sent while issue was closed.
Description was changed from ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2666423002 Cr-Commit-Position: refs/heads/master@{#460773} Committed: https://chromium.googlesource.com/chromium/src/+/f9a212dec0fec22e21d7f66ad1aa... ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2666423002 Cr-Commit-Position: refs/heads/master@{#460773} Committed: https://chromium.googlesource.com/chromium/src/+/f9a212dec0fec22e21d7f66ad1aa... ==========
On 2017/03/30 19:51:55, wjmaclean wrote: > A revert of this CL (patchset #35 id:700001) has been created in > https://codereview.chromium.org/2783393002/ by mailto:wjmaclean@chromium.org. > > The reason for reverting is: Speculative revert to see if this CL is the cause > of the RenderWidgetHostViewChildFrameTest.ForwardsBeginFrameAcks failures on > https://uberchromegw.corp.google.com/i/chromium.win/builders/Win%207%20Tests%.... I think this CL doesn't break it, but just revealed a previously broken test, that should be fixed by http://crrev.com/2785883004 anyway. Let me reland this after I confirm the fix works.
The CQ bit was checked by tzik@chromium.org
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": 700001, "attempt_start_ts": 1490980003565110,
"parent_rev": "6f6d07f111d9df9bc70ff3dc71612d940d9b2d17", "commit_rev":
"3f4231f979090ad23fd89622b3b36ed564650645"}
Message was sent while issue was closed.
Description was changed from ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2666423002 Cr-Commit-Position: refs/heads/master@{#460773} Committed: https://chromium.googlesource.com/chromium/src/+/f9a212dec0fec22e21d7f66ad1aa... ========== to ========== Assert sequence validity on non-thread-safe RefCount manipulations This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy ref count manipulations. A ref counted object is considered to be bound to a sequence if the count is more than one. After this CL, ref count manipulations to a sequenced-bound one cause DCHECK failure. This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out, and applies the opt-out as needed. BUG=688072 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2666423002 Cr-Original-Commit-Position: refs/heads/master@{#460773} Committed: https://chromium.googlesource.com/chromium/src/+/f9a212dec0fec22e21d7f66ad1aa... Review-Url: https://codereview.chromium.org/2666423002 Cr-Commit-Position: refs/heads/master@{#461235} Committed: https://chromium.googlesource.com/chromium/src/+/3f4231f979090ad23fd89622b3b3... ==========
Message was sent while issue was closed.
Committed patchset #35 (id:700001) as https://chromium.googlesource.com/chromium/src/+/3f4231f979090ad23fd89622b3b3... |
