Fix null TaskScheduler dereference introduced by codereview.chromium.org/2608093002

Fix null TaskScheduler dereference introduced by codereview.chromium.org/2608093002

Above changelog caused Cronet (by calling `PostRead()`) to call `PostDelayedTaskWithTraits()`,
which calls `TaskScheduler::GetInstance()`, dereferencing the global `TaskScheduler` pointer
`g_task_scheduler`, which was null because the CL did not add any call to set this pointer in
Cronet's execution path.

Fixed by adding a call to `TaskScheduler::CreateAndSetSimpleTaskSceduler()` to
the beginning `CronetEnvironment::Initialize()`.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2639673002
Cr-Commit-Position: refs/heads/master@{#444368}
Committed: https://chromium.googlesource.com/chromium/src/+/8c4ab790c6018a4501ed7bc02fa1b22bbf293db0

[lilyhoughton1, 2017-01-17 19:17:00]

Description was changed from

==========
Fix null TaskScheduler dereference introduced by
codereview.chromium.org/2608093002

Above changelog caused Cronet (by calling `PostRead()`) to call
`PostDelayedTaskWithTraits()`,
which calls `TaskScheduler::GetInstance()`, dereferencing the global
`TaskScheduler` pointer
`g_task_scheduler`, which was null because the CL did not add any call to set
this pointer in
Cronet's execution path.

Fixed by adding a call to `TaskScheduler::CreateAndSetSimpleTaskSceduler()` to
the beginning `CronetEnvironment::Initialize()`.
==========

to

==========
Fix null TaskScheduler dereference introduced by
codereview.chromium.org/2608093002

Above changelog caused Cronet (by calling `PostRead()`) to call
`PostDelayedTaskWithTraits()`,
which calls `TaskScheduler::GetInstance()`, dereferencing the global
`TaskScheduler` pointer
`g_task_scheduler`, which was null because the CL did not add any call to set
this pointer in
Cronet's execution path.

Fixed by adding a call to `TaskScheduler::CreateAndSetSimpleTaskSceduler()` to
the beginning `CronetEnvironment::Initialize()`.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[lilyhoughton, 2017-01-17 19:19:34]

Description was changed from

==========
Fix null TaskScheduler dereference introduced by
codereview.chromium.org/2608093002

Above changelog caused Cronet (by calling `PostRead()`) to call
`PostDelayedTaskWithTraits()`,
which calls `TaskScheduler::GetInstance()`, dereferencing the global
`TaskScheduler` pointer
`g_task_scheduler`, which was null because the CL did not add any call to set
this pointer in
Cronet's execution path.

Fixed by adding a call to `TaskScheduler::CreateAndSetSimpleTaskSceduler()` to
the beginning `CronetEnvironment::Initialize()`.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Fix null TaskScheduler dereference introduced by
codereview.chromium.org/2608093002

Above changelog caused Cronet (by calling `PostRead()`) to call
`PostDelayedTaskWithTraits()`,
which calls `TaskScheduler::GetInstance()`, dereferencing the global
`TaskScheduler` pointer
`g_task_scheduler`, which was null because the CL did not add any call to set
this pointer in
Cronet's execution path.

Fixed by adding a call to `TaskScheduler::CreateAndSetSimpleTaskSceduler()` to
the beginning `CronetEnvironment::Initialize()`.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[lilyhoughton, 2017-01-17 19:19:34]

lilyhoughton@chromium.org changed reviewers:
+ fdoray@chromium.org, mef@chromium.org

[lilyhoughton, 2017-01-17 19:21:15]

PTAL; in particular, I'm not familiar enough with the control flow to know that
this is the best place for the `SetTaskScheduler` call.

[lilyhoughton, 2017-01-17 19:27:34]

lilyhoughton@chromium.org changed reviewers:
+ gavinp@chromium.org

[fdoray, 2017-01-17 19:34:50]

On 2017/01/17 19:21:15, lilyhoughton wrote:
> PTAL; in particular, I'm not familiar enough with the control flow to know
that
> this is the best place for the `SetTaskScheduler` call.

TaskScheduler must be initialized once per process, early in its lifetime.

Questions:
- Could you add TaskScheduler:GetInstance()->Shutdown() somewhere before the
process exits? This would ensure that all tasks posted with
TaskShutdownBehavior::BLOCK_SHUTDOWN run before the process exits. I know that
it's not always possible to call this on mobile (process may exit at any time).
- Is CronetEnvironment::Initialize() used in Chrome itself, or in another
program?

[lilyhoughton, 2017-01-17 19:49:43]

On 2017/01/17 19:34:50, fdoray wrote:
> On 2017/01/17 19:21:15, lilyhoughton wrote:
> > PTAL; in particular, I'm not familiar enough with the control flow to know
> that
> > this is the best place for the `SetTaskScheduler` call.
> 
> TaskScheduler must be initialized once per process, early in its lifetime.

CronetEnvironment::Initialize() seems like a good place for it then.

> Questions:
> - Could you add TaskScheduler:GetInstance()->Shutdown() somewhere before the
> process exits? This would ensure that all tasks posted with
> TaskShutdownBehavior::BLOCK_SHUTDOWN run before the process exits. I know that
> it's not always possible to call this on mobile (process may exit at any
time).

I added a call to Shutdown() in CronetEnvironment's destructor, which should
satisfy
this requirement for now.

[fdoray, 2017-01-17 19:53:00]

On 2017/01/17 19:49:43, lilyhoughton wrote:
> On 2017/01/17 19:34:50, fdoray wrote:
> > On 2017/01/17 19:21:15, lilyhoughton wrote:
> > > PTAL; in particular, I'm not familiar enough with the control flow to know
> > that
> > > this is the best place for the `SetTaskScheduler` call.
> > 
> > TaskScheduler must be initialized once per process, early in its lifetime.
> 
> CronetEnvironment::Initialize() seems like a good place for it then.
> 
> > Questions:
> > - Could you add TaskScheduler:GetInstance()->Shutdown() somewhere before the
> > process exits? This would ensure that all tasks posted with
> > TaskShutdownBehavior::BLOCK_SHUTDOWN run before the process exits. I know
that
> > it's not always possible to call this on mobile (process may exit at any
> time).
> 
> I added a call to Shutdown() in CronetEnvironment's destructor, which should
> satisfy
> this requirement for now.

lgtm

Where is CronetEnvironment used? In Chrome or in another standalone program?

[mef, 2017-01-17 20:15:25]

On 2017/01/17 19:53:00, fdoray wrote:
> On 2017/01/17 19:49:43, lilyhoughton wrote:
> > On 2017/01/17 19:34:50, fdoray wrote:
> > > On 2017/01/17 19:21:15, lilyhoughton wrote:
> > > > PTAL; in particular, I'm not familiar enough with the control flow to
know
> > > that
> > > > this is the best place for the `SetTaskScheduler` call.
> > > 
> > > TaskScheduler must be initialized once per process, early in its lifetime.
> > 
> > CronetEnvironment::Initialize() seems like a good place for it then.
> > 
> > > Questions:
> > > - Could you add TaskScheduler:GetInstance()->Shutdown() somewhere before
the
> > > process exits? This would ensure that all tasks posted with
> > > TaskShutdownBehavior::BLOCK_SHUTDOWN run before the process exits. I know
> that
> > > it's not always possible to call this on mobile (process may exit at any
> > time).
> > 
> > I added a call to Shutdown() in CronetEnvironment's destructor, which should
> > satisfy
> > this requirement for now.
> 
> lgtm
> 
> Where is CronetEnvironment used? In Chrome or in another standalone program?

lgtm. Cronet is Chromium net stacked packaged as a library and used by other
apps. 
CronetEnvironment is used in Cronet.mm to provide ObjC API for use in other
programs.

[lilyhoughton, 2017-01-18 15:02:46]

The CQ bit was checked by lilyhoughton@chromium.org

[commit-bot: I haz the power, 2017-01-18 15:03:11]

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-01-18 15:34:18]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484751766408570,
"parent_rev": "1dbd7cc07bef48cc80ea7667fc9724a6b9500f29", "commit_rev":
"8c4ab790c6018a4501ed7bc02fa1b22bbf293db0"}

[commit-bot: I haz the power, 2017-01-18 15:34:46]

Description was changed from

==========
Fix null TaskScheduler dereference introduced by
codereview.chromium.org/2608093002

Above changelog caused Cronet (by calling `PostRead()`) to call
`PostDelayedTaskWithTraits()`,
which calls `TaskScheduler::GetInstance()`, dereferencing the global
`TaskScheduler` pointer
`g_task_scheduler`, which was null because the CL did not add any call to set
this pointer in
Cronet's execution path.

Fixed by adding a call to `TaskScheduler::CreateAndSetSimpleTaskSceduler()` to
the beginning `CronetEnvironment::Initialize()`.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Fix null TaskScheduler dereference introduced by
codereview.chromium.org/2608093002

Above changelog caused Cronet (by calling `PostRead()`) to call
`PostDelayedTaskWithTraits()`,
which calls `TaskScheduler::GetInstance()`, dereferencing the global
`TaskScheduler` pointer
`g_task_scheduler`, which was null because the CL did not add any call to set
this pointer in
Cronet's execution path.

Fixed by adding a call to `TaskScheduler::CreateAndSetSimpleTaskSceduler()` to
the beginning `CronetEnvironment::Initialize()`.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2639673002
Cr-Commit-Position: refs/heads/master@{#444368}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c4ab790c6018a4501ed7bc02fa1...
==========

[commit-bot: I haz the power, 2017-01-18 15:34:48]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8c4ab790c6018a4501ed7bc02fa1...