[Cronet] Leak NetLog instance used by CronetUrlRequestContextAdapter

[Cronet] Leak NetLog instance used by CronetUrlRequestContextAdapter

BUG=486120
BUG=600890
Committed: https://crrev.com/732deb3a4536d90f0469a6a498bb6f73fe9bf6d2
Cr-Commit-Position: refs/heads/master@{#385906}

[xunjieli, 2016-04-06 17:23:39]

Description was changed from

==========
[Cronet] Leak NetLog instance used by CronetUrlRequestContextAdapter

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

to

==========
[Cronet] Leak NetLog instance used by CronetUrlRequestContextAdapter

BUG=486120
BUG=600890
==========

[xunjieli, 2016-04-06 17:23:39]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org

[xunjieli, 2016-04-06 17:26:15]

Matt, thanks for the suggestions. Does this look about right?  I am not sure how
we should test this change. PTAL

[mmenke, 2016-04-06 17:43:19]

On 2016/04/06 17:26:15, xunjieli wrote:
> Matt, thanks for the suggestions. Does this look about right?  I am not sure
how
> we should test this change. PTAL

You should also update the API docs for the methods to save a log - they now
gather info from all URLRequestContexts.

Two suggested test:

Create a URLRequestContext.  Start logging, send a request, for the response.
Create another URLRequestContext, send a request, wait for a response.  Stop
logging and destroy both URLRequestContexts.  Make sure the log has info both
both requests (Only need the request to the first context to make sure that the
observer is hooked up to the NetLog).

More aimed at the race:  Create a URLRequestContext, send a request for a
cacheable resource, wait for the response.  Tear down the test server.  Create
another request for the same resource, send it, wait for the headers.  Send the
first body read, then immediately cancel the request, tear down the
URLRequestContext.  Hopefully this will somewhat consistently trigger the race.

[xunjieli, 2016-04-06 19:15:21]

Makes sense. Thanks, Matt! PTAL.

[mmenke, 2016-04-06 21:38:24]

On 2016/04/06 19:15:21, xunjieli wrote:
> Makes sense. Thanks, Matt! PTAL.

Looks like both tests are failing.  Mind fixing those before I look?  Been a
heavy review week for me.

[xunjieli, 2016-04-06 22:08:53]

Patchset #3 (id:40001) has been deleted

[xunjieli, 2016-04-07 00:46:33]

On 2016/04/06 21:38:24, mmenke wrote:
> On 2016/04/06 19:15:21, xunjieli wrote:
> > Makes sense. Thanks, Matt! PTAL.
> 
> Looks like both tests are failing.  Mind fixing those before I look?  Been a
> heavy review week for me.

Done. Sorry about that. I also reverted some changes to remove @FlakyTest
notations. One of those test is still failing. I will handle those later.

[mmenke, 2016-04-07 19:55:13]

https://codereview.chromium.org/1868433003/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1868433003/diff/60001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:349:
LAZY_INSTANCE_INITIALIZER;
This should be in the anonymous namespace above.

https://codereview.chromium.org/1868433003/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1868433003/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:682:
request.cancel();
Can we shut down the engine here?

Also, if you revert the fix, does this test often crash?  Just curious if it
semi-consistently fails on regression.

https://codereview.chromium.org/1868433003/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:687:
throw new RuntimeException("Unexpected");
May want to explain why these aren't racy (Namely, they both depend on a single
threaded executor that runs both onResponseStarted and onSucceeded / onFailed)

[xunjieli, 2016-04-07 21:33:26]

Patchset #4 (id:80001) has been deleted

[xunjieli, 2016-04-07 21:34:01]

Thanks, Matt!

https://codereview.chromium.org/1868433003/diff/60001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1868433003/diff/60001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:349:
LAZY_INSTANCE_INITIALIZER;
On 2016/04/07 19:55:13, mmenke wrote:
> This should be in the anonymous namespace above.

Done.

https://codereview.chromium.org/1868433003/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1868433003/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:682:
request.cancel();
On 2016/04/07 19:55:13, mmenke wrote:
> Can we shut down the engine here?

Done. 

> 
> Also, if you revert the fix, does this test often crash?  Just curious if it
> semi-consistently fails on regression.

I was hoping it would crash, but it looks like my device can't repro the crash
on the other known flaky tests (DiskStorageTest) either. So I don't really
know.. But I think in theory, it should crash often without this change.

https://codereview.chromium.org/1868433003/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:687:
throw new RuntimeException("Unexpected");
On 2016/04/07 19:55:13, mmenke wrote:
> May want to explain why these aren't racy (Namely, they both depend on a
single
> threaded executor that runs both onResponseStarted and onSucceeded / onFailed)

Done.

[mmenke, 2016-04-07 21:37:32]

LGTM

https://codereview.chromium.org/1868433003/diff/100001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1868433003/diff/100001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:689:
// onSucceeded must not happen, because the request is canceled
nit:  "must not" -> "will not", since we're more explaining why it won't happen,
than why it's important it won't happen.

[xunjieli, 2016-04-07 21:41:20]

https://codereview.chromium.org/1868433003/diff/100001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1868433003/diff/100001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:689:
// onSucceeded must not happen, because the request is canceled
On 2016/04/07 21:37:32, mmenke wrote:
> nit:  "must not" -> "will not", since we're more explaining why it won't
happen,
> than why it's important it won't happen.

Right. That makes sense. Done.

[xunjieli, 2016-04-07 21:41:29]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-04-07 21:41:29]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1868433003/#ps120001
(title: "Adjust comment")

[commit-bot: I haz the power, 2016-04-07 21:41:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1868433003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1868433003/120001

[commit-bot: I haz the power, 2016-04-07 22:35:23]

Description was changed from

==========
[Cronet] Leak NetLog instance used by CronetUrlRequestContextAdapter

BUG=486120
BUG=600890
==========

to

==========
[Cronet] Leak NetLog instance used by CronetUrlRequestContextAdapter

BUG=486120
BUG=600890
==========

[commit-bot: I haz the power, 2016-04-07 22:35:24]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-04-07 22:36:45]

Description was changed from

==========
[Cronet] Leak NetLog instance used by CronetUrlRequestContextAdapter

BUG=486120
BUG=600890
==========

to

==========
[Cronet] Leak NetLog instance used by CronetUrlRequestContextAdapter

BUG=486120
BUG=600890

Committed: https://crrev.com/732deb3a4536d90f0469a6a498bb6f73fe9bf6d2
Cr-Commit-Position: refs/heads/master@{#385906}
==========

[commit-bot: I haz the power, 2016-04-07 22:36:46]

Patchset 5 (id:??) landed as
https://crrev.com/732deb3a4536d90f0469a6a498bb6f73fe9bf6d2
Cr-Commit-Position: refs/heads/master@{#385906}