Mojo C++ Bindings: Add helpers for bad message reporting

Mojo C++ Bindings: Add helpers for bad message reporting

Adds new C++ API at the bindings layer for convenient reporting
of bad messages from within the stack frames that are actually
processing those messages.

Also adds a new EDK API to set a default process error handler.
This is currently only used to support testing but may be
generally useful for production code as well.

BUG=612500
R=yzshen@chromium.org

Committed: https://crrev.com/160577e9f267226ebd2c388e3bf1f8067484bbb2
Cr-Commit-Position: refs/heads/master@{#409612}

[Ken Rockot(use gerrit already), 2016-08-02 00:59:02]

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

[commit-bot: I haz the power, 2016-08-02 00:59:23]

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

[Ken Rockot(use gerrit already), 2016-08-02 00:59:35]

Hi Yuzhu, please take a look!

[commit-bot: I haz the power, 2016-08-02 01:02:59]

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

[commit-bot: I haz the power, 2016-08-02 01:03:00]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2016-08-02 01:04:42]

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

[commit-bot: I haz the power, 2016-08-02 01:04:57]

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, 2016-08-02 02:08:39]

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

[commit-bot: I haz the power, 2016-08-02 02:08:40]

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

[Ken Rockot(use gerrit already), 2016-08-02 02:53:31]

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

[commit-bot: I haz the power, 2016-08-02 02:53:43]

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, 2016-08-02 03:59:20]

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

[commit-bot: I haz the power, 2016-08-02 03:59:21]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-08-02 18:23:48]

On 2016/08/02 00:59:35, Ken Rockot wrote:
> Hi Yuzhu, please take a look!

Hi, Ken.


- Does it make sense to have something more light-way than the Messsage itself
as the argument of reporting message? Just the source information of a message.

- (I guess it should be an issue but would like to double check.) Is it
expensive to use tls here? It will be called for every message. Besides, when no
messages the stack will be deleted. That will likely happen very frequently
because most of the messages are dispatched from the message loop, i.e, they are
the only message in the stack.

[yzshen1, 2016-08-02 18:25:00]

On 2016/08/02 18:23:48, yzshen1 wrote:
> On 2016/08/02 00:59:35, Ken Rockot wrote:
> > Hi Yuzhu, please take a look!
> 
> Hi, Ken.
> 
> 
> - Does it make sense to have something more light-way than the Messsage itself
> as the argument of reporting message? Just the source information of a
message.
> 
> - (I guess it should be an issue but would like to double check.) Is it
> expensive to use tls here? It will be called for every message. Besides, when
no
> messages the stack will be deleted. That will likely happen very frequently
> because most of the messages are dispatched from the message loop, i.e, they
are
> the only message in the stack.

correction:  (I guess it should *not* be an issue but would like to double
check.)

[Ken Rockot(use gerrit already), 2016-08-02 19:01:11]

On 2016/08/02 at 18:23:48, yzshen wrote:
> On 2016/08/02 00:59:35, Ken Rockot wrote:
> > Hi Yuzhu, please take a look!
> 
> Hi, Ken.
> 
> 
> - Does it make sense to have something more light-way than the Messsage itself
as the argument of reporting message? Just the source information of a message.

I don't think I understand. Do you mean for the API (MojoNotifyBadMessage)
itself? In general someone reporting a bad message doesn't have any more
information other than the message which they want to reject.

> 
> - (I guess it should be an issue but would like to double check.) Is it
expensive to use tls here? It will be called for every message. Besides, when no
messages the stack will be deleted. That will likely happen very frequently
because most of the messages are dispatched from the message loop, i.e, they are
the only message in the stack.

Yeah, I think it might actually be too heavyweight. You're right that most of
the time we aren't going to be doing reentrant dispatch so the stack will very
frequently alloc and delete.

[yzshen1, 2016-08-02 19:16:34]

On 2016/08/02 19:01:11, Ken Rockot wrote:
> On 2016/08/02 at 18:23:48, yzshen wrote:
> > On 2016/08/02 00:59:35, Ken Rockot wrote:
> > > Hi Yuzhu, please take a look!
> > 
> > Hi, Ken.
> > 
> > 
> > - Does it make sense to have something more light-way than the Messsage
itself
> as the argument of reporting message? Just the source information of a
message.
> 
> I don't think I understand. Do you mean for the API (MojoNotifyBadMessage)
> itself? In general someone reporting a bad message doesn't have any more
> information other than the message which they want to reject.

The code stores mojo::Message instances, which contain ScopedMessageHandle plus
some other fields.
Maybe we could use ScopedMessageHandle instead?

> 
> > 
> > - (I guess it should be an issue but would like to double check.) Is it
> expensive to use tls here? It will be called for every message. Besides, when
no
> messages the stack will be deleted. That will likely happen very frequently
> because most of the messages are dispatched from the message loop, i.e, they
are
> the only message in the stack.
> 
> Yeah, I think it might actually be too heavyweight. You're right that most of
> the time we aren't going to be doing reentrant dispatch so the stack will very
> frequently alloc and delete.

[yzshen1, 2016-08-02 19:28:48]

On Tue, Aug 2, 2016 at 12:16 PM, <yzshen@chromium.org> wrote:

