Use the saved task runner if TestTaskScheduler is re-entered from a task executed by RunTask.

Use the saved task runner if TestTaskScheduler is re-entered from a task executed by RunTask.

BUG=690504
Review-Url: https://codereview.chromium.org/2713513003
Cr-Commit-Position: refs/heads/master@{#452226}
Committed: https://chromium.googlesource.com/chromium/src/+/3c23304fa3f46c328792413ea3de60ea86e27fbd

[Joe Mason, 2017-02-22 00:07:43]

joenotcharles@chromium.org changed reviewers:
+ fdoray@chromium.org

[Joe Mason, 2017-02-22 00:07:43]

This fixes a problem in the ranker_model_loader_unittest.cc unit test in
https://codereview.chromium.org/2565873002/. I have no idea how to replicate it
so I didn't add a unit test, but inside FakeURLFetcher::Start,
MessageLoop::current() == message_loop_, so ClearTaskRunnerForTesting left
message_loop_->task_runner() returning null. Then the task that was passed to
RunTask executes and posts a task of its own, which causes a null pointer
reference.

[Joe Mason, 2017-02-22 00:09:27]

On 2017/02/22 00:07:43, Joe Mason wrote:
> This fixes a problem in the ranker_model_loader_unittest.cc unit test in
> https://codereview.chromium.org/2565873002/. I have no idea how to replicate
it
> so I didn't add a unit test, but inside FakeURLFetcher::Start,
> MessageLoop::current() == message_loop_, so ClearTaskRunnerForTesting left
> message_loop_->task_runner() returning null. Then the task that was passed to
> RunTask executes and posts a task of its own, which causes a null pointer
> reference.

While typing this, I figured out how to unit test it!

[fdoray, 2017-02-22 16:23:52]

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler.cc (right):

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:73: // Returns |saved_task_runner_| if called
from inside RunTask.
Returns the TaskRunner to which this TaskScheduler forwards tasks. It may be
|message_loop_->task_runner()| or a reference to it saved on entry to RunTask().

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:74: scoped_refptr<SingleThreadTaskRunner>
CurrentTaskRunner() const {
s/CurrentTaskRunner/MessageLoopTaskRunner/

"Current" make it sound like it changes often, which isn't the case. This will
return the same pointer, obtained from a different source, when called outside
or inside of RunTask().

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:88: // A copy of |message_loop_|'s
task_runner that is saved on entry to RunTask.
A reference to |message_loop_->task_runner()| saved on entry to RunTask(). This
is required because RunTask() overrides |message_loop_->task_runner()|.

Note: |message_loop_->task_runner()| is accessed directly outside of RunTask()
to guarantee that ScopedTaskScheduler always uses the latest TaskRunner set by
external code.

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:208: DCHECK(!saved_task_runner_);
Move these 2 lines before the comment. Add separate comment.

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:217: // Restore the MessageLoop TaskRunner.
DCHECK(!MessageLoop::current()->task_runner());

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler_unittest.cc (right):

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler_unittest.cc:299: 
// Verify that a task can be posted from a task running
// in ScopedTaskScheduler.

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler_unittest.cc:314: 
// Verify that ScopedTaskScheduler posts to the latest TaskRunner
// set on its MessageLoop, not the initial one.
TEST(ScopedTaskSchedulerTest, PostToCurrentMessageLoopTaskRunner) {
  bool task_ran = false;
  ScopedTaskScheduler scoped_task_scheduler;
  auto deferred_task_runner = make_scoped_ref_ptr(new
DeferredSequencedTaskRunner(base::ThreadTaskRunnerHandle::Get()));
  MessageLoop::current()->SetTaskRunner(deferred_task_runner);
  PostTask(
    FROM_HERE,
    Bind([] (bool* task_ran) { *task_ran = true; },
         Unretained(task_ran)));

  // The task shouldn't run if it was posted to
  // |deferred_task_runner| instead of the initial
  // MessageLoop's TaskRunner.
  RunLoop().RunUntilIdle();
  EXPECT_FALSE(task_ran);

  deferred_task_runner->Start();
  RunLoop().RunUntilIdle();
  EXPECT_TRUE(task_ran);     
}

[Joe Mason, 2017-02-22 20:23:31]

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler.cc (right):

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:73: // Returns |saved_task_runner_| if called
from inside RunTask.
On 2017/02/22 16:23:52, fdoray wrote:
> Returns the TaskRunner to which this TaskScheduler forwards tasks. It may be
> |message_loop_->task_runner()| or a reference to it saved on entry to
RunTask().

Done.

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:74: scoped_refptr<SingleThreadTaskRunner>
CurrentTaskRunner() const {
On 2017/02/22 16:23:52, fdoray wrote:
> s/CurrentTaskRunner/MessageLoopTaskRunner/
> 
> "Current" make it sound like it changes often, which isn't the case. This will
> return the same pointer, obtained from a different source, when called outside
> or inside of RunTask().

Done.

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:88: // A copy of |message_loop_|'s
task_runner that is saved on entry to RunTask.
On 2017/02/22 16:23:51, fdoray wrote:
> A reference to |message_loop_->task_runner()| saved on entry to RunTask().
This
> is required because RunTask() overrides |message_loop_->task_runner()|.
> 
> Note: |message_loop_->task_runner()| is accessed directly outside of RunTask()
> to guarantee that ScopedTaskScheduler always uses the latest TaskRunner set by
> external code.

Done.

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:208: DCHECK(!saved_task_runner_);
On 2017/02/22 16:23:51, fdoray wrote:
> Move these 2 lines before the comment. Add separate comment.

Done.

I don't think it needs a comment because assigning to "saved_task_runner_" is
self-documenting.

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler.cc:217: // Restore the MessageLoop TaskRunner.
On 2017/02/22 16:23:52, fdoray wrote:
> DCHECK(!MessageLoop::current()->task_runner());

Done.

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
File base/test/scoped_task_scheduler_unittest.cc (right):

https://codereview.chromium.org/2713513003/diff/20001/base/test/scoped_task_s...
base/test/scoped_task_scheduler_unittest.cc:314: 
On 2017/02/22 16:23:52, fdoray wrote:
> // Verify that ScopedTaskScheduler posts to the latest TaskRunner
> // set on its MessageLoop, not the initial one.
> TEST(ScopedTaskSchedulerTest, PostToCurrentMessageLoopTaskRunner) {
>   bool task_ran = false;
>   ScopedTaskScheduler scoped_task_scheduler;
>   auto deferred_task_runner = make_scoped_ref_ptr(new
> DeferredSequencedTaskRunner(base::ThreadTaskRunnerHandle::Get()));
>   MessageLoop::current()->SetTaskRunner(deferred_task_runner);
>   PostTask(
>     FROM_HERE,
>     Bind([] (bool* task_ran) { *task_ran = true; },
>          Unretained(task_ran)));
> 
>   // The task shouldn't run if it was posted to
>   // |deferred_task_runner| instead of the initial
>   // MessageLoop's TaskRunner.
>   RunLoop().RunUntilIdle();
>   EXPECT_FALSE(task_ran);
> 
>   deferred_task_runner->Start();
>   RunLoop().RunUntilIdle();
>   EXPECT_TRUE(task_ran);     
> }

This test does the same thing as ReassignCurrentTaskRunner, except using
MessageLoop::SetTaskRunner directly instead of leveraging
ScopedMockTimeMessageLoopTaskRunner.

[fdoray, 2017-02-22 20:36:35]

lgtm

[Joe Mason, 2017-02-22 20:41:18]

The CQ bit was checked by joenotcharles@chromium.org

[commit-bot: I haz the power, 2017-02-22 20:42:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 22:07:20]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487796078574240,
"parent_rev": "d92113de572fd2b76fb9185a715833fd79b896f7", "commit_rev":
"3c23304fa3f46c328792413ea3de60ea86e27fbd"}

[commit-bot: I haz the power, 2017-02-22 22:07:57]

Description was changed from

==========
Use the saved task runner if TestTaskScheduler is re-entered from a task
executed by RunTask.

BUG=690504
==========

to

==========
Use the saved task runner if TestTaskScheduler is re-entered from a task
executed by RunTask.

BUG=690504

Review-Url: https://codereview.chromium.org/2713513003
Cr-Commit-Position: refs/heads/master@{#452226}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c23304fa3f46c328792413ea3de...
==========

[commit-bot: I haz the power, 2017-02-22 22:07:58]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3c23304fa3f46c328792413ea3de...