-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unify unhandled error reporting, add PlatformDispatcher.onError #32078
Conversation
lib/ui/platform_dispatcher.dart
Outdated
/// After calling this method, the process or the VM may terminate. Some severe | ||
/// unhandled errors may not be able to call this method either, such as Dart | ||
/// compilation errors or process terminating errors. | ||
typedef ErrorCallback = bool Function(Object exception, StackTrace? stackTrace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hixie I've deviated from your design here - I think we just need a bool about whether the callback tried to handle the error or not. If an application wants to print anything, they can handle that themselves with print
or stderr.writeln
or whatever. Right now if this returns false, we retain existing behavior of writing the exception/stack to stderr (or logcat, or syslog, or whatever Fuchsia uses).
Added a stub for web and filed an issue to implement it separately. |
@@ -94,6 +94,7 @@ executable("dart_utils_unittests") { | |||
":utils", | |||
"$fuchsia_sdk_root/pkg:sys_inspect_cpp", | |||
"//flutter/testing", | |||
"//third_party/dart/runtime:libdart_jit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed because dart_errors.h now references isolate related symbols, which are not exposed by //third_party/dart/runtime:dart_api
by itself. Perhaps this dep should be in tonic's public deps, but that will make things pretty complicated because we'll have to make sure to select the right one. It's easier just to fix this target.
Is there a design doc somewhere that explains the overall view as it will be seen by Flutter developers? (like how this is planned to integrate with the FlutterError widget and so on) |
@Hixie - the one you wrote at https://flutter.dev/go/error-handling |
I meant the updated version since the PR says that doc is out of date. |
The main things that are out of date are that:
None of that affects the framework side of the recommendations in that doc AFAICT. |
No objection to the PR, but wondering if we should hold this until after upcoming branch cut. |
That's fine with me |
Cool, thanks for the clarification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't yet formed an opinion of wether this is a good idea or not. Here is just a code review of the code.
/// After calling this method, the process or the VM may terminate. Some severe | ||
/// unhandled errors may not be able to call this method either, such as Dart | ||
/// compilation errors or process terminating errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring doesn't seem placed in the right spot. It is referring to this as a method, but this is a typedef. This should be documented where it is used since the typedef itself is agnostic to how it is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's documented in both places. Would you like me to remove it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a personal preference, I wouldn't introduce a typedef at all.
Dart has first class Function type syntax, and I (very personally) find typedefs confusing. I see the UpperCamelCase name and expect a class, not a function type.
Consider just removing the type. Who would suffer from that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's documented in both places. Would you like me to remove it here?
I would if you don't take Irhn's suggestion. It can just reference the function that uses it instead of duplicating the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flutter's style is to prefer using typedefs - they benefit from having additional documentation and finding the right callsite, and being easier to read. I know this is a difference in what the Dart team prefers/linter suggests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/ui/platform_dispatcher.dart
Outdated
ErrorCallback? _onError; | ||
Zone? _onErrorZone; | ||
|
||
/// A callback that is invoked when an unhandled error occurs in Dart. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unhandled error where? From any isolate or just the root isolate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure how unhandled errors in child isolates get handled. For sure the root isolate. @mkustermann or @lrhn would know about cases for other isolates...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors in every isolate are handled the same:
- An uncaught error gets passed to the current zone's uncaught error handler.
- If the error reaches the root zone's uncaught error handler (by being in the same error zone, or because the child zone's uncaught error handler passes the error to its parent delegate), then
- Each error listener attached to the isolate (using
Isolate.addErrorListener
or the correspondingIsolate.spawn
argument) is sent a list of[error.toString(), stackTrace.toString()]
. If any. - If the isolate is set to have errors be fatal (default on VM, not on web, set using
Isolate.setErrorsFatal
or trough anIsolate.spawn
argument), the isolate dies. (Possibly sending a message to isolate onExit-listeners, but that's unrelated to the error).
So, where are you intercepting the errors? It seems to be at the point that the root zone's handleUncaughtError
calls to?
The "in Dart" in the description seems vacuous. If it isn't in Dart, what even is an "unhandled error"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for compute
, we forward errors back to the root isolate and rethrow them there. If a user does something else with spawning an isolate I'm not sure we need to cover that here.
Errors thrown in Dart as opposed to Java/Kotlin, Objective C/Swift, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, addErrorListener
doesn't work for our use case here since so many times the Dart code getting invoked is coming from C++ Dart_InvokeClosure
type calls, which don't end up bubbling exceptions up the same way in "dart code" (as opposed to the C++ embedding code).
@@ -10,18 +10,66 @@ | |||
namespace tonic { | |||
namespace DartError { | |||
const char kInvalidArgument[] = "Invalid argument."; | |||
DartPersistentValue on_error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to have DartPersistentValues as global variables? Should we instead attach this to the dart state? How does this handle multiple engines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to support multiple engines there would need to be a separate on_error
handler for each engine instance/isolate group.
I'd be interested in asking the Dart team about the safety of the current implementation within a single engine.
The on_error
closure will be set from the main isolate but will be invoked from whatever isolate is current during the call to CheckAndHandleError
.
The invocation itself should work given that both isolates will be in the same isolate group and the invoked closure will be part of the group's shared heap.
But I'd like to understand whether there is any risk in calling the same closure from multiple isolates (and possibly multiple concurrent threads) given that the on_error
handler calls user-provided code that could do arbitrary mutation of data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At one point I had this handled in the same place we set up the global hook persistent values - maybe if I move it there everything is happy again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkustermann @a-siva @mraleph might be able to help here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I'd like to understand whether there is any risk in calling the same closure from multiple isolates (and possibly multiple concurrent threads) given that the on_error handler calls user-provided code that could do arbitrary mutation of data.
Also, someone could be setting the handler on the UI thread while a different isolate is accessing it on on secondary isolate, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a test that I was sure would fail this (in the latest commit), and to my surprise it passed!
Here's what's up: what looks like a process (or maybe VM)-wide singleton is actually per-isolate. DartPersistentValue keeps a pointer to the isolate, and so even though you're accessing a single DartPersistentValue
, it's getting that persistetn value for the current isolate. This behavior is super weird and I don't want to rely on it, so I've refactored things to more explicitly use UIDartState::Current
in the reporter callback, and modified that to use the Dart_Handles directly so it all works the way it should.
@jason-simmons: If the current isolate throwing the error tries to access this, it will only get the reporter for its own PlatformDispatcher
. If it never set one, then exceptions will just get logged. If it did set one, then it will get the one that it set. The new test in shell_unittests.dart verifies this by spawning a second shell and making sure that the onError for each isolate can be used in an interleaved manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest version of this PR is checking UIDartState::Current()->platform_configuration()
before trying to invoke the user's on_error
callback. Only the main isolate has a PlatformConfiguration
, so unhandled errors from other isolates in the group will instead flow through the LogUnhandledException
path.
This eliminates the issue described above - it is no longer possible to invoke the on_error
closure from an isolate other than the one where it originated.
However, users will need to be aware that the PlatformDispatcher.onError
API only applies to the main isolate. The onError
documentation should be updated accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I'm going to mention that compute
specifically handles this case by forwarding errors to the main isolate.
/// After calling this method, the process or the VM may terminate. Some severe | ||
/// unhandled errors may not be able to call this method either, such as Dart | ||
/// compilation errors or process terminating errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a personal preference, I wouldn't introduce a typedef at all.
Dart has first class Function type syntax, and I (very personally) find typedefs confusing. I see the UpperCamelCase name and expect a class, not a function type.
Consider just removing the type. Who would suffer from that?
lib/ui/platform_dispatcher.dart
Outdated
/// After calling this method, the process or the VM may terminate. Some severe | ||
/// unhandled errors may not be able to call this method either, such as Dart | ||
/// compilation errors or process terminating errors. | ||
typedef ErrorCallback = bool Function(Object exception, StackTrace? stackTrace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is for new usage, I'd make the StackTrace argument non-nullable.
I'd only make it nullable if you're trying to cover some existing code which allows it to be null
. The platform libraries no longer allows a stack trace to be null
in its callbacks. E.g., Zone.handleUncaughtErrror
requires a stack trace.
If you don't have one, use StackTrace.ermpty
.
(Since the type is new, it's probably for new usage).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
/// called. | ||
ErrorCallback? get onError => _onError; | ||
set onError(ErrorCallback? callback) { | ||
_onError = callback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should register the callback in the zone, using
var currentZone = Zone.current;
_onErrorZone = currentZone;
_onError = (callback == null) ? null :
currentZone.registerBinaryCallback(callback);
Notice that that makes the stored function not necessarily be the same as the argument to the setter.
It might be a case where there shouldn't be a getter.
(Why is there a getter? What is the expected use?)
If you don't like having a setter without a getter, you can use the format used by StreamSubscription
with .onData(callback)
instead of .onData = callback
.
Consider not storing a zone if here is no callback. But better yet, use bindBinaryCallback
instead to store the zone inside the callback instead of on the side:
_onError = callback == null ? null :
Zone.current.bindBinaryCallback(callback);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is very commonly used in hooks.dart
. I'd prefer to stick with it for this patch and have filed a bug flutter/flutter#101448 to track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, generally speaking using Zones is pretty confusing and error prone. We try to steer our customers away from using them explicitly as much as possible.
Part of the motivation of this patch is to eliminate the need to use a custom zone to catch unhandled exceptions, and to avoid the usage of addErrorListener
which can't catch certain types of errors when the code is invoked from C++.
assert(_onErrorZone != null); | ||
|
||
if (identical(_onErrorZone, Zone.current)) { | ||
return _onError!(error, stackTrace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop this branch.
You should call _onErrorZone.runBinary
even if it's the same zone as where the function was registered. Some zones depend on getting to intercept the registered functions when they are called.
Calling Zone.run
doesn't necessarily just switch to the zone, it can do a lot of other things too in custom written zones.
Always call register
for callbacks, always call run
to later run them.
You can choose to do bindBinaryCallback
instead of register
, then it remembers the zone for you and automatically runs the function in that zone when you call it. With the Guarded
, it also captures the error and reports it to that zone. That's definitely safer if you expose the registered function through a getter, so people can't accidentally forget to run it in the proper zone. (A registered function can validly throw if it's not run in the zone it was registered in.)
Probably still want to remember the zone for the error handling below.
(Can't use bindBinaryCallbackGuarded
because you need the return type.)
The only zone where we sometimes skip calling register
and run
is the root zone, because we know it doesn't override any of the register
or run
functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto on flutter/flutter#101448
try { | ||
return _onErrorZone!.runBinary<bool, Object, StackTrace?>(_onError!, error, stackTrace); | ||
} catch (e, s) { | ||
_onErrorZone!.handleUncaughtError(e, s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _onErrorZone
could have changed during the call above.
Cache it in a local variable to ensure the error goes to the correct zone.
(Then add a test for changing the the zone during a callback, and make the callback throw.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto on flutter/flutter#101448
lib/ui/platform_dispatcher.dart
Outdated
ErrorCallback? _onError; | ||
Zone? _onErrorZone; | ||
|
||
/// A callback that is invoked when an unhandled error occurs in Dart. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors in every isolate are handled the same:
- An uncaught error gets passed to the current zone's uncaught error handler.
- If the error reaches the root zone's uncaught error handler (by being in the same error zone, or because the child zone's uncaught error handler passes the error to its parent delegate), then
- Each error listener attached to the isolate (using
Isolate.addErrorListener
or the correspondingIsolate.spawn
argument) is sent a list of[error.toString(), stackTrace.toString()]
. If any. - If the isolate is set to have errors be fatal (default on VM, not on web, set using
Isolate.setErrorsFatal
or trough anIsolate.spawn
argument), the isolate dies. (Possibly sending a message to isolate onExit-listeners, but that's unrelated to the error).
So, where are you intercepting the errors? It seems to be at the point that the root zone's handleUncaughtError
calls to?
The "in Dart" in the description seems vacuous. If it isn't in Dart, what even is an "unhandled error"?
lib/ui/platform_dispatcher.dart
Outdated
|
||
/// A callback that is invoked when an unhandled error occurs in Dart. | ||
/// | ||
/// This callback must return true if it has handled the error. Otherwise, it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally prefer to code quote `true` and `false`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
lib/ui/hooks.dart
Outdated
// ignore: always_declare_return_types, prefer_generic_function_type_aliases | ||
typedef _ListStringArgFunction(List<String> args); | ||
|
||
@pragma('vm:entry-point') | ||
void _runMainZoned(Function startMainIsolateFunction, | ||
void _runMain(Function startMainIsolateFunction, | ||
Function userMainFunction, | ||
List<String> args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is now wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
ui.ErrorCallback? get onError => _onError; | ||
@override | ||
set onError(ui.ErrorCallback? callback) { | ||
_onError = callback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not calling registerBinaryCallback
here is fine, you onlu need to register the function when you actually intend to run it.)
@gaaclarke - it looks like the regex matching thing doesn't work on Windows the same way. I'm really not convinced that we're better off using this utility as compared to just matching on the string. If we have to update the whitespace so be it. |
Okay, no problem if it's going to be a headache. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo the other feedback from jason and irhn. It sounds like this is a change the team wants to make and it sounds reasonable to me. Thanks Dan!
lib/ui/ui_dart_state.cc
Outdated
tonic::StdStringFromDart(Dart_ToString(stack_trace_handle)); | ||
|
||
auto state = UIDartState::Current(); | ||
if (state && UIDartState::Current()->unhandled_exception_callback()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: replace UIDartState::Current()
with state
here and below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lib/ui/ui_dart_state.cc
Outdated
LogUnhandledException(exception_handle, stack_trace_handle); | ||
return; | ||
} | ||
auto on_error = UIDartState::Current()->platform_configuration()->on_error(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above - use state
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -3721,6 +3721,74 @@ TEST_F(ShellTest, UsesPlatformMessageHandler) { | |||
DestroyShell(std::move(shell)); | |||
} | |||
|
|||
TEST_F(ShellTest, SpawnWorksWithOnError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DartVMInitializer::Initialize
would call tonic::SetUnhandledExceptionReporter(ReportUnhandledException)
ReportUnhandledException
is now a static function that calls UIDartState::Current()
and then uses the UIDartState
API if available. It is no longer a lambda or method tied to a UIDartState
instance.
ReportUnhandledException
could be declared in ui_dart_state.h
to be publicly accessible to DartVMInitializer
. Or its implementation could be moved to dart_vm_initializer.cc
I think this would be preferable in order to avoid global side effects in the UIDartState
constructor.
/// | ||
/// If it is an error or exception, this method will return true. | ||
/// | ||
/// If it is an unhandled error or exception, first a call will be made to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still accurate?
IIUC the current implementation will call the function given to SetUnhandledExceptionReporter
for any unhandled exception.
The engine provides a SetUnhandledExceptionReporter
handler that will try the first available option in this list:
- the platform configuration's
on_error
closure - the embedder's
Settings.unhandled_exception_callback
function - a default handler that logs the error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll update this.
// If it is not null, defer to the return value of that closure | ||
// to determine whether to report via logging. | ||
bool handled = false; | ||
auto state = flutter::UIDartState::Current(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried testing a scenario that executes this during isolate initialization and found that this is still unsafe. DartState::Current
currently assumes that Dart_CurrentIsolateData()
is non-null.
Add a check for this to https://github.com/flutter/engine/blob/main/third_party/tonic/dart_state.cc#L59 that returns null if Dart_CurrentIsolateData()
is null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and added a test to tonic that creates an isolate group with nullptr data, asserts tonic::DartState::Current()
returns nullptr and doesn't segfault in this case.
/// and falls back to simply printing the exception and stack to an error log if | ||
/// the settings callback is not provided. | ||
/// | ||
/// If UIDartState::Current() is avaialble, it can provide an onError callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "available"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
third_party/tonic/dart_state.cc
Outdated
@@ -56,7 +56,8 @@ DartState* DartState::From(Dart_Isolate isolate) { | |||
DartState* DartState::Current() { | |||
auto isolate_data = | |||
static_cast<std::shared_ptr<DartState>*>(Dart_CurrentIsolateData()); | |||
return isolate_data->get(); | |||
// return isolate_data->get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whups, done.
The |
Fixed the test. It was missing setting up flags related to null safety correctly. |
See the design doc https://flutter.dev/go/error-handling, which is a bit out of date now.
xref b/223514481
This patch does the following:
Zone
we currently use to catch unhandled errors, as well as the Dart to native method we use to forward them over toUIDartState
.PlatformDispatcher.onError
, which, if set, gets called back with the exact exception and stack trace objects for unhandled errors.LogIfError
method to callPlatformDispatcher.onError
with the original objects if it is set. IfonError
returns true, Tonic's job is done. IfonError
returns false, Tonic continues to foward stringified versions of the exception and stack toUIDartState
for the existing unreported error mechanisms, including a fallback to whatever the embedder asked for in theSettings
object. Currently, only fuchsia uses this and we should consider removing that to remove the additional complexity if the Fuchsia folks are ok with that.LogIfError
toCheckAndHandleError
. I'm not in love with this name and am very open to suggestions to improve it.onError
gets called back in the right zone, and the documented behavior of returning true/false/throwing an exception within that handler, including that we delegate to the fallback method.An expected benefit here is improved startup time, since internal testing shows that using custom zones in main impacts startup time.
I've split this patch into three commits: the first one is a big one that just renames
LogIfError
in a bunch of files I did not otherwise change. The second commit is the implementation. The third commit is the tests.I still have to think a little bit about how this will work for web, but starting with a Draft to get feedback on VM.
@xster fyi