Re: [chrome-mojo] Some questions about error handling in Mojo

[Ken Rockot]

-chrome-mojo, +chromium-mojo

On Apr 19, 2017 8:06 AM, "Fabio Tirelo" <fti...@google.com> wrote:

Hi Chrome Mojo team,

I have a few questions about error handling in Mojo and I would appreciate if you can help me answer them.

1. About error handling in PendingChildProcess::Connect():

    a. In what circumstances will the |error_callback| be run?

When you explicitly report a bad message (using mojo::ReportBadMessage) and it's determined internally that the message came from that remote process. That's the only reason we call it in practice today, but there may be other conceivable reasons in the future. It should be interpreted to mean that the process behaved in a way we didn't expect and which may or may not be recoverable.

    b. Is there any recommended way of testing connection failures handled by that function? I'm testing my feature on a browser test using MULTIPROCESS_TEST_MAIN (Chrome is the parent process that defines the Impl object).

Have the child process send any message over a message pipe to the parent. In the parent-side message handler, call mojo::ReportBadMessage. This will cause the callback to be invoked for the child.

2. How to handle cases of invalid data format/versioning on the parent process side (which contains the Impl object)? Should this be done handled by a callback passed using set_connection_error_handler()? If so, how to differentiate from the error received when the other end disconnects?

If you actually receive a message which is invalid according to the mojom interface receiving it, the message will fail validation, sever the pipe, and invoke the set_connection_error_handler callback.

If the message fails some custom validation logic in your implementation -- i.e. some constraint that is not expressible in mojom or StructTraits deserialization -- the correct thing to do is call mojo::ReportBadMessage and close the pipe yourself.

Thanks,

--
You received this message because you are subscribed to the Google Groups "chrome-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chrome-mojo+unsubscribe@google.com.
To post to this group, send email to chrom...@google.com.
To view this discussion on the web visit https://groups.google.com/a/google.com/d/msgid/chrome-mojo/CAA1nPEGGS%3Dxk2EodiX3v09mPY5WoVTKgGHC36Ac5ENv38nniYA%40mail.gmail.com.

[Joe Mason]

Resending from the correct account...

I have a followup question (inline):

On Wed, Apr 19, 2017 at 12:54 PM, Ken Rockot <roc...@chromium.org> wrote:

If you actually receive a message which is invalid according to the mojom interface receiving it, the message will fail validation, sever the pipe, and invoke the set_connection_error_handler callback.

If I'm using an associated interface on the same pipe, and I have a separate error handler on each interface, how does it know which will get called if there's an invalid message? (Say a message which is completely garbled so there's no way to tell which interface it's intended for.)

Aside: is there any problem using two associated interfaces on the same pipe, going in the same direction? Eg.

interface EventsForOperationA {

  Event1(params);

  Event2(params);

}

interface EventsForOperationB {

  Event1(params);

  Event2(params);

}

// Invokes an operation and sends events back to the parent as they happen.

interface Operations {

  DoOperationA(associated EventsForOperationA events);

  DoOperationB(associated EventsForOperationB events);

}

[Yuzhu Shen]

On Fri, Apr 28, 2017 at 10:42 AM, Joe Mason <joenot...@chromium.org> wrote:

Resending from the correct account...

I have a followup question (inline):

On Wed, Apr 19, 2017 at 12:54 PM, Ken Rockot <roc...@chromium.org> wrote:

If you actually receive a message which is invalid according to the mojom interface receiving it, the message will fail validation, sever the pipe, and invoke the set_connection_error_handler callback.

If I'm using an associated interface on the same pipe, and I have a separate error handler on each interface, how does it know which will get called if there's an invalid message? (Say a message which is completely garbled so there's no way to tell which interface it's intended for.)

The bad message causes message pipe disconnection and fire error handlers for all interfaces on that pipe.

 

Aside: is there any problem using two associated interfaces on the same pipe, going in the same direction? Eg.

No problem.

 

interface EventsForOperationA {

  Event1(params);

  Event2(params);

}

interface EventsForOperationB {

  Event1(params);

  Event2(params);

}

// Invokes an operation and sends events back to the parent as they happen.

interface Operations {

  DoOperationA(associated EventsForOperationA events);

  DoOperationB(associated EventsForOperationB events);

}

--
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-mojo+unsubscribe@chromium.org.
To post to this group, send email to chromi...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAH%3DT95SUqomSJCsziEYnH0vsaMMiN1Y7o83dE-CuQF-APeo1aA%40mail.gmail.com.

[leon...@intel.com]

Hi,

