Skip to content
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.

Sign up for GitHub

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

Merged
merged 20 commits into from
Apr 9, 2022
Merged

Unify unhandled error reporting, add PlatformDispatcher.onError #32078

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f12f3b2
renames
dnfield Mar 16, 2022
6edd564
impl
dnfield Mar 16, 2022
f2c9356
tests
dnfield Mar 16, 2022
fbae10c
web stub
dnfield Mar 17, 2022
608a26d
missing prefix
dnfield Mar 17, 2022
98f9d71
Fix build issues
dnfield Mar 17, 2022
77a4bf9
Merge remote-tracking branch 'upstream/main' into errors
dnfield Apr 6, 2022
8cf434f
review
dnfield Apr 6, 2022
1fd91cc
review
dnfield Apr 6, 2022
0e49a4a
Make sure the persistent values get collected and make ties to isolat…
dnfield Apr 8, 2022
d78d102
stray prints
dnfield Apr 8, 2022
51fe9de
more review
dnfield Apr 8, 2022
50984a9
defensive null checks
dnfield Apr 8, 2022
5f713bc
no regex
dnfield Apr 8, 2022
d335d0f
atomic fn ptr
dnfield Apr 8, 2022
96dbb17
lol whitespace
dnfield Apr 8, 2022
ac742e2
use DartVMInitializer, update docs
dnfield Apr 8, 2022
92e0d62
merge
dnfield Apr 8, 2022
6457bc8
typo and additional null check, test
dnfield Apr 9, 2022
12ebb87
Delete commented code, fix test to handle null safety flags properly
dnfield Apr 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions lib/io/dart_io.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include "third_party/tonic/converter/dart_converter.h"
#include "third_party/tonic/logging/dart_error.h"

using tonic::LogIfError;
using tonic::CheckAndHandleError;
using tonic::ToDart;

namespace flutter {
Expand All @@ -20,35 +20,35 @@ void DartIO::InitForIsolate(bool may_insecurely_connect_to_all_domains,
Dart_Handle io_lib = Dart_LookupLibrary(ToDart("dart:io"));
Dart_Handle result = Dart_SetNativeResolver(io_lib, dart::bin::LookupIONative,
dart::bin::LookupIONativeSymbol);
FML_CHECK(!LogIfError(result));
FML_CHECK(!CheckAndHandleError(result));

Dart_Handle embedder_config_type =
Dart_GetNonNullableType(io_lib, ToDart("_EmbedderConfig"), 0, nullptr);
FML_CHECK(!LogIfError(embedder_config_type));
FML_CHECK(!CheckAndHandleError(embedder_config_type));

Dart_Handle allow_insecure_connections_result = Dart_SetField(
embedder_config_type, ToDart("_mayInsecurelyConnectToAllDomains"),
ToDart(may_insecurely_connect_to_all_domains));
FML_CHECK(!LogIfError(allow_insecure_connections_result));
FML_CHECK(!CheckAndHandleError(allow_insecure_connections_result));

Dart_Handle dart_args[1];
dart_args[0] = ToDart(domain_network_policy);
Dart_Handle set_domain_network_policy_result = Dart_Invoke(
embedder_config_type, ToDart("_setDomainPolicies"), 1, dart_args);
FML_CHECK(!LogIfError(set_domain_network_policy_result));
FML_CHECK(!CheckAndHandleError(set_domain_network_policy_result));

Dart_Handle ui_lib = Dart_LookupLibrary(ToDart("dart:ui"));
Dart_Handle dart_validate_args[1];
dart_validate_args[0] = ToDart(may_insecurely_connect_to_all_domains);
Dart_Handle http_connection_hook_closure =
Dart_Invoke(ui_lib, ToDart("_getHttpConnectionHookClosure"),
/*number_of_arguments=*/1, dart_validate_args);
FML_CHECK(!LogIfError(http_connection_hook_closure));
FML_CHECK(!CheckAndHandleError(http_connection_hook_closure));
Dart_Handle http_lib = Dart_LookupLibrary(ToDart("dart:_http"));
FML_CHECK(!LogIfError(http_lib));
FML_CHECK(!CheckAndHandleError(http_lib));
Dart_Handle set_http_connection_hook_result = Dart_SetField(
http_lib, ToDart("_httpConnectionHook"), http_connection_hook_closure);
FML_CHECK(!LogIfError(set_http_connection_hook_result));
FML_CHECK(!CheckAndHandleError(set_http_connection_hook_result));
}

} // namespace flutter
1 change: 0 additions & 1 deletion lib/ui/dart_runtime_hooks.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include "third_party/tonic/scopes/dart_isolate_scope.h"