> On 2016/08/02 19:01:11, Ken Rockot wrote:
> > On 2016/08/02 at 18:23:48, yzshen wrote:
> > > On 2016/08/02 00:59:35, Ken Rockot wrote:
> > > > Hi Yuzhu, please take a look!
> > >
> > > Hi, Ken.
> > >
> > >
> > > - Does it make sense to have something more light-way than the Messsage
> itself
> > as the argument of reporting message? Just the source information of a
> message.
> >
> > I don't think I understand. Do you mean for the API
> (MojoNotifyBadMessage)
> > itself? In general someone reporting a bad message doesn't have any more
> > information other than the message which they want to reject.
>
> The code stores mojo::Message instances, which contain ScopedMessageHandle
> plus
> some other fields.
> Maybe we could use ScopedMessageHandle instead?
>
Given a second thought, I don't think it matters much. Please consider this
an optional comment.

>
>
> >
> > >
> > > - (I guess it should be an issue but would like to double check.) Is it
> > expensive to use tls here? It will be called for every message. Besides,
> when
> no
> > messages the stack will be deleted. That will likely happen very
> frequently
> > because most of the messages are dispatched from the message loop, i.e,
> they
> are
> > the only message in the stack.
> >
> > Yeah, I think it might actually be too heavyweight. You're right that
> most of
> > the time we aren't going to be doing reentrant dispatch so the stack
> will very
> > frequently alloc and delete.
>
>
>
> https://codereview.chromium.org/2202893002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-08-02 22:28:00]

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

[commit-bot: I haz the power, 2016-08-02 22:28:22]

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

[Ken Rockot(use gerrit already), 2016-08-02 22:30:54]

Please take another look. I've done something less stupid now. There's no need
to maintain a thread-local stack... rather the stack can hold context objects
which maintain a single thread-local pointer in ctor/dtor, just like the sync
response context. Duh.

I don't see how we can avoid using TLS here though. WDYT?

[commit-bot: I haz the power, 2016-08-02 23:37:46]

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

[commit-bot: I haz the power, 2016-08-02 23:37:47]

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

[yzshen1, 2016-08-03 17:10:57]

LGTM with a few nits.

https://codereview.chromium.org/2202893002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/message.cc (right):

https://codereview.chromium.org/2202893002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message.cc:165:
internal::MessageDispatchContext::current()->message()
Does it make sense to check whether we have a MessageDispatchContext before
dereferencing it? (here and below)

https://codereview.chromium.org/2202893002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message.cc:170: std::unique_ptr<Message>
new_message(new Message);
After taking the message, if someone tries to report bad message again, it will
crash in Message::NotifyBadMessage().

Maybe we could consider:
- making it a no-op; or
- making it share state with the ReportBadMessageCallback. (This enables
creating multiple callbacks too. But it may be a little overkill.)

WDYT?

https://codereview.chromium.org/2202893002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/report_bad_message_unittest.cc (right):

https://codereview.chromium.org/2202893002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/report_bad_message_unittest.cc:57:
std::vector<int32_t> received_values_;
This is not used.

[Ken Rockot(use gerrit already), 2016-08-03 19:13:18]

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

[Ken Rockot(use gerrit already), 2016-08-03 19:13:26]

Thanks!

https://codereview.chromium.org/2202893002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/message.cc (right):

https://codereview.chromium.org/2202893002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message.cc:165:
internal::MessageDispatchContext::current()->message()
On 2016/08/03 at 17:10:56, yzshen1 wrote:
> Does it make sense to check whether we have a MessageDispatchContext before
dereferencing it? (here and below)

Done. We should always have one, so I've made it a DCHECK

https://codereview.chromium.org/2202893002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/message.cc:170: std::unique_ptr<Message>
new_message(new Message);
On 2016/08/03 at 17:10:57, yzshen1 wrote:
> After taking the message, if someone tries to report bad message again, it
will crash in Message::NotifyBadMessage().
> 
> Maybe we could consider:
> - making it a no-op; or
> - making it share state with the ReportBadMessageCallback. (This enables
creating multiple callbacks too. But it may be a little overkill.)
> 
> WDYT?

Done - Callback is lazily created and takes ownership of the Message.
ReportBadMessage uses the callback.

https://codereview.chromium.org/2202893002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/report_bad_message_unittest.cc (right):

https://codereview.chromium.org/2202893002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/report_bad_message_unittest.cc:57:
std::vector<int32_t> received_values_;
On 2016/08/03 at 17:10:57, yzshen1 wrote:
> This is not used.

Removed

[commit-bot: I haz the power, 2016-08-03 19:13:55]

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

[Ken Rockot(use gerrit already), 2016-08-03 19:17:12]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-08-03 19:17:14]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-08-03 19:17:14]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2202893002/#ps70001
(title: ".")

[commit-bot: I haz the power, 2016-08-03 19:17:32]

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, 2016-08-03 20:25:06]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-08-03 20:27:45]

Description was changed from

==========
Mojo C++ Bindings: Add helpers for bad message reporting

Adds new C++ API at the bindings layer for convenient reporting
of bad messages from within the stack frames that are actually
processing those messages.

Also adds a new EDK API to set a default process error handler.
This is currently only used to support testing but may be
generally useful for production code as well.

BUG=612500
R=yzshen@chromium.org
==========

to

==========
Mojo C++ Bindings: Add helpers for bad message reporting

Adds new C++ API at the bindings layer for convenient reporting
of bad messages from within the stack frames that are actually
processing those messages.

Also adds a new EDK API to set a default process error handler.
This is currently only used to support testing but may be
generally useful for production code as well.

BUG=612500
R=yzshen@chromium.org

Committed: https://crrev.com/160577e9f267226ebd2c388e3bf1f8067484bbb2
Cr-Commit-Position: refs/heads/master@{#409612}
==========

[commit-bot: I haz the power, 2016-08-03 20:27:47]

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