|
|
Created:
4 years, 6 months ago by Sergey Ulanov Modified:
4 years, 5 months ago CC:
chromium-reviews, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@move_closure_runner Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix incorrect usage of ScopedCallbackRunner.Reset().
ScopedCallbackRunner.Reset() was used in two places with
assumption it doesn't call current callback. Replaced Reset()
with ReplaceClosure() and RunAndReset() to make behavior clearer.
Committed: https://crrev.com/668613aa8ef226fd280cbd686654bad521222422
Cr-Commit-Position: refs/heads/master@{#404277}
Patch Set 1 #
Total comments: 2
Patch Set 2 : renamed Reset #Patch Set 3 : . #Patch Set 4 : RunNow #
Total comments: 6
Patch Set 5 : address feedback #Patch Set 6 : RunNow in destructor #Patch Set 7 : revert to PatchSet 5 #
Total comments: 2
Patch Set 8 : RunAndReset #
Total comments: 2
Patch Set 9 : . #
Messages
Total messages: 57 (27 generated)
sergeyu@chromium.org changed reviewers: + danakj@chromium.org
IMO it's best to get rid of ScopedClosureRunner::Reset() to avoid issues like this. This was the only place where it's used. WDYT?
https://codereview.chromium.org/2094803005/diff/1/content/browser/compositor/... File content/browser/compositor/surface_utils.cc (left): https://codereview.chromium.org/2094803005/diff/1/content/browser/compositor/... content/browser/compositor/surface_utils.cc:79: scoped_callback_runner.Reset(base::Bind( Wait this actually want to call two callbacks, with FAILED and ALLOCATION_FAILURE? I suspect you can instead make Reset not call the old callback, and then make operator= do likewise. WDYT?
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by sergeyu@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 ========== Fix PrepareTextureCopyOutputResult to avoid ScopedCallbackRunner. PrepareTextureCopyOutputResult was using ScopedCallbackRunner which was causing it to call provided callback twice in some cases. ========== to ========== Fix incorrect usage of ScopedCallbackRunner.Reset(). ScopedCallbackRunner.Reset() was used in two places with assumption it doesn't call current callback. Replaced Reset() with ReplaceClosure() to make behavior clearer and match expected behavior. ==========
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by sergeyu@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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by sergeyu@chromium.org
The CQ bit was unchecked by sergeyu@chromium.org
The CQ bit was checked by sergeyu@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_ozone_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 sergeyu@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 ========== Fix incorrect usage of ScopedCallbackRunner.Reset(). ScopedCallbackRunner.Reset() was used in two places with assumption it doesn't call current callback. Replaced Reset() with ReplaceClosure() to make behavior clearer and match expected behavior. ========== to ========== Fix incorrect usage of ScopedCallbackRunner.Reset(). ScopedCallbackRunner.Reset() was used in two places with assumption it doesn't call current callback. Replaced Reset() with ReplaceClosure() and RunNow() to make behavior clearer. ==========
PTAL. Replaced Reset() with RunNow() and ReplaceClosure() now. https://codereview.chromium.org/2094803005/diff/1/content/browser/compositor/... File content/browser/compositor/surface_utils.cc (left): https://codereview.chromium.org/2094803005/diff/1/content/browser/compositor/... content/browser/compositor/surface_utils.cc:79: scoped_callback_runner.Reset(base::Bind( On 2016/06/24 23:57:27, danakj wrote: > Wait this actually want to call two callbacks, with FAILED and > ALLOCATION_FAILURE? Are you saying that the old behavior is correct? That would be strange if that |callback| was supposed to be called twice. Single operation fails once, it cannot fail multiple times > I suspect you can instead make Reset not call the old callback, and then make > operator= do likewise. WDYT? Also renamed Reset(closure) -> ReplaceClosure(closure) and Reset() -> RunNow().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2094803005/diff/100001/base/callback_helpers.h File base/callback_helpers.h (right): https://codereview.chromium.org/2094803005/diff/100001/base/callback_helpers.... base/callback_helpers.h:40: // Releases the current closure if it's set and replaces it with the closure It dchecks there's no closure, so this should say it can only be called on an empty ScopedClosureRunner. However, I think it'd also be okay to make it do the same thing as ReplaceClosure and drop the DCHECK. https://codereview.chromium.org/2094803005/diff/100001/base/callback_helpers.... base/callback_helpers.h:47: // Replaces closure with the new one releasing the old one without calling it. The comment says it won't run it but the code does run it still. https://codereview.chromium.org/2094803005/diff/100001/base/callback_helpers_... File base/callback_helpers_unittest.cc (right): https://codereview.chromium.org/2094803005/diff/100001/base/callback_helpers_... base/callback_helpers_unittest.cc:46: EXPECT_EQ(1, run_count_1); won't it be 0 now?
https://codereview.chromium.org/2094803005/diff/100001/base/callback_helpers.h File base/callback_helpers.h (right): https://codereview.chromium.org/2094803005/diff/100001/base/callback_helpers.... base/callback_helpers.h:40: // Releases the current closure if it's set and replaces it with the closure On 2016/06/27 22:49:57, danakj wrote: > It dchecks there's no closure, so this should say it can only be called on an > empty ScopedClosureRunner. However, I think it'd also be okay to make it do the > same thing as ReplaceClosure and drop the DCHECK. Done. https://codereview.chromium.org/2094803005/diff/100001/base/callback_helpers.... base/callback_helpers.h:47: // Replaces closure with the new one releasing the old one without calling it. On 2016/06/27 22:49:57, danakj wrote: > The comment says it won't run it but the code does run it still. Duh, yes, sorry https://codereview.chromium.org/2094803005/diff/100001/base/callback_helpers_... File base/callback_helpers_unittest.cc (right): https://codereview.chromium.org/2094803005/diff/100001/base/callback_helpers_... base/callback_helpers_unittest.cc:46: EXPECT_EQ(1, run_count_1); On 2016/06/27 22:49:57, danakj wrote: > won't it be 0 now? Done.
Also now changed the destructor to call RunNow() instead of calling closure_ directly. RunNow() moves the closure to the stack, which is safer when the ScopedCallbackCaller is allocated on the heap.
On 2016/06/28 19:20:21, Sergey Ulanov wrote: > Also now changed the destructor to call RunNow() instead of calling closure_ > directly. RunNow() moves the closure to the stack, which is safer when the > ScopedCallbackCaller is allocated on the heap. Actually I think it shouldn't matter here since it's a destructor, so I reverted it back to what it was.
The CQ bit was checked by sergeyu@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM https://codereview.chromium.org/2094803005/diff/160001/base/callback_helpers.h File base/callback_helpers.h (right): https://codereview.chromium.org/2094803005/diff/160001/base/callback_helpers.... base/callback_helpers.h:45: void RunNow(); nit: I think just "Run" would work here, or "RunAndReset".
Description was changed from ========== Fix incorrect usage of ScopedCallbackRunner.Reset(). ScopedCallbackRunner.Reset() was used in two places with assumption it doesn't call current callback. Replaced Reset() with ReplaceClosure() and RunNow() to make behavior clearer. ========== to ========== Fix incorrect usage of ScopedCallbackRunner.Reset(). ScopedCallbackRunner.Reset() was used in two places with assumption it doesn't call current callback. Replaced Reset() with ReplaceClosure() and RunAndReset() to make behavior clearer. ==========
sergeyu@chromium.org changed reviewers: + robertshield@chromium.org, sievers@chromium.org
+sievers@chromium.org for content/browser/renderer_host/render_widget_host_view_android.cc +robertshield@chromium.org for chrome/browser/install_verification/win/module_list_unittest.cc https://codereview.chromium.org/2094803005/diff/160001/base/callback_helpers.h File base/callback_helpers.h (right): https://codereview.chromium.org/2094803005/diff/160001/base/callback_helpers.... base/callback_helpers.h:45: void RunNow(); On 2016/06/28 20:02:15, danakj wrote: > nit: I think just "Run" would work here, or "RunAndReset". Done.
chrome/browser/install_verification/win/module_list_unittest.cc LGTM
sievers@: ping
lgtm https://codereview.chromium.org/2094803005/diff/180001/content/browser/compos... File content/browser/compositor/surface_utils.cc (right): https://codereview.chromium.org/2094803005/diff/180001/content/browser/compos... content/browser/compositor/surface_utils.cc:79: scoped_callback_runner.ReplaceClosure(base::Bind( Ah so |callback| ended up getting called twice?
https://codereview.chromium.org/2094803005/diff/180001/content/browser/compos... File content/browser/compositor/surface_utils.cc (right): https://codereview.chromium.org/2094803005/diff/180001/content/browser/compos... content/browser/compositor/surface_utils.cc:79: scoped_callback_runner.ReplaceClosure(base::Bind( On 2016/07/06 19:12:13, sievers wrote: > Ah so |callback| ended up getting called twice? Correct. Previously Reset() was calling the old callback.
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2094803005/#ps180001 (title: "RunAndReset")
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
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 sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, robertshield@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2094803005/#ps200001 (title: ".")
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
Try jobs failed on following builders: mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix incorrect usage of ScopedCallbackRunner.Reset(). ScopedCallbackRunner.Reset() was used in two places with assumption it doesn't call current callback. Replaced Reset() with ReplaceClosure() and RunAndReset() to make behavior clearer. ========== to ========== Fix incorrect usage of ScopedCallbackRunner.Reset(). ScopedCallbackRunner.Reset() was used in two places with assumption it doesn't call current callback. Replaced Reset() with ReplaceClosure() and RunAndReset() to make behavior clearer. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Fix incorrect usage of ScopedCallbackRunner.Reset(). ScopedCallbackRunner.Reset() was used in two places with assumption it doesn't call current callback. Replaced Reset() with ReplaceClosure() and RunAndReset() to make behavior clearer. ========== to ========== Fix incorrect usage of ScopedCallbackRunner.Reset(). ScopedCallbackRunner.Reset() was used in two places with assumption it doesn't call current callback. Replaced Reset() with ReplaceClosure() and RunAndReset() to make behavior clearer. Committed: https://crrev.com/668613aa8ef226fd280cbd686654bad521222422 Cr-Commit-Position: refs/heads/master@{#404277} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/668613aa8ef226fd280cbd686654bad521222422 Cr-Commit-Position: refs/heads/master@{#404277} |