Fix flaky tests in CronetUrlRequestContextTest

Fix flaky tests in CronetUrlRequestContextTest

1. Fix of Bug 635025 & 596929. When
   CronetUrlRequestContextTest#testShutdown() is run multiple times, a
   race condition is created between the test thread and the executor
   threads. When the second run starts, the test replaces the value of
   mTestFramework with a new CronetEngine value; however,  the executor
   thread may still be executing ShutdownTestUrlRequestCallback from the
   previous run, which also references the same shared mTestFramework.
   If the test thread replaces the value of mTestFramework while
   onSucceeded() is still being executed, the callback may shut down the
   wrong engine. The same reasoning applies to testShutdownAfterError().
   The reason why the test was executed multiple times is due to
   http://crbug/637972.
2. Fix of Bug 637986. Block the test thread until the request callback
   finishes its work. See changes in
   ShutdownTestUrlRequestCallback#mCallbackCompletionBlock
3. At the end of the test, shut down the callback executor, to release
   the underlying thread.
4. Removed mTestFramework member variable to avoid similar issues in the
   future.

BUG=596929, 635025, 637986
Committed: https://crrev.com/1d5bef478ace1273acb57a4342be9558e0b7daaa
Cr-Commit-Position: refs/heads/master@{#412885}

[kapishnikov, 2016-08-17 20:30:53]

kapishnikov@chromium.org changed reviewers:
+ mgersh@chromium.org, pauljensen@chromium.org, xunieli@chromium.org

[kapishnikov, 2016-08-17 20:30:53]

Thanks to Paul, we were able to reproduce the flaky CronetUrlRequestContextTest
crashes locally. Here is the fix. PTAL

[pauljensen, 2016-08-18 15:05:29]

https://codereview.chromium.org/2254043002/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/2254043002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:51:
CronetTestFramework mTestFramework;
Please remove this variable.  It's the root cause of this how this bug slipped
in.  It's no longer used in a class-wide scope and should be replaced by
method-scoped variables so as to avoid this bug in the future, and also better
follow our coding style of keeping variables in the narrowest scope.

https://codereview.chromium.org/2254043002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:103:
private ConditionVariable mCallbackCompletionBlock = new ConditionVariable();
final

https://codereview.chromium.org/2254043002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:123:
void blockForCallbackToComplete() {
add: // Wait for request completion callback.

[kapishnikov, 2016-08-18 16:18:33]

Paul, thanks for the review. I have removed the mTestFramework member variable.
PTAL

https://codereview.chromium.org/2254043002/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/2254043002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:51:
CronetTestFramework mTestFramework;
On 2016/08/18 15:05:29, pauljensen wrote:
> Please remove this variable.  It's the root cause of this how this bug slipped
> in.  It's no longer used in a class-wide scope and should be replaced by
> method-scoped variables so as to avoid this bug in the future, and also better
> follow our coding style of keeping variables in the narrowest scope.

Done.

https://codereview.chromium.org/2254043002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:103:
private ConditionVariable mCallbackCompletionBlock = new ConditionVariable();
On 2016/08/18 15:05:29, pauljensen wrote:
> final

Done.

https://codereview.chromium.org/2254043002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:123:
void blockForCallbackToComplete() {
On 2016/08/18 15:05:29, pauljensen wrote:
> add: // Wait for request completion callback.

Done.

[pauljensen, 2016-08-18 16:39:55]

lgtm % one comment

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

https://codereview.chromium.org/2254043002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:123:
// Wait for request completion callback.
move this comment up a line

[kapishnikov, 2016-08-18 16:54:51]

Description was changed from

==========
Fix flaky tests in CronetUrlRequestContextTest

1. Fix of Bug 635025 & 596929. When
   CronetUrlRequestContextTest#testShutdown() is run multiple times, a
   race condition is created between the test thread and the executor
   threads. When the second run starts, the test replaces the value of
   mTestFramework with a new CronetEngine value; however,  the executor
   thread may still be executing ShutdownTestUrlRequestCallback from the
   previous run, which also references the same shared mTestFramework.
   If the test thread replaces the value of mTestFramework while
   onSucceeded() is still being executed, the callback may shut down the
   wrong engine. The same reasoning applies to testShutdownAfterError().
   The reason why the test was executed multiple times is due to
   http://crbug/637972.
2. Fix of Bug 637986. Block the test thread until the request callback
   finishes its work. See changes in
   ShutdownTestUrlRequestCallback#mCallbackCompletionBlock
3. At the end of the test, shut down the callback executor, to release
   the underlying thread.

BUG=596929,635025,637986
==========

to

==========
Fix flaky tests in CronetUrlRequestContextTest

1. Fix of Bug 635025 & 596929. When
   CronetUrlRequestContextTest#testShutdown() is run multiple times, a
   race condition is created between the test thread and the executor
   threads. When the second run starts, the test replaces the value of
   mTestFramework with a new CronetEngine value; however,  the executor
   thread may still be executing ShutdownTestUrlRequestCallback from the
   previous run, which also references the same shared mTestFramework.
   If the test thread replaces the value of mTestFramework while
   onSucceeded() is still being executed, the callback may shut down the
   wrong engine. The same reasoning applies to testShutdownAfterError().
   The reason why the test was executed multiple times is due to
   http://crbug/637972.
2. Fix of Bug 637986. Block the test thread until the request callback
   finishes its work. See changes in
   ShutdownTestUrlRequestCallback#mCallbackCompletionBlock
3. At the end of the test, shut down the callback executor, to release
   the underlying thread.
4. Removed mTestFramework member variable to avoid similar issues in the
   future.

BUG=596929,635025,637986
==========

[kapishnikov, 2016-08-18 16:55:06]

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

https://codereview.chromium.org/2254043002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:123:
// Wait for request completion callback.
On 2016/08/18 16:39:55, pauljensen wrote:
> move this comment up a line

Done.

[kapishnikov, 2016-08-18 16:55:15]

The CQ bit was checked by kapishnikov@chromium.org

[kapishnikov, 2016-08-18 16:55:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/2254043002/#ps40001
(title: "Fixed a comment")

[commit-bot: I haz the power, 2016-08-18 16:55:38]

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

[kapishnikov, 2016-08-18 16:56:35]

kapishnikov@chromium.org changed reviewers:
+ xunjieli@chromium.org
- xunieli@chromium.org

[xunjieli, 2016-08-18 17:24:43]

+ 1 this totally deserves an award!

[commit-bot: I haz the power, 2016-08-18 17:47:14]

Description was changed from

==========
Fix flaky tests in CronetUrlRequestContextTest

1. Fix of Bug 635025 & 596929. When
   CronetUrlRequestContextTest#testShutdown() is run multiple times, a
   race condition is created between the test thread and the executor
   threads. When the second run starts, the test replaces the value of
   mTestFramework with a new CronetEngine value; however,  the executor
   thread may still be executing ShutdownTestUrlRequestCallback from the
   previous run, which also references the same shared mTestFramework.
   If the test thread replaces the value of mTestFramework while
   onSucceeded() is still being executed, the callback may shut down the
   wrong engine. The same reasoning applies to testShutdownAfterError().
   The reason why the test was executed multiple times is due to
   http://crbug/637972.
2. Fix of Bug 637986. Block the test thread until the request callback
   finishes its work. See changes in
   ShutdownTestUrlRequestCallback#mCallbackCompletionBlock
3. At the end of the test, shut down the callback executor, to release
   the underlying thread.
4. Removed mTestFramework member variable to avoid similar issues in the
   future.

BUG=596929,635025,637986
==========

to

==========
Fix flaky tests in CronetUrlRequestContextTest

1. Fix of Bug 635025 & 596929. When
   CronetUrlRequestContextTest#testShutdown() is run multiple times, a
   race condition is created between the test thread and the executor
   threads. When the second run starts, the test replaces the value of
   mTestFramework with a new CronetEngine value; however,  the executor
   thread may still be executing ShutdownTestUrlRequestCallback from the
   previous run, which also references the same shared mTestFramework.
   If the test thread replaces the value of mTestFramework while
   onSucceeded() is still being executed, the callback may shut down the
   wrong engine. The same reasoning applies to testShutdownAfterError().
   The reason why the test was executed multiple times is due to
   http://crbug/637972.
2. Fix of Bug 637986. Block the test thread until the request callback
   finishes its work. See changes in
   ShutdownTestUrlRequestCallback#mCallbackCompletionBlock
3. At the end of the test, shut down the callback executor, to release
   the underlying thread.
4. Removed mTestFramework member variable to avoid similar issues in the
   future.

BUG=596929,635025,637986
==========

[commit-bot: I haz the power, 2016-08-18 17:47:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-18 17:48:49]

Description was changed from

==========
Fix flaky tests in CronetUrlRequestContextTest

1. Fix of Bug 635025 & 596929. When
   CronetUrlRequestContextTest#testShutdown() is run multiple times, a
   race condition is created between the test thread and the executor
   threads. When the second run starts, the test replaces the value of
   mTestFramework with a new CronetEngine value; however,  the executor
   thread may still be executing ShutdownTestUrlRequestCallback from the
   previous run, which also references the same shared mTestFramework.
   If the test thread replaces the value of mTestFramework while
   onSucceeded() is still being executed, the callback may shut down the
   wrong engine. The same reasoning applies to testShutdownAfterError().
   The reason why the test was executed multiple times is due to
   http://crbug/637972.
2. Fix of Bug 637986. Block the test thread until the request callback
   finishes its work. See changes in
   ShutdownTestUrlRequestCallback#mCallbackCompletionBlock
3. At the end of the test, shut down the callback executor, to release
   the underlying thread.
4. Removed mTestFramework member variable to avoid similar issues in the
   future.

BUG=596929,635025,637986
==========

to

==========
Fix flaky tests in CronetUrlRequestContextTest

1. Fix of Bug 635025 & 596929. When
   CronetUrlRequestContextTest#testShutdown() is run multiple times, a
   race condition is created between the test thread and the executor
   threads. When the second run starts, the test replaces the value of
   mTestFramework with a new CronetEngine value; however,  the executor
   thread may still be executing ShutdownTestUrlRequestCallback from the
   previous run, which also references the same shared mTestFramework.
   If the test thread replaces the value of mTestFramework while
   onSucceeded() is still being executed, the callback may shut down the
   wrong engine. The same reasoning applies to testShutdownAfterError().
   The reason why the test was executed multiple times is due to
   http://crbug/637972.
2. Fix of Bug 637986. Block the test thread until the request callback
   finishes its work. See changes in
   ShutdownTestUrlRequestCallback#mCallbackCompletionBlock
3. At the end of the test, shut down the callback executor, to release
   the underlying thread.
4. Removed mTestFramework member variable to avoid similar issues in the
   future.

BUG=596929,635025,637986

Committed: https://crrev.com/1d5bef478ace1273acb57a4342be9558e0b7daaa
Cr-Commit-Position: refs/heads/master@{#412885}
==========

[commit-bot: I haz the power, 2016-08-18 17:48:51]

Patchset 3 (id:??) landed as
https://crrev.com/1d5bef478ace1273acb57a4342be9558e0b7daaa
Cr-Commit-Position: refs/heads/master@{#412885}