Fixing Intermittent SecurityKey Unittest Failures

Fixing Intermittent SecurityKey Unittest Failures

This issue was introduced with the move from IPC to Mojo.  The problem
occurs when tests requiring IPC Support run back to back, running them
individually is fine.  In the error scenario, the IPC Server class fails
to create a Mojo channel.  This appears to be a result of initializing a
new IPC Support object before every test is executed and cleaning it up
afterwards.  The cleanup is asynchronous and can cause problems with the
subsequent test run.

My solution is to initialize the IPC Support object before the call to
execute tests, that way the setup and cleanup only occurs once per test
suite run and we don't hit these race conditions.  This looks like a
common pattern in other unit tests in Chromium.

BUG=686922
Review-Url: https://codereview.chromium.org/2663103003
Cr-Commit-Position: refs/heads/master@{#448650}
Committed: https://chromium.googlesource.com/chromium/src/+/a7c4f961c4f1e8724dc7cb05552aa30b2c58fa9a

[joedow, 2017-01-31 20:52:21]

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

[commit-bot: I haz the power, 2017-01-31 20:52:53]

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-01-31 22:00:20]

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

[commit-bot: I haz the power, 2017-01-31 22:00:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joedow, 2017-01-31 22:09:49]

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

[commit-bot: I haz the power, 2017-01-31 22:10:39]

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-01-31 23:37:12]

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

[commit-bot: I haz the power, 2017-01-31 23:37:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[joedow, 2017-02-01 21:40:35]

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

[commit-bot: I haz the power, 2017-02-01 21:40:56]

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-02-01 22:33:07]

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

[commit-bot: I haz the power, 2017-02-01 22:33:08]

Dry run: This issue passed the CQ dry run.

[joedow, 2017-02-01 22:44:10]

joedow@chromium.org changed reviewers:
+ sergeyu@chromium.org

[joedow, 2017-02-01 22:44:10]

PTAL!

[nicholss, 2017-02-01 23:00:36]

nicholss@chromium.org changed reviewers:
+ nicholss@chromium.org

[nicholss, 2017-02-01 23:00:36]

lgtm

[Sergey Ulanov, 2017-02-02 01:46:40]

https://codereview.chromium.org/2663103003/diff/40001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_server_unittest.cc (right):

