Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2729573004:
Fix TaskSchedulerWorkerPoolHistogramTest.NumTasksBeforeDetach (Closed)
DescriptionFix TaskSchedulerWorkerPoolHistogramTest.NumTasksBeforeDetach
This fix passes the pointer for the PlatformThreadRef to the task for
comparisons and actually gets the PlatformThreadRef from
PlatformThread::CurrentRef().
Previously, the test would race and pass by value. This meant that
if the previous task did not complete in time, it would pass the empty
reference. Since we were accidentally comparing against that, then the
test succeeded. When code ran slowly enough to set the value before
the next PostTask, then we would have the correct PlatformThreadRef,
but incorrectly compare it to the empty PlatformThreadRef, correctly
failing the test.
It's amazing that the tests passed in the first place!
BUG=697697, 698046
Review-Url: https://codereview.chromium.org/2729573004
Cr-Commit-Position: refs/heads/master@{#456558}
Committed: https://chromium.googlesource.com/chromium/src/+/404de72480129c6549041267a171a9b0f1f8189f
Patch Set 1 #Patch Set 2 : Add Assertions Against Null PlatformThreadRefs #
Total comments: 2
Messages
Total messages: 25 (18 generated)
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
The CQ bit was checked by robliao@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 robliao@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.
lgtm
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Added some assertions againsts null PlatformThreadRefs
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 robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2729573004/#ps20001 (title: "Add Assertions Against Null PlatformThreadRefs")
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": 20001, "attempt_start_ts": 1489448778586780,
"parent_rev": "6782567de4ad3aadd41740db62fbd4123b7378b6", "commit_rev":
"404de72480129c6549041267a171a9b0f1f8189f"}
Message was sent while issue was closed.
Description was changed from ========== Fix TaskSchedulerWorkerPoolHistogramTest.NumTasksBeforeDetach This fix passes the pointer for the PlatformThreadRef to the task for comparisons and actually gets the PlatformThreadRef from PlatformThread::CurrentRef(). Previously, the test would race and pass by value. This meant that if the previous task did not complete in time, it would pass the empty reference. Since we were accidentally comparing against that, then the test succeeded. When code ran slowly enough to set the value before the next PostTask, then we would have the correct PlatformThreadRef, but incorrectly compare it to the empty PlatformThreadRef, correctly failing the test. It's amazing that the tests passed in the first place! BUG=697697 ========== to ========== Fix TaskSchedulerWorkerPoolHistogramTest.NumTasksBeforeDetach This fix passes the pointer for the PlatformThreadRef to the task for comparisons and actually gets the PlatformThreadRef from PlatformThread::CurrentRef(). Previously, the test would race and pass by value. This meant that if the previous task did not complete in time, it would pass the empty reference. Since we were accidentally comparing against that, then the test succeeded. When code ran slowly enough to set the value before the next PostTask, then we would have the correct PlatformThreadRef, but incorrectly compare it to the empty PlatformThreadRef, correctly failing the test. It's amazing that the tests passed in the first place! BUG=697697 Review-Url: https://codereview.chromium.org/2729573004 Cr-Commit-Position: refs/heads/master@{#456558} Committed: https://chromium.googlesource.com/chromium/src/+/404de72480129c6549041267a171... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/404de72480129c6549041267a171...
Message was sent while issue was closed.
lgtm % nit https://codereview.chromium.org/2729573004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2729573004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:722: // TODO(crbug.com/698046): disabled due to flakyness. rm comment
Message was sent while issue was closed.
Description was changed from ========== Fix TaskSchedulerWorkerPoolHistogramTest.NumTasksBeforeDetach This fix passes the pointer for the PlatformThreadRef to the task for comparisons and actually gets the PlatformThreadRef from PlatformThread::CurrentRef(). Previously, the test would race and pass by value. This meant that if the previous task did not complete in time, it would pass the empty reference. Since we were accidentally comparing against that, then the test succeeded. When code ran slowly enough to set the value before the next PostTask, then we would have the correct PlatformThreadRef, but incorrectly compare it to the empty PlatformThreadRef, correctly failing the test. It's amazing that the tests passed in the first place! BUG=697697 Review-Url: https://codereview.chromium.org/2729573004 Cr-Commit-Position: refs/heads/master@{#456558} Committed: https://chromium.googlesource.com/chromium/src/+/404de72480129c6549041267a171... ========== to ========== Fix TaskSchedulerWorkerPoolHistogramTest.NumTasksBeforeDetach This fix passes the pointer for the PlatformThreadRef to the task for comparisons and actually gets the PlatformThreadRef from PlatformThread::CurrentRef(). Previously, the test would race and pass by value. This meant that if the previous task did not complete in time, it would pass the empty reference. Since we were accidentally comparing against that, then the test succeeded. When code ran slowly enough to set the value before the next PostTask, then we would have the correct PlatformThreadRef, but incorrectly compare it to the empty PlatformThreadRef, correctly failing the test. It's amazing that the tests passed in the first place! BUG=697697, 698046 Review-Url: https://codereview.chromium.org/2729573004 Cr-Commit-Position: refs/heads/master@{#456558} Committed: https://chromium.googlesource.com/chromium/src/+/404de72480129c6549041267a171... ==========
Message was sent while issue was closed.
https://codereview.chromium.org/2729573004/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl_unittest.cc (right): https://codereview.chromium.org/2729573004/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl_unittest.cc:722: // TODO(crbug.com/698046): disabled due to flakyness. On 2017/03/15 19:23:11, gab wrote: > rm comment Done in https://codereview.chromium.org/2750103004/ |