using tonic::DartConverter;
using tonic::LogIfError;
using tonic::ToDart;

namespace flutter {
Expand Down
53 changes: 50 additions & 3 deletions lib/ui/fixtures/ui_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,33 @@ void main() {}
/// validation.
void _finish() native 'Finish';

@pragma('vm:entry-point')
void customOnErrorTrue() {
gaaclarke marked this conversation as resolved.
Show resolved Hide resolved
PlatformDispatcher.instance.onError = (Object error, StackTrace? stack) {
_finish();
return true;
};
throw Exception('true');
}

@pragma('vm:entry-point')
void customOnErrorFalse() {
PlatformDispatcher.instance.onError = (Object error, StackTrace? stack) {
_finish();
return false;
};
throw Exception('false');
}

@pragma('vm:entry-point')
void customOnErrorThrow() {
PlatformDispatcher.instance.onError = (Object error, StackTrace? stack) {
_finish();
throw Exception('throw2');
};
throw Exception('throw1');
}

@pragma('vm:entry-point')
void validateSceneBuilderAndScene() {
final SceneBuilder builder = SceneBuilder();
Expand Down Expand Up @@ -311,9 +338,9 @@ void hooksTests() {
}
}

void expectIdentical(Zone originalZone, Zone callbackZone) {
if (!identical(callbackZone, originalZone)) {
throw 'Callback called in wrong zone.';
void expectIdentical(Object a, Object b) {
if (!identical(a, b)) {
throw 'Expected $a to be identical to $b.';
}
}

Expand Down Expand Up @@ -368,6 +395,26 @@ void hooksTests() {
}
});

test('onError preserves the callback zone', () {
late Zone originalZone;
late Zone callbackZone;
final Object error = Exception('foo');
StackTrace? stackTrace;

runZoned(() {
originalZone = Zone.current;
PlatformDispatcher.instance.onError = (Object exception, StackTrace? stackTrace) {
callbackZone = Zone.current;
expectIdentical(exception, error);
expectNotEquals(stackTrace, null);
return true;
};
});

_callHook('_onError', 2, error, StackTrace.current);
expectIdentical(originalZone, callbackZone);
});

test('updateUserSettings can handle an empty object', () {
_callHook('_updateUserSettingsData', 1, '{}');
});
Expand Down
27 changes: 13 additions & 14 deletions lib/ui/hooks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,28 +115,27 @@ void _drawFrame() {
PlatformDispatcher.instance._drawFrame();
}

@pragma('vm:entry-point')
bool _onError(Object error, StackTrace? stackTrace) {
return PlatformDispatcher.instance._dispatchError(error, stackTrace ?? StackTrace.empty);
}

// ignore: always_declare_return_types, prefer_generic_function_type_aliases
typedef _ListStringArgFunction(List<String> args);

@pragma('vm:entry-point')
void _runMainZoned(Function startMainIsolateFunction,
Function userMainFunction,
List<String> args) {
void _runMain(Function startMainIsolateFunction,
Function userMainFunction,
List<String> args) {
startMainIsolateFunction(() {
runZonedGuarded<void>(() {
if (userMainFunction is _ListStringArgFunction) {
userMainFunction(args);
} else {
userMainFunction();
}
}, (Object error, StackTrace stackTrace) {
_reportUnhandledException(error.toString(), stackTrace.toString());
});
if (userMainFunction is _ListStringArgFunction) {
userMainFunction(args);
} else {
userMainFunction();
}
}, null);
}

void _reportUnhandledException(String error, String stackTrace) native 'PlatformConfiguration_reportUnhandledException';

/// Invokes [callback] inside the given [zone].
void _invoke(void Function()? callback, Zone zone) {
if (callback == null) {
Expand Down
4 changes: 2 additions & 2 deletions lib/ui/painting/paint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,10 +323,10 @@ flutter::Paint DartConverter<flutter::Paint>::FromArguments(
int index,
Dart_Handle& exception) {
Dart_Handle paint_objects = Dart_GetNativeArgument(args, index);
FML_DCHECK(!LogIfError(paint_objects));
FML_DCHECK(!CheckAndHandleError(paint_objects));

Dart_Handle paint_data = Dart_GetNativeArgument(args, index + 1);
FML_DCHECK(!LogIfError(paint_data));
FML_DCHECK(!CheckAndHandleError(paint_data));

return flutter::Paint(paint_objects, paint_data);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/ui/painting/rrect.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ RRect DartConverter<flutter::RRect>::FromArguments(Dart_NativeArguments args,
int index,
Dart_Handle& exception) {
Dart_Handle value = Dart_GetNativeArgument(args, index);
FML_DCHECK(!LogIfError(value));
FML_DCHECK(!CheckAndHandleError(value));
return FromDart(value);
}

Expand Down
54 changes: 54 additions & 0 deletions lib/ui/platform_dispatcher.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,16 @@ typedef _SetNeedsReportTimingsFunc = void Function(bool value);
/// Signature for [PlatformDispatcher.onConfigurationChanged].
typedef PlatformConfigurationChangedCallback = void Function(PlatformConfiguration configuration);

/// Signature for [PlatformDispatcher.onError].
///
/// If this method returns false, the engine may use some fallback method to
/// provide information about the error.
///
/// 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.
Comment on lines +61 to +63
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#naming-rules-for-typedefs-and-function-variables

typedef ErrorCallback = bool Function(Object exception, StackTrace stackTrace);

// A gesture setting value that indicates it has not been set by the engine.
const double _kUnsetGestureSetting = -1.0;

Expand Down Expand Up @@ -1012,6 +1022,50 @@ class PlatformDispatcher {
);
}

ErrorCallback? _onError;
Zone? _onErrorZone;

/// A callback that is invoked when an unhandled error occurs in the root
/// isolate.
///
/// This callback must return `true` if it has handled the error. Otherwise,
/// it must return `false` and a fallback mechanism such as printing to stderr
/// will be used, as configured by the specific platform embedding via
/// `Settings::unhandled_exception_callback`.
///
/// The VM or the process may exit or become unresponsive after calling this
/// callback. The callback will not be called for exceptions that cause the VM
/// or process to terminate or become unresponsive before the callback can be
/// invoked.
///
/// This callback is not directly invoked by errors in child isolates of the
/// root isolate. Programs that create new isolates must listen for errors on
/// those isolates and forward the errors to the root isolate. An example of
/// this can be found in the Flutter framework's `compute` function.
ErrorCallback? get onError => _onError;
set onError(ErrorCallback? callback) {
_onError = callback;
Copy link
Contributor

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);

Copy link
Contributor Author

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.

Copy link
Contributor Author

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++.

_onErrorZone = Zone.current;
}

bool _dispatchError(Object error, StackTrace stackTrace) {
if (_onError == null) {
return false;
}
assert(_onErrorZone != null);

if (identical(_onErrorZone, Zone.current)) {
return _onError!(error, stackTrace);
Copy link
Contributor

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.

Copy link
Contributor Author

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

} else {
try {
return _onErrorZone!.runBinary<bool, Object, StackTrace>(_onError!, error, stackTrace);
} catch (e, s) {
_onErrorZone!.handleUncaughtError(e, s);
Copy link
Contributor

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.)

Copy link
Contributor Author

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

return false;
}
}
}

/// The route or path that the embedder requested when the application was
/// launched.
///
Expand Down
15 changes: 1 addition & 14 deletions lib/ui/ui_dart_state.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ std::shared_ptr<VolatilePathTracker> UIDartState::GetVolatilePathTracker()
}

void UIDartState::ScheduleMicrotask(Dart_Handle closure) {
if (tonic::LogIfError(closure) || !Dart_IsClosure(closure)) {
if (tonic::CheckAndHandleError(closure) || !Dart_IsClosure(closure)) {
return;
}

Expand Down Expand Up @@ -186,19 +186,6 @@ tonic::DartErrorHandleType UIDartState::GetLastError() {
return error;
}

void UIDartState::ReportUnhandledException(const std::string& error,
const std::string& stack_trace) {
if (unhandled_exception_callback_ &&
unhandled_exception_callback_(error, stack_trace)) {
return;
}

// Either the exception handler was not set or it could not handle the error,
// just log the exception.
FML_LOG(ERROR) << "Unhandled Exception: " << error << std::endl
<< stack_trace;
}

void UIDartState::LogMessage(const std::string& tag,
const std::string& message) const {
if (log_message_callback_) {
Expand Down
7 changes: 4 additions & 3 deletions lib/ui/ui_dart_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,6 @@ class UIDartState : public tonic::DartState {

tonic::DartErrorHandleType GetLastError();

void ReportUnhandledException(const std::string& error,
const std::string& stack_trace);

// Logs `print` messages from the application via an embedder-specified
// logging mechanism.
//
Expand All @@ -154,6 +151,10 @@ class UIDartState : public tonic::DartState {
return {std::move(object), std::move(queue)};
};

UnhandledExceptionCallback unhandled_exception_callback() const {
return unhandled_exception_callback_;
}

protected:
UIDartState(TaskObserverAdd add_callback,
TaskObserverRemove remove_callback,
Expand Down
Loading