[mojo] Change error behaviour for incoming message processing [chromium/src : main]
Fred Shih (Gerrit)
Fred Shih added 1 comment![]( "Open in Gerrit")
oops, sorry, just realized I accidentally made a bunch of submodule changes when I rebased..
Related details
- Alex Gough
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Gough (Gerrit)
Alex Gough added 6 comments![]( "Open in Gerrit")
this seems sensible but also feels quite risky as we can't run an experiment - what changes to crashes might we expect and do you have some queries ready to look at crashes in android and webview after this goes in?
if (decoder0 == null) {
return null;
}Alex GoughNo idea why we even allow this. Is there a use case where we'd want decode to return null because a null decoder was passed in?
no idea!
* Indicates an error that occurred at the mojo transport layer. These errors are not caused by the
* user. Instead, this exception indicates some sort of channel corruption or a code defect. When
* this exception is thrown, we can no longer make any assumptions about the validity of the
* underlying channel.this is sort of confusing - these represent messages that fail validation which can be a user error but in a different process?
// TODO(crbug.com/469861566): The exception handler should close the pipe in this
// case. The current impl does not really handle this, so we'll just maintain the
// current behaviour to limit the blast radius of this CL.reword to not talk about 'this CL' this will be in the code
can we dumpwithoutcrashing here?
// Rewrap the BadMessageException to conform with existing
// behaviour.comment is confusing reword to explain why it happens /now/ or TODO what would be an improvement (or just the comment above is enough)
// Rewrap the BadMessageException to conform with existing
// behaviour.ditto
Related details
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 6 comments![]( "Open in Gerrit")
this seems sensible but also feels quite risky as we can't run an experiment - what changes to crashes might we expect and do you have some queries ready to look at crashes in android and webview after this goes in?
tbh -- I can't really of any way to get more metrics for this change besides keeping a close eye at the webview and android crash reports. Unfortunately there just isn't any infra regarding error rates for the java binding. We should probably change that.
That being said, we did get sorta lucky that old implementation catches RuntimeException (which is a big nono). This CL adds a checked exception, so if anything, we should be reducing the amount of places we will be encountering unexpected exceptions...
I added Sky to take a look at this CL -- maybe he'll have some ideas.
if (decoder0 == null) {
return null;
}Alex GoughNo idea why we even allow this. Is there a use case where we'd want decode to return null because a null decoder was passed in?
no idea!
welp, I'll open this can of worms later on down the road!
* Indicates an error that occurred at the mojo transport layer. These errors are not caused by the
* user. Instead, this exception indicates some sort of channel corruption or a code defect. When
* this exception is thrown, we can no longer make any assumptions about the validity of the
* underlying channel.this is sort of confusing - these represent messages that fail validation which can be a user error but in a different process?
you're right. When I came up with this exception, I was originally thinking of bad messages coming from the mojo runtime. But a client could also create a bad message. Updated the comment.
// TODO(crbug.com/469861566): The exception handler should close the pipe in this
// case. The current impl does not really handle this, so we'll just maintain the
// current behaviour to limit the blast radius of this CL.reword to not talk about 'this CL' this will be in the code
can we dumpwithoutcrashing here?
done -- I did try this, but this turned out to be way more difficult than I thought.
But unfortunately all the crash components depend on mojo at some point, so we will probably have to roll our own crash reporter (or refactor their build if possible..).
// Rewrap the BadMessageException to conform with existing
// behaviour.comment is confusing reword to explain why it happens /now/ or TODO what would be an improvement (or just the comment above is enough)
Done
// Rewrap the BadMessageException to conform with existing
// behaviour.Fred Shihditto
Done
Related details
- Alex Gough
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 1 comment![]( "Open in Gerrit")
Fred Shihthis seems sensible but also feels quite risky as we can't run an experiment - what changes to crashes might we expect and do you have some queries ready to look at crashes in android and webview after this goes in?
tbh -- I can't really of any way to get more metrics for this change besides keeping a close eye at the webview and android crash reports. Unfortunately there just isn't any infra regarding error rates for the java binding. We should probably change that.
That being said, we did get sorta lucky that old implementation catches RuntimeException (which is a big nono). This CL adds a checked exception, so if anything, we should be reducing the amount of places we will be encountering unexpected exceptions...
I added Sky to take a look at this CL -- maybe he'll have some ideas.
oh and to be more clear, I don't expect any additional crashes that weren't already present. If anything, I'd expect a reduction in crashes because this CL caught some client code which could've triggered some unhandled exceptions.
Related details
- Alex Gough
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Malice (Gerrit)
Sky Malice voted and added 7 comments![]( "Open in Gerrit")
Votes added by Sky Malice
| Code-Review | +1 |
7 comments
I don't use mojo bindings so I may be missing some context of what's going on here. Closing the pipe and matching C++ behavior sounds great. I'm a bit surprised by the switch from the runtime exception to the checked exception, it feels somewhat hostile to clients. So often clients don't really care and assume the happy case. For the fraction that want to handle errors, they could still catch specific exceptions that extend RuntimeException, right?
Additionally, if you didn't change the checked-ness of the exception, this CL would be much smaller. And there may actually be some small pieces in RouterImpl#dispatchMessages or something that you could consider feature gating two versions of the behavior. I'm not exactly sure.
I don't know enough to be concerned here, this all seems mostly reasonable to me though.
} catch (BadMessageException e) {
throw new IllegalStateException(e);So many call sites are catching the checked exception and just wrapping with an unchecked exception. Does it really make sense for deserialize to have to throw a checked exception?
} catch (org.chromium.mojo.bindings.DeserializationException e) {This is odd that we're fully qualifying the exception here, I guess it doesn't really matter.
} catch (BadMessageException e) {I would have excepted this to come before RuntimeException, the BME feels much more narrowly scoped. And if you ever make it extend RuntimeException, we'd probably want its specific handling to be run.
// TODO(crbug.com/469861566?): this is not correct, because it swallows anyWhat does this `?` here mean?
// Needed to match existing behaviour before |BadMessageException|
// was introduced.I don't understand this. Why do you need to match the old behavior? The whole client is being updated in this CL. What do I not understand?
private static enum DispatchResult {Does this strictly need to be an enum? Typically we use @IntDef for the smaller overhead.
Related details
- Alex Gough
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 8 comments![]( "Open in Gerrit")
I don't use mojo bindings so I may be missing some context of what's going on here. Closing the pipe and matching C++ behavior sounds great. I'm a bit surprised by the switch from the runtime exception to the checked exception, it feels somewhat hostile to clients. So often clients don't really care and assume the happy case. For the fraction that want to handle errors, they could still catch specific exceptions that extend RuntimeException, right?
Additionally, if you didn't change the checked-ness of the exception, this CL would be much smaller. And there may actually be some small pieces in RouterImpl#dispatchMessages or something that you could consider feature gating two versions of the behavior. I'm not exactly sure.
I don't know enough to be concerned here, this all seems mostly reasonable to me though.
Thanks for taking a look! It's always nice to have someone without any context to go over the code for some objective feedback :)
Regarding the checked exception: it's not really exposed to clients. Most of the checked exception *should* stay within the mojo runtime. Unfortunately the current implementation doesn't really do a great job separating mojo internal code and client facing external code. This is where we end up with unexpected client library behaviours (e.g.: the only function that checks if an enum value is valid will throw exceptions instead of returning boolean).
Having checked exception in the mojo runtime also ensures that we're able to propagate pipe closing errors more effectively (and ensured that they're handled). Without the checked exception, most of the runtime code just uses a boolean return to signal that something has gone wrong. At some places it turns into a RuntimeException when a method actually needs to return something. In order to get around this, the current implementation has a catch-them-all handler in the Connector. The really nasty side effect of this is that this will *also* swallow any exception thrown by the client implementation.
} catch (BadMessageException e) {
throw new IllegalStateException(e);So many call sites are catching the checked exception and just wrapping with an unchecked exception. Does it really make sense for deserialize to have to throw a checked exception?
I mulled about this for a while (I mulled about this before I made the initial change too, lol). In the end I was inclined to agree with you. It would have been nice if we had an internal and public deserialization signatures, but at this point I think Deserialization is widely used enough that it probably doesn't make sense to change it.
I changed DeserializationException back to a RuntimeException and added an UnmarshalException that is intended to be used by the mojo runtime internally. It's not ideal, but probably the most reasonable approach at this point.
That allowed me to revert a bunch of changes. I still think it's funky to call validateEnum to check whether an enum value is valid, but that can be cleaned up in a separate CL..
} catch (org.chromium.mojo.bindings.DeserializationException e) {This is odd that we're fully qualifying the exception here, I guess it doesn't really matter.
ended up reverting this file
I would have excepted this to come before RuntimeException, the BME feels much more narrowly scoped. And if you ever make it extend RuntimeException, we'd probably want its specific handling to be run.
Done
// TODO(crbug.com/469861566?): this is not correct, because it swallows anyWhat does this `?` here mean?
most likely a copy paste error
// Needed to match existing behaviour before |BadMessageException|
// was introduced.I don't understand this. Why do you need to match the old behavior? The whole client is being updated in this CL. What do I not understand?
The correct behaviour in this case is to close the pipe then rethrow the exception, because all BadMessages should cause a pipe closure.
Blindly letting the exception propagate would lead to UB if an upper layer happens to catch the exception.
I'll make the change in a later CL, I'm attempting to limit the scope of this change (which is already way too yuge at this point...). Does that make sense?
Does this strictly need to be an enum? Typically we use @IntDef for the smaller overhead.
- Alex Gough
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 1 comment![]( "Open in Gerrit")
Fred Shihthis seems sensible but also feels quite risky as we can't run an experiment - what changes to crashes might we expect and do you have some queries ready to look at crashes in android and webview after this goes in?
Fred Shihtbh -- I can't really of any way to get more metrics for this change besides keeping a close eye at the webview and android crash reports. Unfortunately there just isn't any infra regarding error rates for the java binding. We should probably change that.
That being said, we did get sorta lucky that old implementation catches RuntimeException (which is a big nono). This CL adds a checked exception, so if anything, we should be reducing the amount of places we will be encountering unexpected exceptions...
I added Sky to take a look at this CL -- maybe he'll have some ideas.
oh and to be more clear, I don't expect any additional crashes that weren't already present. If anything, I'd expect a reduction in crashes because this CL caught some client code which could've triggered some unhandled exceptions.
Ok, new day and I am slightly less dumb than I was yesterday. I have a crash report query that will show all mojo bindings exceptions, here:
Turns out we actually have a few exceptions that have been lurking. Interestingly, they are serialization exceptions. I manually induced a crash, a report can be seen here:
http://go/crash/cd51739d4acd9748
To send reports, we can use JavExceptionReporter:
https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/JavaExceptionReporter.java?q=javaexceptionreporter
This is installed by the JNI and will capture all unhandled exceptions in Java and upload it to crashpad. This works for both Android Chrome and Android Webview.
The numbers is quite low (0, so as low as it can get), so any regressino should be easy to spot.
Does that sound reasonable to you?
Related details
- Alex Gough
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Gough (Gerrit)
Alex Gough voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alex Gough
| Code-Review | +1 |
Fred Shihthis seems sensible but also feels quite risky as we can't run an experiment - what changes to crashes might we expect and do you have some queries ready to look at crashes in android and webview after this goes in?
Fred Shihtbh -- I can't really of any way to get more metrics for this change besides keeping a close eye at the webview and android crash reports. Unfortunately there just isn't any infra regarding error rates for the java binding. We should probably change that.
That being said, we did get sorta lucky that old implementation catches RuntimeException (which is a big nono). This CL adds a checked exception, so if anything, we should be reducing the amount of places we will be encountering unexpected exceptions...
I added Sky to take a look at this CL -- maybe he'll have some ideas.
Fred Shihoh and to be more clear, I don't expect any additional crashes that weren't already present. If anything, I'd expect a reduction in crashes because this CL caught some client code which could've triggered some unhandled exceptions.
Ok, new day and I am slightly less dumb than I was yesterday. I have a crash report query that will show all mojo bindings exceptions, here:
Turns out we actually have a few exceptions that have been lurking. Interestingly, they are serialization exceptions. I manually induced a crash, a report can be seen here:
http://go/crash/cd51739d4acd9748
To send reports, we can use JavExceptionReporter:
https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/JavaExceptionReporter.java?q=javaexceptionreporterThis is installed by the JNI and will capture all unhandled exceptions in Java and upload it to crashpad. This works for both Android Chrome and Android Webview.
The numbers is quite low (0, so as low as it can get), so any regressino should be easy to spot.
Does that sound reasonable to you?
Yep thanks for digging into this - I'm happy once remaining comments are resolved.
Related details
- Fred Shih
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sky Malice (Gerrit)
Sky Malice voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sky Malice
| Code-Review | +1 |
2 comments
I don't use mojo bindings so I may be missing some context of what's going on here. Closing the pipe and matching C++ behavior sounds great. I'm a bit surprised by the switch from the runtime exception to the checked exception, it feels somewhat hostile to clients. So often clients don't really care and assume the happy case. For the fraction that want to handle errors, they could still catch specific exceptions that extend RuntimeException, right?
Additionally, if you didn't change the checked-ness of the exception, this CL would be much smaller. And there may actually be some small pieces in RouterImpl#dispatchMessages or something that you could consider feature gating two versions of the behavior. I'm not exactly sure.
I don't know enough to be concerned here, this all seems mostly reasonable to me though.
Thanks for taking a look! It's always nice to have someone without any context to go over the code for some objective feedback :)
Regarding the checked exception: it's not really exposed to clients. Most of the checked exception *should* stay within the mojo runtime. Unfortunately the current implementation doesn't really do a great job separating mojo internal code and client facing external code. This is where we end up with unexpected client library behaviours (e.g.: the only function that checks if an enum value is valid will throw exceptions instead of returning boolean).
Having checked exception in the mojo runtime also ensures that we're able to propagate pipe closing errors more effectively (and ensured that they're handled). Without the checked exception, most of the runtime code just uses a boolean return to signal that something has gone wrong. At some places it turns into a RuntimeException when a method actually needs to return something. In order to get around this, the current implementation has a catch-them-all handler in the Connector. The really nasty side effect of this is that this will *also* swallow any exception thrown by the client implementation.
also swallow any exception thrown by the client implementation.
Oh that is tricky, I see. That makes a lot of sense, thanks! Your changes sound good to me.
// Needed to match existing behaviour before |BadMessageException|
// was introduced.Fred ShihI don't understand this. Why do you need to match the old behavior? The whole client is being updated in this CL. What do I not understand?
The correct behaviour in this case is to close the pipe then rethrow the exception, because all BadMessages should cause a pipe closure.
Blindly letting the exception propagate would lead to UB if an upper layer happens to catch the exception.
I'll make the change in a later CL, I'm attempting to limit the scope of this change (which is already way too yuge at this point...). Does that make sense?
Thanks for the detailed explanation! Yes, makes more sense now.
Related details
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fred Shih
| Commit-Queue | +2 |
1 comment
Thank you for the reviews! I learned quite a bit. Sorry this became such a monster..
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |