[cronet] Add mechanism for restarting CronetEnvironment

[cronet] Add mechanism for restarting CronetEnvironment

This CL adds a primitive mechanism for restarting CronetEnvironment, primarily
to allow tests to run with multiple configurations.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2836063005
Cr-Commit-Position: refs/heads/master@{#470033}
Committed: https://chromium.googlesource.com/chromium/src/+/94bde17fed2469a58c0fa6ae4374d71264ef1370

[lilyhoughton, 2017-04-25 14:42:57]

Description was changed from

==========
[cronet] Add mechanism for restarting CronetEnvironment

This CL adds a primitive mechanism for restarting CronetEnvironment, primarily
to allow tests to run with multiple configurations.

BUG=
==========

to

==========
[cronet] Add mechanism for restarting CronetEnvironment

This CL adds a primitive mechanism for restarting CronetEnvironment, primarily
to allow tests to run with multiple configurations.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[lilyhoughton, 2017-04-25 14:44:13]

Description was changed from

==========
[cronet] Add mechanism for restarting CronetEnvironment

This CL adds a primitive mechanism for restarting CronetEnvironment, primarily
to allow tests to run with multiple configurations.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[cronet] Add mechanism for restarting CronetEnvironment

This CL adds a primitive mechanism for restarting CronetEnvironment, primarily
to allow tests to run with multiple configurations.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[lilyhoughton, 2017-04-25 14:44:13]

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

[lilyhoughton, 2017-04-25 15:52:06]

On 2017/04/25 14:44:13, lilyhoughton wrote:
> mailto:lilyhoughton@chromium.org changed reviewers:
> + mailto:mef@chromium.org

PTAL

I think that the actual internal code needs to be a little smarter about a)
checking for pending requests and b) shutting down the TaskScheduler (see the
commented out code in the destructor).  I should also double check to see if the
android code does anything else I should be doing as well.

From an API perspective, I want to add an explicit shutdown method, in
particular so that the configuration methods can still check if Cronet has been
started yet.  (Right now, since Cronet can't actually be shut down per se, only
restarted, configuration for a new instance happens while the old instance is
still running.  This works, but seems like it might be confusing).

[mef, 2017-04-25 17:55:11]

https://codereview.chromium.org/2836063005/diff/1/components/cronet/ios/Cronet.h
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2836063005/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.h:77: + (void)initializeCronet;
Does it need to be public?

https://codereview.chromium.org/2836063005/diff/1/components/cronet/ios/Crone...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2836063005/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.mm:245: [self initializeCronet];
maybe call cronet::CronetEnvironment::Initialize(); here and below and get rid
of initializeCronet?

https://codereview.chromium.org/2836063005/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.mm:252: [self startInternal];
This now will happen current thread instead of main thread, but I think that's
ok.

[lilyhoughton, 2017-04-25 18:56:38]

The CQ bit was checked by lilyhoughton@chromium.org to run a CQ dry run

[lilyhoughton, 2017-04-25 18:57:06]

The CQ bit was unchecked by lilyhoughton@chromium.org

[lilyhoughton, 2017-04-25 18:57:07]

The CQ bit was checked by lilyhoughton@chromium.org

[commit-bot: I haz the power, 2017-04-25 18:57:14]

Dry run: CQ is trying da patch.

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

[lilyhoughton, 2017-04-25 18:57:30]

The CQ bit was unchecked by lilyhoughton@chromium.org

[lilyhoughton, 2017-04-25 18:59:17]

The CQ bit was checked by lilyhoughton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 18:59:52]

Dry run: 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-04-25 19:05:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 19:05:39]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lilyhoughton, 2017-04-26 17:08:12]

The CQ bit was checked by lilyhoughton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 17:08:40]

Dry run: 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-04-26 17:49:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 17:49:47]

Dry run: This issue passed the CQ dry run.

[lilyhoughton, 2017-04-26 18:31:53]