https://codereview.chromium.org/2663103003/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_server_unittest.cc:112: void
SecurityKeyIpcServerTest::WaitForPendingOperations() {
Maybe call this RunPendingTasks()? This function doesn't really wait for
anything. It just runs the tasks in the queue.

https://codereview.chromium.org/2663103003/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_server_unittest.cc:114:
run_loop_.reset(new base::RunLoop());
Each RunLoop is usable only once, i.e. Run[UntilIdle]() cannot be called more
than once (see
https://codesearch.chromium.org/chromium/src/base/run_loop.cc?l=71). But this
function creates RunLoop twice, but uses it only once, so I'm not sure you want
to reset RunLoop here, given that it's replaced after each Run() call in
WaitForOperationComplete()

Also, since this function doesn't depend on OperationComplete(), can you just
call RunLoop().RunUntilIdle(), without replacing run_loop_?

https://codereview.chromium.org/2663103003/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_server_unittest.cc:170:
WaitForOperationComplete();
Since this needs to be done after every test maybe put it in TearDown()?
There are many tests that use the same logic for classes that requires async
shutdown, e.g. see
https://codesearch.chromium.org/chromium/src/remoting/protocol/connection_uni...

[joedow, 2017-02-03 22:14:18]

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

[commit-bot: I haz the power, 2017-02-03 22:15:00]

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-02-03 22:31:17]

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

[commit-bot: I haz the power, 2017-02-03 22:31:18]

Dry run: This issue passed the CQ dry run.

[joedow, 2017-02-03 22:32:46]

Addressed feedback, PTAL!

https://codereview.chromium.org/2663103003/diff/40001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_server_unittest.cc (right):

https://codereview.chromium.org/2663103003/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_server_unittest.cc:112: void
SecurityKeyIpcServerTest::WaitForPendingOperations() {
On 2017/02/02 01:46:40, Sergey Ulanov wrote:
> Maybe call this RunPendingTasks()? This function doesn't really wait for
> anything. It just runs the tasks in the queue.

Done.

https://codereview.chromium.org/2663103003/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_server_unittest.cc:114:
run_loop_.reset(new base::RunLoop());
On 2017/02/02 01:46:40, Sergey Ulanov wrote:
> Each RunLoop is usable only once, i.e. Run[UntilIdle]() cannot be called more
> than once (see
> https://codesearch.chromium.org/chromium/src/base/run_loop.cc?l=71). But this
> function creates RunLoop twice, but uses it only once, so I'm not sure you
want
> to reset RunLoop here, given that it's replaced after each Run() call in
> WaitForOperationComplete()
> 
> Also, since this function doesn't depend on OperationComplete(), can you just
> call RunLoop().RunUntilIdle(), without replacing run_loop_?

Excellent suggestion, thanks!

https://codereview.chromium.org/2663103003/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_server_unittest.cc:170:
WaitForOperationComplete();
On 2017/02/02 01:46:40, Sergey Ulanov wrote:
> Since this needs to be done after every test maybe put it in TearDown()?
> There are many tests that use the same logic for classes that requires async
> shutdown, e.g. see
>
https://codesearch.chromium.org/chromium/src/remoting/protocol/connection_uni...

There are some cases which do not complete the connection process so they don't
need this logic.

[Sergey Ulanov, 2017-02-06 17:26:58]

lgtm, but please see my comment in the previous message

[joedow, 2017-02-07 16:52:41]

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

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

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-02-07 17:07:27]

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

[commit-bot: I haz the power, 2017-02-07 17:07:28]

Dry run: This issue passed the CQ dry run.

[joedow, 2017-02-07 17:09:20]

I added a TearDown method which drains the current RunLoop (based on Sergey's
previous comments) and removed the new 'WaitForCompletion' calls.

Thanks for the review!

[joedow, 2017-02-07 17:09:28]

The CQ bit was checked by joedow@chromium.org

[joedow, 2017-02-07 17:09:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org, sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2663103003/#ps80001
(title: "Addressing more feedback")

[commit-bot: I haz the power, 2017-02-07 17:09:47]

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-07 17:13:21]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486487368821780,
"parent_rev": "9b171087251910401d4ef71fca267f6a2b53b1f5", "commit_rev":
"a7c4f961c4f1e8724dc7cb05552aa30b2c58fa9a"}

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

Description was changed from

==========
Fixing Intermittent SecurityKey Unittest Failures

This issue was introduced with the move from IPC to Mojo.  The problem
occurs when tests requiring IPC Support run back to back, running them
individually is fine.  In the error scenario, the IPC Server class fails
to create a Mojo channel.  This appears to be a result of initializing a
new IPC Support object before every test is executed and cleaning it up
afterwards.  The cleanup is asynchronous and can cause problems with the
subsequent test run.

My solution is to initialize the IPC Support object before the call to
execute tests, that way the setup and cleanup only occurs once per test
suite run and we don't hit these race conditions.  This looks like a
common pattern in other unit tests in Chromium.

BUG=686922
==========

to

==========
Fixing Intermittent SecurityKey Unittest Failures

This issue was introduced with the move from IPC to Mojo.  The problem
occurs when tests requiring IPC Support run back to back, running them
individually is fine.  In the error scenario, the IPC Server class fails
to create a Mojo channel.  This appears to be a result of initializing a
new IPC Support object before every test is executed and cleaning it up
afterwards.  The cleanup is asynchronous and can cause problems with the
subsequent test run.

My solution is to initialize the IPC Support object before the call to
execute tests, that way the setup and cleanup only occurs once per test
suite run and we don't hit these race conditions.  This looks like a
common pattern in other unit tests in Chromium.

BUG=686922

Review-Url: https://codereview.chromium.org/2663103003
Cr-Commit-Position: refs/heads/master@{#448650}
Committed:
https://chromium.googlesource.com/chromium/src/+/a7c4f961c4f1e8724dc7cb05552a...
==========

[commit-bot: I haz the power, 2017-02-07 17:13:59]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a7c4f961c4f1e8724dc7cb05552a...