Mojo: add support for disallowing sync calls for a process.

Mojo: add support for disallowing sync calls for a process.

This CLs introduce a new system API to expose the configuration whether sync calls are allowed to mojo apps.
The bindings check this configuration before processing a sync call (if DCHECK is enabled).

This CL also disables sync calls for the browser process.

BUG=620733
Committed: https://crrev.com/519639829bd17eaae4b147f7a96cbd1406fc6244
Cr-Commit-Position: refs/heads/master@{#401076}

[yzshen1, 2016-06-21 16:45:41]

yzshen@chromium.org changed reviewers:
+ jam@chromium.org, rockot@chromium.org

[yzshen1, 2016-06-21 16:45:42]

Hi, Ken and John.

Would you please take a look? Thanks!

https://codereview.chromium.org/2084993002/diff/1/mojo/edk/system/core.h
File mojo/edk/system/core.h (right):

https://codereview.chromium.org/2084993002/diff/1/mojo/edk/system/core.h#newc...
mojo/edk/system/core.h:307: base::Lock property_lock_;
Ken: Do you think this is okay?

Alternatives are:
1) use an existing lock in this class; or
2) don't lock; explicitly require that SetProperty() can only be done prior to
any GetProperty() calls (and somehow insert a memory fence, not sure what is the
best practice in chrome to do that).

WDYT?

https://codereview.chromium.org/2084993002/diff/1/mojo/public/cpp/bindings/sy...
File mojo/public/cpp/bindings/sync_call_restrictions.h (right):

https://codereview.chromium.org/2084993002/diff/1/mojo/public/cpp/bindings/sy...
mojo/public/cpp/bindings/sync_call_restrictions.h:43: // DO NOT ADD ANY OTHER
FRIEND STATEMENTS, talk to jam first.
John: do you think this comment is okay? (I think you might want to restrict who
can approve sync calls. But happy to put myself or others here if you don't have
time to do this.)

[jam, 2016-06-21 17:00:33]

lgtm

https://codereview.chromium.org/2084993002/diff/1/mojo/public/cpp/bindings/sy...
File mojo/public/cpp/bindings/sync_call_restrictions.h (right):

https://codereview.chromium.org/2084993002/diff/1/mojo/public/cpp/bindings/sy...
mojo/public/cpp/bindings/sync_call_restrictions.h:43: // DO NOT ADD ANY OTHER
FRIEND STATEMENTS, talk to jam first.
On 2016/06/21 16:45:41, yzshen1 wrote:
> John: do you think this comment is okay? (I think you might want to restrict
who
> can approve sync calls. But happy to put myself or others here if you don't
have
> time to do this.)

mojo/owners is fine (i.e. replace jam with mojo/OWNERS). please email the rest
of the owners to give them a heads up to know about this and to make sure that
they don't let through any instances unless it maps to existing allowed cases
(i.e. a few gpu process calls, but never to any other child process).

also please update the comment to say that in the browser process we don't want
any sync calls to child processes for performance, security and stability
reasons.

[yzshen1, 2016-06-21 17:35:50]

https://codereview.chromium.org/2084993002/diff/1/mojo/public/cpp/bindings/sy...
File mojo/public/cpp/bindings/sync_call_restrictions.h (right):

https://codereview.chromium.org/2084993002/diff/1/mojo/public/cpp/bindings/sy...
mojo/public/cpp/bindings/sync_call_restrictions.h:43: // DO NOT ADD ANY OTHER
FRIEND STATEMENTS, talk to jam first.
Done. Thanks!

[Ken Rockot(use gerrit already), 2016-06-21 17:51:12]

lgtm

https://codereview.chromium.org/2084993002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/sync_call_restrictions.cc (right):

https://codereview.chromium.org/2084993002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/sync_call_restrictions.cc:71: LOG(FATAL) << "Mojo
sync calls are not allowed to prevent jank and "
nit: This can also read as if mojo sync calls may prevent jank and deadlock, but
we don't allow them to do so. Which is nonsensical, but still parses.
Suggestion:

"Mojo sync calls are not allowed because they can lead to jank and deadlock. If
you must make an exception, please see SyncCallRestrictions::ScopedAllowSyncCall
and consult mojo/OWNERS."

[yzshen1, 2016-06-21 18:07:18]

On 2016/06/21 17:51:12, Ken Rockot wrote:
> lgtm
> 
>
https://codereview.chromium.org/2084993002/diff/20001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/sync_call_restrictions.cc (right):
> 
>
https://codereview.chromium.org/2084993002/diff/20001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/sync_call_restrictions.cc:71: LOG(FATAL) << "Mojo
> sync calls are not allowed to prevent jank and "
> nit: This can also read as if mojo sync calls may prevent jank and deadlock,
but
> we don't allow them to do so. Which is nonsensical, but still parses.
> Suggestion:
> 
> "Mojo sync calls are not allowed because they can lead to jank and deadlock.
If
> you must make an exception, please see
SyncCallRestrictions::ScopedAllowSyncCall
> and consult mojo/OWNERS."

This looks much better. Done.

[yzshen1, 2016-06-21 18:07:24]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-06-21 18:07:25]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2084993002/#ps40001
(title: ".")

[commit-bot: I haz the power, 2016-06-21 18:08:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084993002/40001

[commit-bot: I haz the power, 2016-06-21 19:37:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-21 19:41:50]

Description was changed from

==========
Mojo: add support for disallowing sync calls for a process.

This CLs introduce a new system API to expose the configuration whether sync
calls are allowed to mojo apps.
The bindings check this configuration before processing a sync call (if DCHECK
is enabled).

This CL also disables sync calls for the browser process.

BUG=620733
==========

to

==========
Mojo: add support for disallowing sync calls for a process.

This CLs introduce a new system API to expose the configuration whether sync
calls are allowed to mojo apps.
The bindings check this configuration before processing a sync call (if DCHECK
is enabled).

This CL also disables sync calls for the browser process.

BUG=620733

Committed: https://crrev.com/519639829bd17eaae4b147f7a96cbd1406fc6244
Cr-Commit-Position: refs/heads/master@{#401076}
==========

[commit-bot: I haz the power, 2016-06-21 19:41:51]

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