https://codereview.chromium.org/2836063005/diff/1/components/cronet/ios/Cronet.h
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2836063005/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.h:77: + (void)initializeCronet;
On 2017/04/25 17:55:10, mef wrote:
> Does it need to be public?

Nope.  Removed.

https://codereview.chromium.org/2836063005/diff/1/components/cronet/ios/Crone...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2836063005/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.mm:245: [self initializeCronet];
On 2017/04/25 17:55:10, mef wrote:
> maybe call cronet::CronetEnvironment::Initialize(); here and below and get rid
> of initializeCronet?

Good call.  Done.

https://codereview.chromium.org/2836063005/diff/1/components/cronet/ios/Crone...
components/cronet/ios/Cronet.mm:252: [self startInternal];
On 2017/04/25 17:55:10, mef wrote:
> This now will happen current thread instead of main thread, but I think that's
> ok.

Acknowledged.

[mef, 2017-04-26 22:19:40]

mef@chromium.org changed reviewers:
+ kapishnikov@chromium.org

[mef, 2017-04-26 22:19:41]

Looks pretty good, but I'm very scared of having public |shutdown| method.

Adding Andrei to weigh in.

https://codereview.chromium.org/2836063005/diff/60001/components/cronet/ios/C...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2836063005/diff/60001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:84: + (void)shutdown;
// need comment.