When we need to report a bad message inside implementation of a mojo interface method(under //content/browser), should we prefer using mojo::ReportBadMessage() rather than content::bad_message::ReceivedBadMessage()? Would you please help to elaborate more about difference between their behaviors/usages?

And, if we're using content::bad_message::ReceivedBadMessage(), in test environments we can override the filter's ShutdownForBadMessage() function to intercept the bad message report so that we can check later whether a bad message has been reported just as expected. A sample is here: https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_handle_unittest.cc?rcl=1de4db070869cd5b1450258b94794d9015089e9d&l=58

Then, if we changed to use mojo::ReportBadMessage(), is there a similar mechanism like this?

Thanks!

On Thursday, April 20, 2017 at 12:54:32 AM UTC+8, Ken Rockot wrote:

-chrome-mojo, +chromium-mojo

On Apr 19, 2017 8:06 AM, "Fabio Tirelo" <fti...@google.com> wrote:

Hi Chrome Mojo team,

I have a few questions about error handling in Mojo and I would appreciate if you can help me answer them.

1. About error handling in PendingChildProcess::Connect():

    a. In what circumstances will the |error_callback| be run?

When you explicitly report a bad message (using mojo::ReportBadMessage) and it's determined internally that the message came from that remote process. That's the only reason we call it in practice today, but there may be other conceivable reasons in the future. It should be interpreted to mean that the process behaved in a way we didn't expect and which may or may not be recoverable.

    b. Is there any recommended way of testing connection failures handled by that function? I'm testing my feature on a browser test using MULTIPROCESS_TEST_MAIN (Chrome is the parent process that defines the Impl object).

Have the child process send any message over a message pipe to the parent. In the parent-side message handler, call mojo::ReportBadMessage. This will cause the callback to be invoked for the child.

2. How to handle cases of invalid data format/versioning on the parent process side (which contains the Impl object)? Should this be done handled by a callback passed using set_connection_error_handler()? If so, how to differentiate from the error received when the other end disconnects?

If you actually receive a message which is invalid according to the mojom interface receiving it, the message will fail validation, sever the pipe, and invoke the set_connection_error_handler callback.

If the message fails some custom validation logic in your implementation -- i.e. some constraint that is not expressible in mojom or StructTraits deserialization -- the correct thing to do is call mojo::ReportBadMessage and close the pipe yourself.

Thanks,

--
You received this message because you are subscribed to the Google Groups "chrome-mojo" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chrome-mojo...@google.com.

[Ken Rockot]

On Tue, Aug 29, 2017 at 11:25 PM, <leon...@intel.com> wrote:

Hi,

When we need to report a bad message inside implementation of a mojo interface method(under //content/browser), should we prefer using mojo::ReportBadMessage() rather than content::bad_message::ReceivedBadMessage()? Would you please help to elaborate more about difference between their behaviors/usages?

There is ongoing discussion at http://crbug.com/756115, but please consider ReceivedBadMessage to be deprecated when using Mojom bindings.

Main advantages of mojo::ReportBadMessage are:

• It provides more context in the crash reports it generates, by way of a free-form crash key string which is often provided by generated bindings validation code. ReceivedBadMessage on the other hand predates Mojo, so its reports lack any Mojo-related context.
• It works in any service context, not just browser code referring to renderer messages. Mojo embedders will ultimately have flexibility to control how the system responds to reports within one process about messages from another process, while callers of mojo::ReportBadMessage never have to reason about process identity on either end of a pipe.

 Main disadvantage of mojo::ReportBadMessage is that it lacks a way to disambiguate magic signatures for different crash reasons. The simple plan to address this is to affix magic signatures with a hash of the Mojo error message crash key, since these are generally expected to be unique per bug.

And, if we're using content::bad_message::ReceivedBadMessage(), in test environments we can override the filter's ShutdownForBadMessage() function to intercept the bad message report so that we can check later whether a bad message has been reported just as expected. A sample is here: https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_handle_unittest.cc?rcl=1de4db070869cd5b1450258b94794d9015089e9d&l=58

Then, if we changed to use mojo::ReportBadMessage(), is there a similar mechanism like this?

There is mojo::edk::SetDefaultProcessErrorCallback for handling ReportBadMessage calls in the otherwise unhandled case.

In production today, we do set an explicit error callback on browser-renderer process connections, so ReportBadMessage on messages from a renderer will not hit the default error callback. We'd need to add an override mechanism to RenderProcessHost if we wanted to make intercepting such errors possible.

Thanks!

On Thursday, April 20, 2017 at 12:54:32 AM UTC+8, Ken Rockot wrote:

-chrome-mojo, +chromium-mojo

On Apr 19, 2017 8:06 AM, "Fabio Tirelo" <fti...@google.com> wrote:

Hi Chrome Mojo team,

I have a few questions about error handling in Mojo and I would appreciate if you can help me answer them.

1. About error handling in PendingChildProcess::Connect():

    a. In what circumstances will the |error_callback| be run?

When you explicitly report a bad message (using mojo::ReportBadMessage) and it's determined internally that the message came from that remote process. That's the only reason we call it in practice today, but there may be other conceivable reasons in the future. It should be interpreted to mean that the process behaved in a way we didn't expect and which may or may not be recoverable.

    b. Is there any recommended way of testing connection failures handled by that function? I'm testing my feature on a browser test using MULTIPROCESS_TEST_MAIN (Chrome is the parent process that defines the Impl object).

Have the child process send any message over a message pipe to the parent. In the parent-side message handler, call mojo::ReportBadMessage. This will cause the callback to be invoked for the child.

2. How to handle cases of invalid data format/versioning on the parent process side (which contains the Impl object)? Should this be done handled by a callback passed using set_connection_error_handler()? If so, how to differentiate from the error received when the other end disconnects?

If you actually receive a message which is invalid according to the mojom interface receiving it, the message will fail validation, sever the pipe, and invoke the set_connection_error_handler callback.

If the message fails some custom validation logic in your implementation -- i.e. some constraint that is not expressible in mojom or StructTraits deserialization -- the correct thing to do is call mojo::ReportBadMessage and close the pipe yourself.

Thanks,

--
You received this message because you are subscribed to the Google Groups "chrome-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chrome-mojo...@google.com.
To post to this group, send email to chrom...@google.com.
To view this discussion on the web visit https://groups.google.com/a/google.com/d/msgid/chrome-mojo/CAA1nPEGGS%3Dxk2EodiX3v09mPY5WoVTKgGHC36Ac5ENv38nniYA%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-mojo+unsubscribe@chromium.org.
To post to this group, send email to chromi...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/397e12ac-e549-4fea-af73-6eccb7c9e836%40chromium.org.

[Han, Leon]

Hi, Ken,

Big thanks for all the helpful information sharing!

For our case it's no problem that we can use mojo::edk::SetDefaultProcessErrorCallback to intercept output of mojo::ReportBadMessage well, because in unit test world RenderProcessHost does not set the capture error callback at all.

BR,
Han Leon

---

From: roc...@google.com [roc...@google.com] on behalf of Ken Rockot [roc...@chromium.org]
Sent: Wednesday, August 30, 2017 11:03 PM
To: Han, Leon
Cc: chromium-mojo; Fabio Tirelo; Yuzhu Shen
Subject: Re: [chrome-mojo] Some questions about error handling in Mojo

[Matt Falkenhagen]

Is there a recommended pattern about what to do with the Mojo callback when using mojo::ReportBadMessage()?

If we don't run the callback, Mojo seems to DCHECK. Currently our code looks like this which seems silly:

  mojo::ReportBadMessage("kill renderer");

  // We still have to run the callback. Just give nonsense arguments.

  std::move(callback)::Run(error, nonsense, nonsense, nonsense);

An example is ServiceWorkerProviderHost::Register().

In our case, the browser-side host instance lifetime is tied to the binding with the renderer: when the renderer dies the connection error handler destroys the host instance. Instead of running the callback, we could close the binding, but that doesn't run the error handler. So we originally decided it'd be cleaner to just let the renderer die via ReportBadMessage() and wait for the binding to break as normal, but it's leading to bloaty code either keeping the callback around or running it with useless arguments.

Is there a recommended pattern here? (Having written this, kinda feels like we should just be closing the binding and running the error handler manually, is that what most code does?).

[Ken Rockot]

On Thu, Sep 7, 2017 at 9:41 PM, Matt Falkenhagen <fal...@chromium.org> wrote:

Is there a recommended pattern about what to do with the Mojo callback when using mojo::ReportBadMessage()?

The recommended pattern is normally to drop the callback.

If we don't run the callback, Mojo seems to DCHECK. Currently our code looks like this which seems silly:

Right, it will DCHECK if the binding is still open. If you want to drop the callback, just close the binding first.

 

  mojo::ReportBadMessage("kill renderer");

  // We still have to run the callback. Just give nonsense arguments.

  std::move(callback)::Run(error, nonsense, nonsense, nonsense); 

An example is ServiceWorkerProviderHost::Register().

In our case, the browser-side host instance lifetime is tied to the binding with the renderer: when the renderer dies the connection error handler destroys the host instance. Instead of running the callback, we could close the binding, but that doesn't run the error handler. So we originally decided it'd be cleaner to just let the renderer die via ReportBadMessage() and wait for the binding to break as normal, but it's leading to bloaty code either keeping the callback around or running it with useless arguments.

Is there a recommended pattern here? (Having written this, kinda feels like we should just be closing the binding and running the error handler manually, is that what most code does?).

Yeah, sounds like you've got it :)

I've thought about how we could make this more straightforward.

Note that you can also call the ReportBadMessage() method on your Binding or BindingSet and this will automatically close the Binding (or remove that bad receiver from the set) in addition to reporting the bad message. Perhaps we could make those methods also invoke the error handler if set.