Send a message to the client if bad It2Me policies are read.

Send a message to the client if bad It2Me policies are read.

BUG=433009
Review-Url: https://codereview.chromium.org/2867223003
Cr-Commit-Position: refs/heads/master@{#470984}
Committed: https://chromium.googlesource.com/chromium/src/+/c96bfeee5db953d0c7a5e2d636f2622323b61f4f

[Jamie, 2017-05-08 23:19:40]

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

[Jamie, 2017-05-08 23:19:41]

ptal

[joedow, 2017-05-09 18:09:48]

lgtm

https://codereview.chromium.org/2867223003/diff/1/remoting/host/it2me/it2me_n...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/2867223003/diff/1/remoting/host/it2me/it2me_n...
remoting/host/it2me/it2me_native_messaging_host.cc:78:
DCHECK(!callback.is_null());
nit: DCHECK(callback);

https://codereview.chromium.org/2867223003/diff/1/remoting/host/it2me/it2me_n...
remoting/host/it2me/it2me_native_messaging_host.cc:531: 
nit: remove newline

https://codereview.chromium.org/2867223003/diff/1/remoting/host/it2me/it2me_n...
File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/2867223003/diff/1/remoting/host/it2me/it2me_n...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:217: void
SendDisonnectMessage(int id);
s/SendDisonnectMessage/SendDisconnectMessage

[Jamie, 2017-05-09 18:19:33]

FYI

https://codereview.chromium.org/2867223003/diff/1/remoting/host/it2me/it2me_n...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/2867223003/diff/1/remoting/host/it2me/it2me_n...
remoting/host/it2me/it2me_native_messaging_host.cc:78:
DCHECK(!callback.is_null());
On 2017/05/09 18:09:47, joedow wrote:
> nit: DCHECK(callback);

Done, here and above.

https://codereview.chromium.org/2867223003/diff/1/remoting/host/it2me/it2me_n...
remoting/host/it2me/it2me_native_messaging_host.cc:531: 
On 2017/05/09 18:09:47, joedow wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/2867223003/diff/1/remoting/host/it2me/it2me_n...
File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/2867223003/diff/1/remoting/host/it2me/it2me_n...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:217: void
SendDisonnectMessage(int id);
On 2017/05/09 18:09:47, joedow wrote:
> s/SendDisonnectMessage/SendDisconnectMessage

Done.

[Jamie, 2017-05-09 18:19:37]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2017-05-09 18:19:37]

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/2867223003/#ps20001
(title: "Reviewer feedback.")

[commit-bot: I haz the power, 2017-05-09 18:21:05]

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-09 19:43:33]

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

[commit-bot: I haz the power, 2017-05-09 19:43:34]

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_...)

[Jamie, 2017-05-10 00:44:39]

The race condition was due in part to the threading within
It2MeNativeMessagingHost itself, so the only way I could think of fixing it was
with a test-only closure to indicate when processing was complete. LMK if you
think this is acceptable.

[Jamie, 2017-05-10 00:44:43]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2017-05-10 00:44:43]

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/2867223003/#ps40001
(title: "Fix race condition.")

[Jamie, 2017-05-10 00:44:44]

The CQ bit was unchecked by jamiewalch@chromium.org

[Jamie, 2017-05-10 00:45:13]

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

[commit-bot: I haz the power, 2017-05-10 00:46:18]

Dry run: CQ is trying da patch.

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

[joedow, 2017-05-10 00:55:04]

https://codereview.chromium.org/2867223003/diff/40001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/2867223003/diff/40001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:314:
run_loop.QuitClosure()));
I think this is fine, another option would be to pass in a callback to a method
in the test fixture class in the SetUp method.  That would prevent the need to
store a * to the It2MeHost instance.

[Jamie, 2017-05-10 01:00:41]

On 2017/05/10 00:55:04, joedow wrote:
>
https://codereview.chromium.org/2867223003/diff/40001/remoting/host/it2me/it2...
> File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/2867223003/diff/40001/remoting/host/it2me/it2...
> remoting/host/it2me/it2me_native_messaging_host_unittest.cc:314:
> run_loop.QuitClosure()));
> I think this is fine, another option would be to pass in a callback to a
method
> in the test fixture class in the SetUp method.  That would prevent the need to
> store a * to the It2MeHost instance.

I think this approach is more readable. The closure refers to a run loop that is
local to the SetPolicies method, which I think makes sense since that run loop
only applies there. If I understand your proposal correctly, that run loop would
need to be a class member instead. Overall, I think I like my approach better :)

[commit-bot: I haz the power, 2017-05-10 01:43:52]

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

[commit-bot: I haz the power, 2017-05-10 01:43:53]

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_...)

[Jamie, 2017-05-10 18:33:14]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2017-05-10 18:33:14]

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/2867223003/#ps60001
(title: "Fix deadlock.")

[commit-bot: I haz the power, 2017-05-10 18:35: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-05-10 20:37:22]

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

[commit-bot: I haz the power, 2017-05-10 20:37:23]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jamie, 2017-05-10 21:39:33]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2017-05-10 21:39:33]

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/2867223003/#ps80001
(title: "Fix TSAN tests.")

[commit-bot: I haz the power, 2017-05-10 21:40: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-10 23:40:27]

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

[commit-bot: I haz the power, 2017-05-10 23:40:28]

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_...)

[commit-bot: I haz the power, 2017-05-10 23:42:45]

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

[commit-bot: I haz the power, 2017-05-10 23:42:47]

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_...)

[Jamie, 2017-05-10 23:50:42]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2017-05-10 23:50:42]

The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/2867223003/#ps100001
(title: "Fix race.")

[commit-bot: I haz the power, 2017-05-10 23:51:34]

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-11 05:55:07]

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

[commit-bot: I haz the power, 2017-05-11 05:55:08]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Jamie, 2017-05-11 16:24:56]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2017-05-11 16:25:53]

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-11 16:31:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494519896710180,
"parent_rev": "b125c58e21baa8d69246459e842737f0e87fc1a5", "commit_rev":
"939bac656fd600a7accbb468c41f30cd858394ec"}

[commit-bot: I haz the power, 2017-05-11 16:34:23]

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

[commit-bot: I haz the power, 2017-05-11 16:34:26]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[Jamie, 2017-05-11 17:00:31]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2017-05-11 17:01:27]

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-11 17:11:03]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494522031999690,
"parent_rev": "6054fad43db3d76dfd926601c3be4e5d284d6d70", "commit_rev":
"c96bfeee5db953d0c7a5e2d636f2622323b61f4f"}

[commit-bot: I haz the power, 2017-05-11 17:11:17]

Description was changed from

==========
Send a message to the client if bad It2Me policies are read.

BUG=433009
==========

to

==========
Send a message to the client if bad It2Me policies are read.

BUG=433009

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

[commit-bot: I haz the power, 2017-05-11 17:11:18]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c96bfeee5db953d0c7a5e2d636f2...