https://codereview.chromium.org/2836063005/diff/60001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2836063005/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.mm:260: // TODO(lilyhoughton) this
should be smarter about making sure there are no
I think there is also a problem with having objects owned by CronetEnvironment
(https://cs.chromium.org/chromium/src/components/cronet/ios/cronet_environment...)
deleted before request context.

Maybe for the purposes of this CL we should make |shutdown| a
|shutdownForTesting| and take it out of public API in Cronet.h?

https://codereview.chromium.org/2836063005/diff/60001/components/grpc_support...
File components/grpc_support/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/2836063005/diff/60001/components/grpc_support...
components/grpc_support/bidirectional_stream_unittest.cc:44:
ShutdownTestStreamEngine();
I'd move it above ShutdownQuicTestServer() to make it easier to reason about
their relative lifetimes.

https://codereview.chromium.org/2836063005/diff/60001/components/grpc_support...
File components/grpc_support/test/get_stream_engine.h (right):

https://codereview.chromium.org/2836063005/diff/60001/components/grpc_support...
components/grpc_support/test/get_stream_engine.h:17: void
StartTestStreamEngine(int port);
Public methods need comments.

[lilyhoughton, 2017-04-27 13:53:39]

https://codereview.chromium.org/2836063005/diff/60001/components/cronet/ios/C...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2836063005/diff/60001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:84: + (void)shutdown;
On 2017/04/26 22:19:41, mef wrote:
> // need comment.

Actually, just removing this.

https://codereview.chromium.org/2836063005/diff/60001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2836063005/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.mm:260: // TODO(lilyhoughton) this
should be smarter about making sure there are no
On 2017/04/26 22:19:41, mef wrote:
> I think there is also a problem with having objects owned by CronetEnvironment
>
(https://cs.chromium.org/chromium/src/components/cronet/ios/cronet_environment...)
> deleted before request context.

Not really sure what to do about this, since afaict we can't block on waiting
for request context to be deleted on network thread?

> Maybe for the purposes of this CL we should make |shutdown| a
> |shutdownForTesting| and take it out of public API in Cronet.h?

Done.

https://codereview.chromium.org/2836063005/diff/60001/components/grpc_support...
File components/grpc_support/bidirectional_stream_unittest.cc (right):

https://codereview.chromium.org/2836063005/diff/60001/components/grpc_support...
components/grpc_support/bidirectional_stream_unittest.cc:44:
ShutdownTestStreamEngine();
On 2017/04/26 22:19:41, mef wrote:
> I'd move it above ShutdownQuicTestServer() to make it easier to reason about
> their relative lifetimes.

Done.

https://codereview.chromium.org/2836063005/diff/60001/components/grpc_support...
File components/grpc_support/test/get_stream_engine.h (right):

https://codereview.chromium.org/2836063005/diff/60001/components/grpc_support...
components/grpc_support/test/get_stream_engine.h:17: void
StartTestStreamEngine(int port);
On 2017/04/26 22:19:41, mef wrote:
> Public methods need comments.

Done, though PTAL to make sure it's sufficiently clear etc.

[mef, 2017-04-27 14:58:20]

https://codereview.chromium.org/2836063005/diff/60001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2836063005/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.mm:260: // TODO(lilyhoughton) this
should be smarter about making sure there are no
On 2017/04/27 13:53:39, lilyhoughton wrote:
> On 2017/04/26 22:19:41, mef wrote:
> > I think there is also a problem with having objects owned by
CronetEnvironment
> >
>
(https://cs.chromium.org/chromium/src/components/cronet/ios/cronet_environment...)
> > deleted before request context.
> 
> Not really sure what to do about this, since afaict we can't block on waiting
> for request context to be deleted on network thread?
Yeah, this needs more thought and test coverage, so I think it is fine to punt
to later CL.
> 
> > Maybe for the purposes of this CL we should make |shutdown| a
> > |shutdownForTesting| and take it out of public API in Cronet.h?
> 
> Done.

https://codereview.chromium.org/2836063005/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_http_test.mm (right):

https://codereview.chromium.org/2836063005/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:24: @interface Cronet
(ExposedForTesting)
I wonder whether it makes sense to create a header like
Cronet+ExposedForTesting.h and move declaration of all *ForTesting methods into
it?

https://codereview.chromium.org/2836063005/diff/80001/components/grpc_support...
File components/grpc_support/test/get_stream_engine.h (right):

https://codereview.chromium.org/2836063005/diff/80001/components/grpc_support...
components/grpc_support/test/get_stream_engine.h:20: // between test cases
(including between all of the bidirectional
nit: missing )

[lilyhoughton, 2017-05-01 15:31:35]

https://codereview.chromium.org/2836063005/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_http_test.mm (right):

https://codereview.chromium.org/2836063005/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:24: @interface Cronet
(ExposedForTesting)
On 2017/04/27 14:58:20, mef wrote:
> I wonder whether it makes sense to create a header like
> Cronet+ExposedForTesting.h and move declaration of all *ForTesting methods
into
> it?

I like this idea a lot.  It probably makes sense as a separate CL, though (esp.
since it's going to involve a bit of restructuring of things).

https://codereview.chromium.org/2836063005/diff/80001/components/grpc_support...
File components/grpc_support/test/get_stream_engine.h (right):

https://codereview.chromium.org/2836063005/diff/80001/components/grpc_support...
components/grpc_support/test/get_stream_engine.h:20: // between test cases
(including between all of the bidirectional
On 2017/04/27 14:58:20, mef wrote:
> nit: missing )

Done.

[mef, 2017-05-01 19:12:59]

It looks reasonably, considering that we use it for testing only.

https://codereview.chromium.org/2836063005/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_http_test.mm (right):

https://codereview.chromium.org/2836063005/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:24: @interface Cronet
(ExposedForTesting)
On 2017/05/01 15:31:35, lilyhoughton wrote:
> On 2017/04/27 14:58:20, mef wrote:
> > I wonder whether it makes sense to create a header like
> > Cronet+ExposedForTesting.h and move declaration of all *ForTesting methods
> into
> > it?
> 
> I like this idea a lot.  It probably makes sense as a separate CL, though
(esp.
> since it's going to involve a bit of restructuring of things).

sgtm

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.mm:252: //
net::HTTPProtocolHandlerDelegate::SetInstance(nullptr);
Please add TODO to make unregistering of HTTPProtocolHandlerDelegate work.

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.mm:256: // base::TaskScheduler* ts =
base::TaskScheduler::GetInstance();
Do we actually need to call this at all?

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/start_cronet.h (right):

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
components/cronet/ios/test/start_cronet.h:7: // Starts Cronet if it hasn't
already been started. Will always update Cronet
This comment needs to be updated.

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/start_cronet.mm (left):

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
components/cronet/ios/test/start_cronet.mm:27: [Cronet
setHttpCacheType:CRNHttpCacheTypeDisabled];
This line is lost in sync?

[lilyhoughton, 2017-05-01 19:28:13]

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.mm:252: //
net::HTTPProtocolHandlerDelegate::SetInstance(nullptr);
On 2017/05/01 19:12:58, mef wrote:
> Please add TODO to make unregistering of HTTPProtocolHandlerDelegate work.

Done.

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.mm:256: // base::TaskScheduler* ts =
base::TaskScheduler::GetInstance();
On 2017/05/01 19:12:58, mef wrote:
> Do we actually need to call this at all?

I will do more research to check.

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/start_cronet.h (right):

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
components/cronet/ios/test/start_cronet.h:7: // Starts Cronet if it hasn't
already been started. Will always update Cronet
On 2017/05/01 19:12:58, mef wrote:
> This comment needs to be updated.

Done.

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/start_cronet.mm (left):

https://codereview.chromium.org/2836063005/diff/100001/components/cronet/ios/...
components/cronet/ios/test/start_cronet.mm:27: [Cronet
setHttpCacheType:CRNHttpCacheTypeDisabled];
On 2017/05/01 19:12:58, mef wrote:
> This line is lost in sync?

Done.

[mef, 2017-05-02 19:03:08]

lgtm

[lilyhoughton, 2017-05-05 15:53:23]

kapishnikov@ could you PTAL

[kapishnikov, 2017-05-05 19:46:50]

Sorry, I haven't realised that you are waiting for me. I am okay with the change
but I think we should add more tests, e.g. to cover a scenario when the engine
is started from a worker thread. Ideally we should use a code coverage tool to
see if our tests cover the code well. You can do it in this CL or another one.
LGTM.

[lilyhoughton, 2017-05-08 14:10:07]

The CQ bit was checked by lilyhoughton@chromium.org

[commit-bot: I haz the power, 2017-05-08 14:10:21]

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-05-08 14:13:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 14:13:49]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lilyhoughton, 2017-05-08 15:12:56]

The CQ bit was checked by lilyhoughton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 15:13:24]

Dry run: 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-05-08 15:57:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 15:57:57]

Dry run: This issue passed the CQ dry run.

[lilyhoughton, 2017-05-08 17:16:10]

The CQ bit was checked by lilyhoughton@chromium.org

[lilyhoughton, 2017-05-08 17:16:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
kapishnikov@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2836063005/#ps140001
(title: "sync")

[commit-bot: I haz the power, 2017-05-08 17:16:29]

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-05-08 17:25:22]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494263770608040,
"parent_rev": "9af279fc5d9475d998c2080bb62be6a732279827", "commit_rev":
"94bde17fed2469a58c0fa6ae4374d71264ef1370"}

[commit-bot: I haz the power, 2017-05-08 17:25:36]

Description was changed from

==========
[cronet] Add mechanism for restarting CronetEnvironment

This CL adds a primitive mechanism for restarting CronetEnvironment, primarily
to allow tests to run with multiple configurations.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[cronet] Add mechanism for restarting CronetEnvironment

This CL adds a primitive mechanism for restarting CronetEnvironment, primarily
to allow tests to run with multiple configurations.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2836063005
Cr-Commit-Position: refs/heads/master@{#470033}
Committed:
https://chromium.googlesource.com/chromium/src/+/94bde17fed2469a58c0fa6ae4374...
==========

[commit-bot: I haz the power, 2017-05-08 17:25:37]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/94bde17fed2469a58c0fa6ae4374...