Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697843002:
Add ScopedTaskScheduler in URLRequestDataJobFuzzerHarness. (Closed)
DescriptionAdd ScopedTaskScheduler in URLRequestDataJobFuzzerHarness.
net::URLRequestSimpleJob uses the base/task_scheduler/post_task.h
and thus can only be used when there is a registered TaskScheduler.
Verified using reproducer testcase that this fixes issue
crbug.com/691750.
BUG=691750
Review-Url: https://codereview.chromium.org/2697843002
Cr-Commit-Position: refs/heads/master@{#450971}
Committed: https://chromium.googlesource.com/chromium/src/+/f2647997874eead6de0e93308011b55b6ae9160d
Patch Set 1 #
Total comments: 5
Patch Set 2 : no TaskRunner #
Total comments: 3
Patch Set 3 : CR xunjieli #8 #Messages
Total messages: 18 (8 generated)
Description was changed from ========== Add ScopedTaskScheduler in URLRequestDataJobFuzzerHarness. net::URLRequestSimpleJob uses the base/task_scheduler/post_task.h and thus can only be used when there is a registered TaskScheduler. BUG=691750 ========== to ========== Add ScopedTaskScheduler in URLRequestDataJobFuzzerHarness. net::URLRequestSimpleJob uses the base/task_scheduler/post_task.h and thus can only be used when there is a registered TaskScheduler. Verified using reproducer testcase that this fixes issue crbug.com/691750. BUG=691750 ==========
fdoray@chromium.org changed reviewers: + xunjieli@chromium.org
PTAL
https://codereview.chromium.org/2697843002/diff/1/net/url_request/url_request... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/2697843002/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:163: base::test::ScopedTaskScheduler scoped_task_scheduler_; The requirement that every subclass needs a ScopedTaskScheduler is not intuitive. Can you make URLRequestSimpleJob have a ScopedTaskScheduler instead? It's clearer for the actual call places to have the ScopedTaskScheduler.
PTAnL https://codereview.chromium.org/2697843002/diff/1/net/url_request/url_request... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/2697843002/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:163: base::test::ScopedTaskScheduler scoped_task_scheduler_; On 2017/02/14 16:17:37, xunjieli wrote: > The requirement that every subclass needs a ScopedTaskScheduler is not > intuitive. Can you make URLRequestSimpleJob have a ScopedTaskScheduler instead? > It's clearer for the actual call places to have the ScopedTaskScheduler. A TaskScheduler must be registered in the current process to use a function from base/task_scheduler/post_task.h (cf. https://cs.chromium.org/chromium/src/base/task_scheduler/post_task.h?l=26). ScopedTaskScheduler is a convenient way to register a TaskScheduler within a scope. This is very similar to how MessageLoop works: you need to initialize a MessageLoop in your test if it uses code that calls MessageLoop::current(). We can't add a base::test::ScopedTaskScheduler in URLRequestSimpleJob because: - It can only be used in tests. - There can only be one active TaskScheduler per process.
https://codereview.chromium.org/2697843002/diff/1/net/url_request/url_request... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/2697843002/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:34: : task_runner_(base::ThreadTaskRunnerHandle::Get()), Is the |task_runner_| still needed? Can you post the task to the task scheduler? https://codereview.chromium.org/2697843002/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:163: base::test::ScopedTaskScheduler scoped_task_scheduler_; On 2017/02/14 17:44:10, fdoray wrote: > On 2017/02/14 16:17:37, xunjieli wrote: > > The requirement that every subclass needs a ScopedTaskScheduler is not > > intuitive. Can you make URLRequestSimpleJob have a ScopedTaskScheduler > instead? > > It's clearer for the actual call places to have the ScopedTaskScheduler. > > A TaskScheduler must be registered in the current process to use a function from > base/task_scheduler/post_task.h (cf. > https://cs.chromium.org/chromium/src/base/task_scheduler/post_task.h?l=26). > ScopedTaskScheduler is a convenient way to register a TaskScheduler within a > scope. This is very similar to how MessageLoop works: you need to initialize a > MessageLoop in your test if it uses code that calls MessageLoop::current(). > > We can't add a base::test::ScopedTaskScheduler in URLRequestSimpleJob because: > - It can only be used in tests. > - There can only be one active TaskScheduler per process. Acknowledged.
PTAnL https://codereview.chromium.org/2697843002/diff/1/net/url_request/url_request... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/2697843002/diff/1/net/url_request/url_request... net/url_request/url_request_data_job_fuzzer.cc:34: : task_runner_(base::ThreadTaskRunnerHandle::Get()), On 2017/02/14 17:51:59, xunjieli wrote: > Is the |task_runner_| still needed? > Can you post the task to the task scheduler? In fact, we don't even need to post a task in QuitLoop(). We can call read_loop_->QuitWhenIdle() directly. Posting a task is useful to quit a loop running on a different thread, but this fuzzer is single-threaded. https://codereview.chromium.org/2697843002/diff/20001/net/url_request/url_req... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/2697843002/diff/20001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:99: read_loop_->QuitWhenIdle(); QuitWhenIdle() quits the loop when there are no more pending tasks. Alternatively, we could use Quit() to quit immediately. I prefer QuitWhenIdle() because it prevents potential errors from being skipped.
https://codereview.chromium.org/2697843002/diff/20001/net/url_request/url_req... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/2697843002/diff/20001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:99: read_loop_->QuitWhenIdle(); On 2017/02/14 18:24:30, fdoray wrote: > QuitWhenIdle() quits the loop when there are no more pending tasks. > Alternatively, we could use Quit() to quit immediately. > > I prefer QuitWhenIdle() because it prevents potential errors from being skipped. QuitWhenIdle() can be flaky in some cases. I know this fuzzer is only run on a single thread, so QuitWhenIdle() might be equivalent to the original code. However, I prefer if you post read_loop_->QuitClosure() to the task scheduler, so the original logic is kept. Is that feasible?
The CQ bit was checked by fdoray@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...
PTAnL https://codereview.chromium.org/2697843002/diff/20001/net/url_request/url_req... File net/url_request/url_request_data_job_fuzzer.cc (right): https://codereview.chromium.org/2697843002/diff/20001/net/url_request/url_req... net/url_request/url_request_data_job_fuzzer.cc:99: read_loop_->QuitWhenIdle(); On 2017/02/14 18:55:31, xunjieli wrote: > On 2017/02/14 18:24:30, fdoray wrote: > > QuitWhenIdle() quits the loop when there are no more pending tasks. > > Alternatively, we could use Quit() to quit immediately. > > > > I prefer QuitWhenIdle() because it prevents potential errors from being > skipped. > > QuitWhenIdle() can be flaky in some cases. I know this fuzzer is only run on a > single thread, so QuitWhenIdle() might be equivalent to the original code. > > However, I prefer if you post read_loop_->QuitClosure() to the task scheduler, > so the original logic is kept. > > Is that feasible? Done.
lgtm
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@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": 40001, "attempt_start_ts": 1487258270062310,
"parent_rev": "2d145fe56a1d995fc1a016da03c1f91f217612e0", "commit_rev":
"f2647997874eead6de0e93308011b55b6ae9160d"}
Message was sent while issue was closed.
Description was changed from ========== Add ScopedTaskScheduler in URLRequestDataJobFuzzerHarness. net::URLRequestSimpleJob uses the base/task_scheduler/post_task.h and thus can only be used when there is a registered TaskScheduler. Verified using reproducer testcase that this fixes issue crbug.com/691750. BUG=691750 ========== to ========== Add ScopedTaskScheduler in URLRequestDataJobFuzzerHarness. net::URLRequestSimpleJob uses the base/task_scheduler/post_task.h and thus can only be used when there is a registered TaskScheduler. Verified using reproducer testcase that this fixes issue crbug.com/691750. BUG=691750 Review-Url: https://codereview.chromium.org/2697843002 Cr-Commit-Position: refs/heads/master@{#450971} Committed: https://chromium.googlesource.com/chromium/src/+/f2647997874eead6de0e93308011... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f2647997874eead6de0e93308011... |
