From f12f3b24d3c251925e6e3b686205e41e2f2db7fa Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 16 Mar 2022 15:55:54 -0700 Subject: [PATCH 01/18] renames --- lib/io/dart_io.cc | 16 +++--- lib/ui/dart_runtime_hooks.cc | 1 - lib/ui/painting/paint.cc | 4 +- lib/ui/painting/rrect.cc | 2 +- lib/ui/window/window.cc | 4 +- runtime/dart_isolate.cc | 21 +++---- runtime/dart_isolate_unittests.cc | 6 +- runtime/dart_plugin_registrant.cc | 3 +- runtime/type_conversions_unittests.cc | 20 +++---- .../dart-pkg/fuchsia/sdk_ext/fuchsia.cc | 12 ++-- .../dart-pkg/zircon/sdk_ext/handle_waiter.cc | 12 ++-- .../dart-pkg/zircon/sdk_ext/natives.cc | 4 +- .../fuchsia/dart-pkg/zircon/sdk_ext/system.cc | 22 ++++---- .../fuchsia/dart_runner/builtin_libraries.cc | 56 +++++++++---------- .../dart_runner/dart_component_controller.cc | 2 +- .../fuchsia/flutter/isolate_configurator.cc | 16 +++--- testing/test_dart_native_resolver.cc | 4 +- third_party/tonic/converter/dart_converter.h | 16 +++--- third_party/tonic/dart_args.h | 4 +- third_party/tonic/dart_list.cc | 6 +- third_party/tonic/dart_message_handler.cc | 4 +- third_party/tonic/dart_microtask_queue.cc | 2 +- third_party/tonic/dart_persistent_value.cc | 1 + third_party/tonic/dart_wrappable.cc | 13 +++-- third_party/tonic/logging/dart_invoke.cc | 4 +- .../tonic/typed_data/dart_byte_data.cc | 4 +- third_party/tonic/typed_data/typed_list.cc | 6 +- 27 files changed, 134 insertions(+), 131 deletions(-) diff --git a/lib/io/dart_io.cc b/lib/io/dart_io.cc index 017054a5388fb..d9b6d59f3b04c 100644 --- a/lib/io/dart_io.cc +++ b/lib/io/dart_io.cc @@ -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 { @@ -20,22 +20,22 @@ 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]; @@ -43,12 +43,12 @@ void DartIO::InitForIsolate(bool 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 diff --git a/lib/ui/dart_runtime_hooks.cc b/lib/ui/dart_runtime_hooks.cc index 5cc82cb5832bd..60b7cf08d78b5 100644 --- a/lib/ui/dart_runtime_hooks.cc +++ b/lib/ui/dart_runtime_hooks.cc @@ -29,7 +29,6 @@ #include "third_party/tonic/scopes/dart_isolate_scope.h" using tonic::DartConverter; -using tonic::LogIfError; using tonic::ToDart; namespace flutter { diff --git a/lib/ui/painting/paint.cc b/lib/ui/painting/paint.cc index 4aa339bf5ea13..7de6c5fef1497 100644 --- a/lib/ui/painting/paint.cc +++ b/lib/ui/painting/paint.cc @@ -323,10 +323,10 @@ flutter::Paint DartConverter::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); } diff --git a/lib/ui/painting/rrect.cc b/lib/ui/painting/rrect.cc index 8bd9714f7635e..39889060e9cf3 100644 --- a/lib/ui/painting/rrect.cc +++ b/lib/ui/painting/rrect.cc @@ -40,7 +40,7 @@ RRect DartConverter::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); } diff --git a/lib/ui/window/window.cc b/lib/ui/window/window.cc index 01ff4eb146ca8..454608c251e92 100644 --- a/lib/ui/window/window.cc +++ b/lib/ui/window/window.cc @@ -32,7 +32,7 @@ void Window::DispatchPointerDataPacket(const PointerDataPacket& packet) { if (Dart_IsError(data_handle)) { return; } - tonic::LogIfError(tonic::DartInvokeField( + tonic::CheckAndHandleError(tonic::DartInvokeField( library_.value(), "_dispatchPointerDataPacket", {data_handle})); } @@ -44,7 +44,7 @@ void Window::UpdateWindowMetrics(const ViewportMetrics& metrics) { return; } tonic::DartState::Scope scope(dart_state); - tonic::LogIfError(tonic::DartInvokeField( + tonic::CheckAndHandleError(tonic::DartInvokeField( library_.value(), "_updateWindowMetrics", { tonic::ToDart(window_id_), diff --git a/runtime/dart_isolate.cc b/runtime/dart_isolate.cc index bb62265549f89..7e2c78a0f4507 100644 --- a/runtime/dart_isolate.cc +++ b/runtime/dart_isolate.cc @@ -332,12 +332,13 @@ bool DartIsolate::Initialize(Dart_Isolate dart_isolate) { SetMessageHandlingTaskRunner(GetTaskRunners().GetUITaskRunner()); - if (tonic::LogIfError( + if (tonic::CheckAndHandleError( Dart_SetLibraryTagHandler(tonic::DartState::HandleLibraryTag))) { return false; } - if (tonic::LogIfError(Dart_SetDeferredLoadHandler(OnDartLoadLibrary))) { + if (tonic::CheckAndHandleError( + Dart_SetDeferredLoadHandler(OnDartLoadLibrary))) { return false; } @@ -366,7 +367,7 @@ bool DartIsolate::LoadLoadingUnit( Dart_Handle result = Dart_DeferredLoadComplete( loading_unit_id, dart_snapshot->GetDataMapping(), dart_snapshot->GetInstructionsMapping()); - if (tonic::LogIfError(result)) { + if (tonic::CheckAndHandleError(result)) { LoadLoadingUnitError(loading_unit_id, Dart_GetError(result), /*transient*/ true); return false; @@ -381,7 +382,7 @@ void DartIsolate::LoadLoadingUnitError(intptr_t loading_unit_id, tonic::DartState::Scope scope(this); Dart_Handle result = Dart_DeferredLoadCompleteError( loading_unit_id, error_message.c_str(), transient); - tonic::LogIfError(result); + tonic::CheckAndHandleError(result); } void DartIsolate::SetMessageHandlingTaskRunner( @@ -520,7 +521,7 @@ bool DartIsolate::LoadKernel(std::shared_ptr mapping, Dart_Handle library = Dart_LoadLibraryFromKernel(mapping->GetMapping(), mapping->GetSize()); - if (tonic::LogIfError(library)) { + if (tonic::CheckAndHandleError(library)) { return false; } @@ -530,7 +531,7 @@ bool DartIsolate::LoadKernel(std::shared_ptr mapping, } Dart_SetRootLibrary(library); - if (tonic::LogIfError(Dart_FinalizeLoading(false))) { + if (tonic::CheckAndHandleError(Dart_FinalizeLoading(false))) { return false; } return true; @@ -664,7 +665,7 @@ bool DartIsolate::MarkIsolateRunnable() { [[nodiscard]] static bool InvokeMainEntrypoint( Dart_Handle user_entrypoint_function, Dart_Handle args) { - if (tonic::LogIfError(user_entrypoint_function)) { + if (tonic::CheckAndHandleError(user_entrypoint_function)) { FML_LOG(ERROR) << "Could not resolve main entrypoint function."; return false; } @@ -673,13 +674,13 @@ bool DartIsolate::MarkIsolateRunnable() { tonic::DartInvokeField(Dart_LookupLibrary(tonic::ToDart("dart:isolate")), "_getStartMainIsolateFunction", {}); - if (tonic::LogIfError(start_main_isolate_function)) { + if (tonic::CheckAndHandleError(start_main_isolate_function)) { FML_LOG(ERROR) << "Could not resolve main entrypoint trampoline."; return false; } - if (tonic::LogIfError(tonic::DartInvokeField( - Dart_LookupLibrary(tonic::ToDart("dart:ui")), "_runMainZoned", + if (tonic::CheckAndHandleError(tonic::DartInvokeField( + Dart_LookupLibrary(tonic::ToDart("dart:ui")), "_runMain", {start_main_isolate_function, user_entrypoint_function, args}))) { FML_LOG(ERROR) << "Could not invoke the main entrypoint."; return false; diff --git a/runtime/dart_isolate_unittests.cc b/runtime/dart_isolate_unittests.cc index 0bf7dd0da1756..295b1a4c762b9 100644 --- a/runtime/dart_isolate_unittests.cc +++ b/runtime/dart_isolate_unittests.cc @@ -170,8 +170,8 @@ TEST_F(DartIsolateTest, CanRunDartCodeCodeSynchronously) { ASSERT_TRUE(isolate); ASSERT_EQ(isolate->get()->GetPhase(), DartIsolate::Phase::Running); ASSERT_TRUE(isolate->RunInIsolateScope([]() -> bool { - if (tonic::LogIfError(::Dart_Invoke(Dart_RootLibrary(), - tonic::ToDart("sayHi"), 0, nullptr))) { + if (tonic::CheckAndHandleError(::Dart_Invoke( + Dart_RootLibrary(), tonic::ToDart("sayHi"), 0, nullptr))) { return false; } return true; @@ -503,7 +503,7 @@ TEST_F(DartIsolateTest, DISABLED_ValidLoadingUnitSucceeds) { AddNativeCallback( "NotifySuccess", CREATE_NATIVE_ENTRY([this](Dart_NativeArguments args) { auto bool_handle = Dart_GetNativeArgument(args, 0); - ASSERT_FALSE(tonic::LogIfError(bool_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(bool_handle)); ASSERT_TRUE(tonic::DartConverter::FromDart(bool_handle)); Signal(); })); diff --git a/runtime/dart_plugin_registrant.cc b/runtime/dart_plugin_registrant.cc index 08916ed477226..4922541e62e60 100644 --- a/runtime/dart_plugin_registrant.cc +++ b/runtime/dart_plugin_registrant.cc @@ -60,7 +60,8 @@ bool InvokeDartPluginRegistrantIfAvailable(Dart_Handle library_handle) { if (Dart_IsError(plugin_registrant)) { return false; } - tonic::LogIfError(tonic::DartInvokeField(plugin_registrant, "register", {})); + tonic::CheckAndHandleError( + tonic::DartInvokeField(plugin_registrant, "register", {})); return true; } diff --git a/runtime/type_conversions_unittests.cc b/runtime/type_conversions_unittests.cc index 650066ed2fd3d..345eb78ff9e5d 100644 --- a/runtime/type_conversions_unittests.cc +++ b/runtime/type_conversions_unittests.cc @@ -56,7 +56,7 @@ TEST_F(TypeConversionsTest, CanConvertEmptyList) { AddNativeCallback( "NotifySuccess", CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { auto bool_handle = Dart_GetNativeArgument(args, 0); - ASSERT_FALSE(tonic::LogIfError(bool_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(bool_handle)); ASSERT_TRUE(tonic::DartConverter::FromDart(bool_handle)); event.Signal(); })); @@ -64,7 +64,7 @@ TEST_F(TypeConversionsTest, CanConvertEmptyList) { "NotifyNative", CREATE_NATIVE_ENTRY([&](Dart_NativeArguments) { std::vector items; auto items_handle = tonic::ToDart(items); - ASSERT_FALSE(tonic::LogIfError(items_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(items_handle)); tonic::DartInvokeField(::Dart_RootLibrary(), "testCanConvertEmptyList", {items_handle}); })); @@ -77,7 +77,7 @@ TEST_F(TypeConversionsTest, CanConvertListOfStrings) { AddNativeCallback( "NotifySuccess", CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { auto bool_handle = Dart_GetNativeArgument(args, 0); - ASSERT_FALSE(tonic::LogIfError(bool_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(bool_handle)); ASSERT_TRUE(tonic::DartConverter::FromDart(bool_handle)); event.Signal(); })); @@ -89,7 +89,7 @@ TEST_F(TypeConversionsTest, CanConvertListOfStrings) { items.push_back("soldier"); items.push_back("sailor"); auto items_handle = tonic::ToDart(items); - ASSERT_FALSE(tonic::LogIfError(items_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(items_handle)); tonic::DartInvokeField(::Dart_RootLibrary(), "testCanConvertListOfStrings", {items_handle}); })); @@ -102,7 +102,7 @@ TEST_F(TypeConversionsTest, CanConvertListOfDoubles) { AddNativeCallback( "NotifySuccess", CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { auto bool_handle = Dart_GetNativeArgument(args, 0); - ASSERT_FALSE(tonic::LogIfError(bool_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(bool_handle)); ASSERT_TRUE(tonic::DartConverter::FromDart(bool_handle)); event.Signal(); })); @@ -114,7 +114,7 @@ TEST_F(TypeConversionsTest, CanConvertListOfDoubles) { items.push_back(3.0); items.push_back(4.0); auto items_handle = tonic::ToDart(items); - ASSERT_FALSE(tonic::LogIfError(items_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(items_handle)); tonic::DartInvokeField(::Dart_RootLibrary(), "testCanConvertListOfDoubles", {items_handle}); })); @@ -127,7 +127,7 @@ TEST_F(TypeConversionsTest, CanConvertListOfInts) { AddNativeCallback( "NotifySuccess", CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { auto bool_handle = Dart_GetNativeArgument(args, 0); - ASSERT_FALSE(tonic::LogIfError(bool_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(bool_handle)); ASSERT_TRUE(tonic::DartConverter::FromDart(bool_handle)); event.Signal(); })); @@ -139,7 +139,7 @@ TEST_F(TypeConversionsTest, CanConvertListOfInts) { items.push_back(3); items.push_back(4); auto items_handle = tonic::ToDart(items); - ASSERT_FALSE(tonic::LogIfError(items_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(items_handle)); tonic::DartInvokeField(::Dart_RootLibrary(), "testCanConvertListOfInts", {items_handle}); })); @@ -152,7 +152,7 @@ TEST_F(TypeConversionsTest, CanConvertListOfFloatsToListOfDartDoubles) { AddNativeCallback( "NotifySuccess", CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { auto bool_handle = Dart_GetNativeArgument(args, 0); - ASSERT_FALSE(tonic::LogIfError(bool_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(bool_handle)); ASSERT_TRUE(tonic::DartConverter::FromDart(bool_handle)); event.Signal(); })); @@ -164,7 +164,7 @@ TEST_F(TypeConversionsTest, CanConvertListOfFloatsToListOfDartDoubles) { items.push_back(3.0f); items.push_back(4.0f); auto items_handle = tonic::ToDart(items); - ASSERT_FALSE(tonic::LogIfError(items_handle)); + ASSERT_FALSE(tonic::CheckAndHandleError(items_handle)); // This will fail on type mismatch. tonic::DartInvokeField(::Dart_RootLibrary(), "testCanConvertListOfDoubles", {items_handle}); diff --git a/shell/platform/fuchsia/dart-pkg/fuchsia/sdk_ext/fuchsia.cc b/shell/platform/fuchsia/dart-pkg/fuchsia/sdk_ext/fuchsia.cc index 9ac85679c7b5d..2c80baa4be360 100644 --- a/shell/platform/fuchsia/dart-pkg/fuchsia/sdk_ext/fuchsia.cc +++ b/shell/platform/fuchsia/dart-pkg/fuchsia/sdk_ext/fuchsia.cc @@ -93,7 +93,7 @@ void SetReturnCode(Dart_NativeArguments arguments) { int64_t return_code; Dart_Handle status = Dart_GetNativeIntegerArgument(arguments, 0, &return_code); - if (!tonic::LogIfError(status)) { + if (!tonic::CheckAndHandleError(status)) { tonic::DartState::Current()->SetReturnCode(return_code); } } @@ -106,10 +106,10 @@ void Initialize(fidl::InterfaceHandle environment, zircon::dart::Initialize(); Dart_Handle library = Dart_LookupLibrary(ToDart("dart:fuchsia")); - FML_CHECK(!tonic::LogIfError(library)); + FML_CHECK(!tonic::CheckAndHandleError(library)); Dart_Handle result = Dart_SetNativeResolver( library, fuchsia::dart::NativeLookup, fuchsia::dart::NativeSymbol); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); auto dart_state = tonic::DartState::Current(); std::unique_ptr fuchsia_class_provider( @@ -122,21 +122,21 @@ void Initialize(fidl::InterfaceHandle environment, result = Dart_SetField( library, ToDart("_environment"), ToDart(zircon::dart::Handle::Create(environment.TakeChannel()))); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); } if (directory_request) { result = Dart_SetField( library, ToDart("_outgoingServices"), ToDart(zircon::dart::Handle::Create(std::move(directory_request)))); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); } if (view_ref) { result = Dart_SetField( library, ToDart("_viewRef"), ToDart(zircon::dart::Handle::Create((*view_ref).release()))); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); } } diff --git a/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/handle_waiter.cc b/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/handle_waiter.cc index 96d4d3180867c..4c502e4b70afe 100644 --- a/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/handle_waiter.cc +++ b/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/handle_waiter.cc @@ -91,30 +91,30 @@ void HandleWaiter::OnWaitComplete(async_dispatcher_t* dispatcher, // Put the closure invocation on the microtask queue. Dart_Handle zircon_lib = Dart_LookupLibrary(ToDart("dart:zircon")); - FML_DCHECK(!tonic::LogIfError(zircon_lib)); + FML_DCHECK(!tonic::CheckAndHandleError(zircon_lib)); Dart_Handle owc_type = Dart_GetClass(zircon_lib, ToDart("_OnWaitCompleteClosure")); - FML_DCHECK(!tonic::LogIfError(owc_type)); + FML_DCHECK(!tonic::CheckAndHandleError(owc_type)); FML_DCHECK(!callback_.is_empty()); std::vector owc_args{callback_.Release(), ToDart(status), ToDart(signal->observed)}; Dart_Handle owc = Dart_New(owc_type, Dart_Null(), owc_args.size(), owc_args.data()); - FML_DCHECK(!tonic::LogIfError(owc)); + FML_DCHECK(!tonic::CheckAndHandleError(owc)); Dart_Handle closure = Dart_GetField(owc, ToDart("_closure")); - FML_DCHECK(!tonic::LogIfError(closure)); + FML_DCHECK(!tonic::CheckAndHandleError(closure)); // TODO(issue#tbd): Use tonic::DartMicrotaskQueue::ScheduleMicrotask() // instead when tonic::DartState gets a microtask queue field. Dart_Handle async_lib = Dart_LookupLibrary(ToDart("dart:async")); - FML_DCHECK(!tonic::LogIfError(async_lib)); + FML_DCHECK(!tonic::CheckAndHandleError(async_lib)); std::vector sm_args{closure}; Dart_Handle sm_result = Dart_Invoke(async_lib, ToDart("scheduleMicrotask"), sm_args.size(), sm_args.data()); - FML_DCHECK(!tonic::LogIfError(sm_result)); + FML_DCHECK(!tonic::CheckAndHandleError(sm_result)); } } // namespace dart diff --git a/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/natives.cc b/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/natives.cc index 1d5a2ea814f9b..b3558ad882ec8 100644 --- a/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/natives.cc +++ b/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/natives.cc @@ -67,10 +67,10 @@ const uint8_t* NativeSymbol(Dart_NativeFunction native_function) { void Initialize() { Dart_Handle library = Dart_LookupLibrary(ToDart("dart:zircon")); - FML_CHECK(!tonic::LogIfError(library)); + FML_CHECK(!tonic::CheckAndHandleError(library)); Dart_Handle result = Dart_SetNativeResolver( library, zircon::dart::NativeLookup, zircon::dart::NativeSymbol); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); auto dart_state = tonic::DartState::Current(); std::unique_ptr zircon_class_provider( diff --git a/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/system.cc b/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/system.cc index 29a5d75a8e071..da4bf54e7985d 100644 --- a/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/system.cc +++ b/shell/platform/fuchsia/dart-pkg/zircon/sdk_ext/system.cc @@ -46,7 +46,7 @@ class ByteDataScope { explicit ByteDataScope(size_t size) { dart_handle_ = Dart_NewTypedData(Dart_TypedData_kByteData, size); - FML_DCHECK(!tonic::LogIfError(dart_handle_)); + FML_DCHECK(!tonic::CheckAndHandleError(dart_handle_)); Acquire(); FML_DCHECK(size == size_); } @@ -65,7 +65,7 @@ class ByteDataScope { void Release() { FML_DCHECK(is_valid_); Dart_Handle result = Dart_TypedDataReleaseData(dart_handle_); - tonic::LogIfError(result); + tonic::CheckAndHandleError(result); is_valid_ = false; data_ = nullptr; size_ = 0; @@ -81,8 +81,8 @@ class ByteDataScope { intptr_t size; Dart_Handle result = Dart_TypedDataAcquireData(dart_handle_, &type, &data_, &size); - is_valid_ = - !tonic::LogIfError(result) && type == Dart_TypedData_kByteData && data_; + is_valid_ = !tonic::CheckAndHandleError(result) && + type == Dart_TypedData_kByteData && data_; if (is_valid_) { size_ = size; } else { @@ -119,7 +119,7 @@ Dart_Handle ConstructDartObject(const char* class_name, Args&&... args) { tonic::DartState::Current()->class_library(); Dart_Handle type = Dart_HandleFromPersistent(class_library.GetClass("zircon", class_name)); - FML_DCHECK(!tonic::LogIfError(type)); + FML_DCHECK(!tonic::CheckAndHandleError(type)); const char* cstr; Dart_StringToCString(Dart_ToString(type), &cstr); @@ -128,7 +128,7 @@ Dart_Handle ConstructDartObject(const char* class_name, Args&&... args) { {std::forward(args)...}}; Dart_Handle object = Dart_New(type, Dart_EmptyString(), sizeof...(Args), args_array.data()); - FML_DCHECK(!tonic::LogIfError(object)); + FML_DCHECK(!tonic::CheckAndHandleError(object)); return object; } @@ -158,16 +158,16 @@ Dart_Handle MakeHandleInfoList( fdio_ns_t* GetNamespace() { // Grab the fdio_ns_t* out of the isolate. Dart_Handle zircon_lib = Dart_LookupLibrary(ToDart("dart:zircon")); - FML_DCHECK(!tonic::LogIfError(zircon_lib)); + FML_DCHECK(!tonic::CheckAndHandleError(zircon_lib)); Dart_Handle namespace_type = Dart_GetNonNullableType(zircon_lib, ToDart("_Namespace"), 0, nullptr); - FML_DCHECK(!tonic::LogIfError(namespace_type)); + FML_DCHECK(!tonic::CheckAndHandleError(namespace_type)); Dart_Handle namespace_field = Dart_GetField(namespace_type, ToDart("_namespace")); - FML_DCHECK(!tonic::LogIfError(namespace_field)); + FML_DCHECK(!tonic::CheckAndHandleError(namespace_field)); uint64_t fdio_ns_ptr; Dart_Handle result = Dart_IntegerToUint64(namespace_field, &fdio_ns_ptr); - FML_DCHECK(!tonic::LogIfError(result)); + FML_DCHECK(!tonic::CheckAndHandleError(result)); return reinterpret_cast(fdio_ns_ptr); } @@ -551,7 +551,7 @@ Dart_Handle System::VmoMap(fml::RefPtr vmo) { void* data = reinterpret_cast(mapped_addr); Dart_Handle object = Dart_NewExternalTypedData(Dart_TypedData_kUint8, data, static_cast(size)); - FML_DCHECK(!tonic::LogIfError(object)); + FML_DCHECK(!tonic::CheckAndHandleError(object)); SizedRegion* r = new SizedRegion(data, size); Dart_NewFinalizableHandle(object, reinterpret_cast(r), diff --git a/shell/platform/fuchsia/dart_runner/builtin_libraries.cc b/shell/platform/fuchsia/dart_runner/builtin_libraries.cc index 59cbd8bc1a08e..9fd6a9b203401 100644 --- a/shell/platform/fuchsia/dart_runner/builtin_libraries.cc +++ b/shell/platform/fuchsia/dart_runner/builtin_libraries.cc @@ -89,7 +89,7 @@ void Logger_PrintString(Dart_NativeArguments args) { void ScheduleMicrotask(Dart_NativeArguments args) { Dart_Handle closure = Dart_GetNativeArgument(args, 0); - if (tonic::LogIfError(closure) || !Dart_IsClosure(closure)) + if (tonic::CheckAndHandleError(closure) || !Dart_IsClosure(closure)) return; tonic::DartMicrotaskQueue::GetForCurrentThread()->ScheduleMicrotask(closure); } @@ -114,38 +114,38 @@ void InitBuiltinLibrariesForIsolate( // dart:fuchsia.builtin ------------------------------------------------------ Dart_Handle builtin_lib = Dart_LookupLibrary(ToDart("dart:fuchsia.builtin")); - FML_CHECK(!tonic::LogIfError(builtin_lib)); + FML_CHECK(!tonic::CheckAndHandleError(builtin_lib)); Dart_Handle result = Dart_SetNativeResolver(builtin_lib, BuiltinNativeLookup, BuiltinNativeSymbol); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); // dart:io ------------------------------------------------------------------- Dart_Handle io_lib = Dart_LookupLibrary(ToDart("dart:io")); - FML_CHECK(!tonic::LogIfError(io_lib)); + FML_CHECK(!tonic::CheckAndHandleError(io_lib)); result = Dart_SetNativeResolver(io_lib, dart::bin::IONativeLookup, dart::bin::IONativeSymbol); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); // dart:zircon --------------------------------------------------------------- Dart_Handle zircon_lib = Dart_LookupLibrary(ToDart("dart:zircon")); - FML_CHECK(!tonic::LogIfError(zircon_lib)); + FML_CHECK(!tonic::CheckAndHandleError(zircon_lib)); // NativeResolver already set by fuchsia::dart::Initialize(). // Core libraries ------------------------------------------------------------ Dart_Handle async_lib = Dart_LookupLibrary(ToDart("dart:async")); - FML_CHECK(!tonic::LogIfError(async_lib)); + FML_CHECK(!tonic::CheckAndHandleError(async_lib)); Dart_Handle core_lib = Dart_LookupLibrary(ToDart("dart:core")); - FML_CHECK(!tonic::LogIfError(core_lib)); + FML_CHECK(!tonic::CheckAndHandleError(core_lib)); Dart_Handle internal_lib = Dart_LookupLibrary(ToDart("dart:_internal")); - FML_CHECK(!tonic::LogIfError(internal_lib)); + FML_CHECK(!tonic::CheckAndHandleError(internal_lib)); Dart_Handle isolate_lib = Dart_LookupLibrary(ToDart("dart:isolate")); - FML_CHECK(!tonic::LogIfError(isolate_lib)); + FML_CHECK(!tonic::CheckAndHandleError(isolate_lib)); #if !defined(AOT_RUNTIME) // AOT: These steps already happened at compile time in gen_snapshot. @@ -153,16 +153,16 @@ void InitBuiltinLibrariesForIsolate( // We need to ensure that all the scripts loaded so far are finalized // as we are about to invoke some Dart code below to set up closures. result = Dart_FinalizeLoading(false); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); #endif // Setup the internal library's 'internalPrint' function. Dart_Handle print = Dart_Invoke(builtin_lib, ToDart("_getPrintClosure"), 0, nullptr); - FML_CHECK(!tonic::LogIfError(print)); + FML_CHECK(!tonic::CheckAndHandleError(print)); result = Dart_SetField(internal_lib, ToDart("_printClosure"), print); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); // Set up the 'scheduleImmediate' closure. Dart_Handle schedule_immediate_closure; @@ -175,33 +175,33 @@ void InitBuiltinLibrariesForIsolate( schedule_immediate_closure = Dart_Invoke( builtin_lib, ToDart("_getScheduleMicrotaskClosure"), 0, nullptr); } - FML_CHECK(!tonic::LogIfError(schedule_immediate_closure)); + FML_CHECK(!tonic::CheckAndHandleError(schedule_immediate_closure)); Dart_Handle schedule_args[1]; schedule_args[0] = schedule_immediate_closure; result = Dart_Invoke(async_lib, ToDart("_setScheduleImmediateClosure"), 1, schedule_args); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); // Set up the namespace in dart:io. Dart_Handle namespace_type = Dart_GetNonNullableType(io_lib, ToDart("_Namespace"), 0, nullptr); - FML_CHECK(!tonic::LogIfError(namespace_type)); + FML_CHECK(!tonic::CheckAndHandleError(namespace_type)); Dart_Handle namespace_args[1]; namespace_args[0] = ToDart(reinterpret_cast(namespc)); result = Dart_Invoke(namespace_type, ToDart("_setupNamespace"), 1, namespace_args); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); // Set up the namespace in dart:zircon. namespace_type = Dart_GetNonNullableType(zircon_lib, ToDart("_Namespace"), 0, nullptr); - FML_CHECK(!tonic::LogIfError(namespace_type)); + FML_CHECK(!tonic::CheckAndHandleError(namespace_type)); result = Dart_SetField(namespace_type, ToDart("_namespace"), ToDart(reinterpret_cast(namespc))); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); // Set up stdout and stderr. Dart_Handle stdio_args[3]; @@ -209,36 +209,36 @@ void InitBuiltinLibrariesForIsolate( stdio_args[1] = Dart_NewInteger(stdoutfd); stdio_args[2] = Dart_NewInteger(stderrfd); result = Dart_Invoke(io_lib, ToDart("_setStdioFDs"), 3, stdio_args); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); // Disable some dart:io operations. Dart_Handle embedder_config_type = Dart_GetNonNullableType(io_lib, ToDart("_EmbedderConfig"), 0, nullptr); - FML_CHECK(!tonic::LogIfError(embedder_config_type)); + FML_CHECK(!tonic::CheckAndHandleError(embedder_config_type)); result = Dart_SetField(embedder_config_type, ToDart("_mayExit"), Dart_False()); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); // Set the script location. result = Dart_SetField(builtin_lib, ToDart("_rawScript"), ToDart(script_uri)); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); // Setup the uriBase with the base uri of the fidl app. Dart_Handle uri_base = Dart_Invoke(io_lib, ToDart("_getUriBaseClosure"), 0, nullptr); - FML_CHECK(!tonic::LogIfError(uri_base)); + FML_CHECK(!tonic::CheckAndHandleError(uri_base)); result = Dart_SetField(core_lib, ToDart("_uriBaseClosure"), uri_base); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); Dart_Handle setup_hooks = ToDart("_setupHooks"); result = Dart_Invoke(builtin_lib, setup_hooks, 0, nullptr); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); result = Dart_Invoke(io_lib, setup_hooks, 0, nullptr); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); result = Dart_Invoke(isolate_lib, setup_hooks, 0, nullptr); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); } } // namespace dart_runner diff --git a/shell/platform/fuchsia/dart_runner/dart_component_controller.cc b/shell/platform/fuchsia/dart_runner/dart_component_controller.cc index b3d7a4ca1342c..7fbc755086a42 100644 --- a/shell/platform/fuchsia/dart_runner/dart_component_controller.cc +++ b/shell/platform/fuchsia/dart_runner/dart_component_controller.cc @@ -415,7 +415,7 @@ bool DartComponentController::Main() { return false; } for (size_t i = 0; i < arguments.size(); i++) { - tonic::LogIfError( + tonic::CheckAndHandleError( Dart_ListSetAt(dart_arguments, i, ToDart(arguments.at(i)))); } diff --git a/shell/platform/fuchsia/flutter/isolate_configurator.cc b/shell/platform/fuchsia/flutter/isolate_configurator.cc index d5bf4dc60abb2..030f4bf9bcfbb 100644 --- a/shell/platform/fuchsia/flutter/isolate_configurator.cc +++ b/shell/platform/fuchsia/flutter/isolate_configurator.cc @@ -51,44 +51,44 @@ void IsolateConfigurator::BindFuchsia() { void IsolateConfigurator::BindZircon() { // Tell dart:zircon about the FDIO namespace configured for this instance. Dart_Handle zircon_lib = Dart_LookupLibrary(tonic::ToDart("dart:zircon")); - FML_CHECK(!tonic::LogIfError(zircon_lib)); + FML_CHECK(!tonic::CheckAndHandleError(zircon_lib)); Dart_Handle namespace_type = Dart_GetNonNullableType( zircon_lib, tonic::ToDart("_Namespace"), 0, nullptr); - FML_CHECK(!tonic::LogIfError(namespace_type)); + FML_CHECK(!tonic::CheckAndHandleError(namespace_type)); Dart_Handle result = Dart_SetField(namespace_type, // tonic::ToDart("_namespace"), // tonic::ToDart(reinterpret_cast(fdio_ns_.get()))); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); } void IsolateConfigurator::BindDartIO() { // Grab the dart:io lib. Dart_Handle io_lib = Dart_LookupLibrary(tonic::ToDart("dart:io")); - FML_CHECK(!tonic::LogIfError(io_lib)); + FML_CHECK(!tonic::CheckAndHandleError(io_lib)); // Disable dart:io exit() Dart_Handle embedder_config_type = Dart_GetNonNullableType( io_lib, tonic::ToDart("_EmbedderConfig"), 0, nullptr); - FML_CHECK(!tonic::LogIfError(embedder_config_type)); + FML_CHECK(!tonic::CheckAndHandleError(embedder_config_type)); Dart_Handle result = Dart_SetField(embedder_config_type, tonic::ToDart("_mayExit"), Dart_False()); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); // Tell dart:io about the FDIO namespace configured for this instance. Dart_Handle namespace_type = Dart_GetNonNullableType(io_lib, tonic::ToDart("_Namespace"), 0, nullptr); - FML_CHECK(!tonic::LogIfError(namespace_type)); + FML_CHECK(!tonic::CheckAndHandleError(namespace_type)); Dart_Handle namespace_args[] = { Dart_NewInteger(reinterpret_cast(fdio_ns_.get())), // }; result = Dart_Invoke(namespace_type, tonic::ToDart("_setupNamespace"), 1, namespace_args); - FML_CHECK(!tonic::LogIfError(result)); + FML_CHECK(!tonic::CheckAndHandleError(result)); } } // namespace flutter_runner diff --git a/testing/test_dart_native_resolver.cc b/testing/test_dart_native_resolver.cc index f1123d18b751e..dc2cf3e303e34 100644 --- a/testing/test_dart_native_resolver.cc +++ b/testing/test_dart_native_resolver.cc @@ -101,11 +101,11 @@ void TestDartNativeResolver::SetNativeResolverForIsolate() { auto result = Dart_SetNativeResolver(Dart_RootLibrary(), DartNativeEntryResolverCallback, DartNativeEntrySymbolCallback); - FML_CHECK(!tonic::LogIfError(result)) + FML_CHECK(!tonic::CheckAndHandleError(result)) << "Could not set native resolver in test."; result = Dart_SetFfiNativeResolver(Dart_RootLibrary(), &FfiNativeResolver); - FML_CHECK(!tonic::LogIfError(result)) + FML_CHECK(!tonic::CheckAndHandleError(result)) << "Could not set FFI native resolver in test."; std::scoped_lock lock(gIsolateResolversMutex); diff --git a/third_party/tonic/converter/dart_converter.h b/third_party/tonic/converter/dart_converter.h index 0947241130a3b..be2d7818febde 100644 --- a/third_party/tonic/converter/dart_converter.h +++ b/third_party/tonic/converter/dart_converter.h @@ -397,15 +397,15 @@ inline Dart_Handle LookupNonNullableType(const std::string& library_name, const std::string& type_name) { auto library = Dart_LookupLibrary(DartConverter::ToDart(library_name)); - if (LogIfError(library)) { + if (CheckAndHandleError(library)) { return library; } auto type_string = DartConverter::ToDart(type_name); - if (LogIfError(type_string)) { + if (CheckAndHandleError(type_string)) { return type_string; } auto type = Dart_GetNonNullableType(library, type_string, 0, nullptr); - if (LogIfError(type)) { + if (CheckAndHandleError(type)) { return type; } return type; @@ -439,7 +439,7 @@ Dart_Handle CreateZeroInitializedDartObject( return ::Dart_NewDouble(0.0); } else { auto object = ::Dart_New(type_handle_or_null, ::Dart_Null(), 0, nullptr); - LogIfError(object); + CheckAndHandleError(object); return object; } return ::Dart_Null(); @@ -450,20 +450,20 @@ struct DartListFactory { static Dart_Handle NewList(Dart_Handle type_handle, intptr_t length) { bool is_nullable = false; auto is_nullable_handle = ::Dart_IsNullableType(type_handle, &is_nullable); - if (LogIfError(is_nullable_handle)) { + if (CheckAndHandleError(is_nullable_handle)) { return is_nullable_handle; } if (is_nullable) { auto list = ::Dart_NewListOfType(type_handle, length); - LogIfError(list); + CheckAndHandleError(list); return list; } else { auto sentinel = CreateZeroInitializedDartObject(type_handle); - if (LogIfError(sentinel)) { + if (CheckAndHandleError(sentinel)) { return sentinel; } auto list = ::Dart_NewListOfTypeFilled(type_handle, sentinel, length); - LogIfError(list); + CheckAndHandleError(list); return list; } return ::Dart_Null(); diff --git a/third_party/tonic/dart_args.h b/third_party/tonic/dart_args.h index 438ff7082bc81..2469e24f7b6ef 100644 --- a/third_party/tonic/dart_args.h +++ b/third_party/tonic/dart_args.h @@ -240,10 +240,10 @@ void DartCallConstructor(Sig func, Dart_NativeArguments args) { } Dart_Handle wrapper = Dart_GetNativeArgument(args, 0); - TONIC_CHECK(!LogIfError(wrapper)); + TONIC_CHECK(!CheckAndHandleError(wrapper)); intptr_t native_fields[DartWrappable::kNumberOfNativeFields]; - TONIC_CHECK(!LogIfError(Dart_GetNativeFieldsOfArgument( + TONIC_CHECK(!CheckAndHandleError(Dart_GetNativeFieldsOfArgument( args, 0, DartWrappable::kNumberOfNativeFields, native_fields))); TONIC_CHECK(!native_fields[DartWrappable::kPeerIndex]); diff --git a/third_party/tonic/dart_list.cc b/third_party/tonic/dart_list.cc index 095b45ade5364..302de94f49269 100644 --- a/third_party/tonic/dart_list.cc +++ b/third_party/tonic/dart_list.cc @@ -12,7 +12,7 @@ DartList::DartList(Dart_Handle dart_handle) : dart_handle_(dart_handle) { TONIC_DCHECK(Dart_IsList(dart_handle_)); intptr_t length; - is_valid_ = !LogIfError(Dart_ListLength(dart_handle_, &length)); + is_valid_ = !CheckAndHandleError(Dart_ListLength(dart_handle_, &length)); size_ = length; } @@ -32,14 +32,14 @@ DartList::DartList(DartList&& other) } void DartList::Set(size_t index, Dart_Handle value) { - LogIfError(Dart_ListSetAt(dart_handle_, index, value)); + CheckAndHandleError(Dart_ListSetAt(dart_handle_, index, value)); } DartList DartConverter::FromArguments(Dart_NativeArguments args, int index, Dart_Handle& exception) { Dart_Handle list = Dart_GetNativeArgument(args, index); - if (LogIfError(list) || !Dart_IsList(list)) { + if (CheckAndHandleError(list) || !Dart_IsList(list)) { exception = Dart_NewApiError("Invalid Argument"); return DartList(); } diff --git a/third_party/tonic/dart_message_handler.cc b/third_party/tonic/dart_message_handler.cc index 4119d54ec34c1..b1c501ea1f225 100644 --- a/third_party/tonic/dart_message_handler.cc +++ b/third_party/tonic/dart_message_handler.cc @@ -91,7 +91,7 @@ void DartMessageHandler::OnHandleMessage(DartState* dart_state) { Dart_SetPausedOnStart(false); // We've resumed, handle normal messages that are in the queue. result = Dart_HandleMessage(); - error = LogIfError(result); + error = CheckAndHandleError(result); dart_state->MessageEpilogue(result); if (!Dart_CurrentIsolate()) { isolate_exited_ = true; @@ -118,7 +118,7 @@ void DartMessageHandler::OnHandleMessage(DartState* dart_state) { Dart_IsFatalError(result)) { error = true; } else { - error = LogIfError(result); + error = CheckAndHandleError(result); } dart_state->MessageEpilogue(result); if (!Dart_CurrentIsolate()) { diff --git a/third_party/tonic/dart_microtask_queue.cc b/third_party/tonic/dart_microtask_queue.cc index 766151338040f..7aef037e836f4 100644 --- a/third_party/tonic/dart_microtask_queue.cc +++ b/third_party/tonic/dart_microtask_queue.cc @@ -80,7 +80,7 @@ void DartMicrotaskQueue::RunMicrotasks() { // log message. if (!dart_state->has_set_return_code() || !Dart_IsError(result) || !Dart_IsFatalError(result)) { - LogIfError(result); + CheckAndHandleError(result); } DartErrorHandleType error = GetErrorHandleType(result); if (error != kNoError) { diff --git a/third_party/tonic/dart_persistent_value.cc b/third_party/tonic/dart_persistent_value.cc index 652d2c9fc3a1e..4c19b46946ddc 100644 --- a/third_party/tonic/dart_persistent_value.cc +++ b/third_party/tonic/dart_persistent_value.cc @@ -40,6 +40,7 @@ void DartPersistentValue::Clear() { auto dart_state = dart_state_.lock(); if (!dart_state) { + value_ = nullptr; return; } diff --git a/third_party/tonic/dart_wrappable.cc b/third_party/tonic/dart_wrappable.cc index b746f9f8c7948..69cc7a533f6da 100644 --- a/third_party/tonic/dart_wrappable.cc +++ b/third_party/tonic/dart_wrappable.cc @@ -26,16 +26,16 @@ Dart_Handle DartWrappable::CreateDartWrapper(DartState* dart_state) { const DartWrapperInfo& info = GetDartWrapperInfo(); Dart_PersistentHandle type = dart_state->class_library().GetClass(info); - TONIC_DCHECK(!LogIfError(type)); + TONIC_DCHECK(!CheckAndHandleError(type)); Dart_Handle wrapper = Dart_New(type, dart_state->private_constructor_name(), 0, nullptr); - TONIC_DCHECK(!LogIfError(wrapper)); + TONIC_DCHECK(!CheckAndHandleError(wrapper)); Dart_Handle res = Dart_SetNativeInstanceField( wrapper, kPeerIndex, reinterpret_cast(this)); - TONIC_DCHECK(!LogIfError(res)); + TONIC_DCHECK(!CheckAndHandleError(res)); this->RetainDartWrappableReference(); // Balanced in FinalizeDartWrapper. dart_wrapper_.Set(dart_state, wrapper, this, GetAllocationSize(), @@ -51,9 +51,9 @@ void DartWrappable::AssociateWithDartWrapper(Dart_Handle wrapper) { dart_wrapper_.Clear(); } - TONIC_CHECK(!LogIfError(wrapper)); + TONIC_CHECK(!CheckAndHandleError(wrapper)); - TONIC_CHECK(!LogIfError(Dart_SetNativeInstanceField( + TONIC_CHECK(!CheckAndHandleError(Dart_SetNativeInstanceField( wrapper, kPeerIndex, reinterpret_cast(this)))); this->RetainDartWrappableReference(); // Balanced in FinalizeDartWrapper. @@ -66,7 +66,8 @@ void DartWrappable::AssociateWithDartWrapper(Dart_Handle wrapper) { void DartWrappable::ClearDartWrapper() { TONIC_DCHECK(!dart_wrapper_.is_empty()); Dart_Handle wrapper = dart_wrapper_.Get(); - TONIC_CHECK(!LogIfError(Dart_SetNativeInstanceField(wrapper, kPeerIndex, 0))); + TONIC_CHECK(!CheckAndHandleError( + Dart_SetNativeInstanceField(wrapper, kPeerIndex, 0))); dart_wrapper_.Clear(); this->ReleaseDartWrappableReference(); } diff --git a/third_party/tonic/logging/dart_invoke.cc b/third_party/tonic/logging/dart_invoke.cc index 0b17c8349c764..abdcc82c29f33 100644 --- a/third_party/tonic/logging/dart_invoke.cc +++ b/third_party/tonic/logging/dart_invoke.cc @@ -22,13 +22,13 @@ Dart_Handle DartInvoke(Dart_Handle closure, int argc = args.size(); Dart_Handle* argv = const_cast(args.begin()); Dart_Handle handle = Dart_InvokeClosure(closure, argc, argv); - LogIfError(handle); + CheckAndHandleError(handle); return handle; } Dart_Handle DartInvokeVoid(Dart_Handle closure) { Dart_Handle handle = Dart_InvokeClosure(closure, 0, nullptr); - LogIfError(handle); + CheckAndHandleError(handle); return handle; } diff --git a/third_party/tonic/typed_data/dart_byte_data.cc b/third_party/tonic/typed_data/dart_byte_data.cc index cfb07399f22eb..cab038d14f85d 100644 --- a/third_party/tonic/typed_data/dart_byte_data.cc +++ b/third_party/tonic/typed_data/dart_byte_data.cc @@ -61,7 +61,7 @@ DartByteData::DartByteData(Dart_Handle list) Dart_TypedData_Type type; Dart_TypedDataAcquireData(list, &type, &data_, &length_in_bytes_); - TONIC_DCHECK(!LogIfError(list)); + TONIC_DCHECK(!CheckAndHandleError(list)); if (type != Dart_TypedData_kByteData) Dart_ThrowException(ToDart("Non-genuine ByteData passed to engine.")); } @@ -95,7 +95,7 @@ DartByteData DartConverter::FromArguments( int index, Dart_Handle& exception) { Dart_Handle data = Dart_GetNativeArgument(args, index); - TONIC_DCHECK(!LogIfError(data)); + TONIC_DCHECK(!CheckAndHandleError(data)); return DartByteData(data); } diff --git a/third_party/tonic/typed_data/typed_list.cc b/third_party/tonic/typed_data/typed_list.cc index ffe662591dea4..3336e0a8be395 100644 --- a/third_party/tonic/typed_data/typed_list.cc +++ b/third_party/tonic/typed_data/typed_list.cc @@ -23,7 +23,7 @@ TypedList::TypedList(Dart_Handle list) Dart_TypedData_Type type; Dart_TypedDataAcquireData(list, &type, reinterpret_cast(&data_), &num_elements_); - TONIC_DCHECK(!LogIfError(list)); + TONIC_DCHECK(!CheckAndHandleError(list)); if (type != kTypeName) Dart_ThrowException(ToDart("Non-genuine TypedData passed to engine.")); } @@ -61,7 +61,7 @@ DartConverter>::FromArguments( int index, Dart_Handle& exception) { Dart_Handle list = Dart_GetNativeArgument(args, index); - TONIC_DCHECK(!LogIfError(list)); + TONIC_DCHECK(!CheckAndHandleError(list)); return TypedList(list); } @@ -80,7 +80,7 @@ Dart_Handle DartConverter>::ToDart( unsigned int length) { const intptr_t buffer_length = static_cast(length); Dart_Handle array = Dart_NewTypedData(kTypeName, buffer_length); - TONIC_DCHECK(!LogIfError(array)); + TONIC_DCHECK(!CheckAndHandleError(array)); { Dart_TypedData_Type type; void* data = nullptr; From 6edd5642f981042c76b1c18c337996b51b642184 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 16 Mar 2022 15:56:10 -0700 Subject: [PATCH 02/18] impl --- lib/ui/hooks.dart | 23 +++++---- lib/ui/platform_dispatcher.dart | 48 +++++++++++++++++++ lib/ui/ui_dart_state.cc | 6 ++- third_party/tonic/logging/dart_error.cc | 62 ++++++++++++++++++++++--- third_party/tonic/logging/dart_error.h | 50 +++++++++++++++++++- 5 files changed, 168 insertions(+), 21 deletions(-) diff --git a/lib/ui/hooks.dart b/lib/ui/hooks.dart index 5ba01a6ca84e2..0a7a5c776c54e 100644 --- a/lib/ui/hooks.dart +++ b/lib/ui/hooks.dart @@ -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); +} + // ignore: always_declare_return_types, prefer_generic_function_type_aliases typedef _ListStringArgFunction(List args); @pragma('vm:entry-point') -void _runMainZoned(Function startMainIsolateFunction, +void _runMain(Function startMainIsolateFunction, Function userMainFunction, List args) { startMainIsolateFunction(() { - runZonedGuarded(() { - 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) { diff --git a/lib/ui/platform_dispatcher.dart b/lib/ui/platform_dispatcher.dart index b6de42b98fc34..2d4ae32c19b0c 100644 --- a/lib/ui/platform_dispatcher.dart +++ b/lib/ui/platform_dispatcher.dart @@ -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. +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; @@ -1012,6 +1022,44 @@ class PlatformDispatcher { ); } + ErrorCallback? _onError; + Zone? _onErrorZone; + + /// 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 + /// must return false and a fallback mechanism such as printing to stderr may + /// 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 + /// method. The method will not be called for exceptions that cause the VM or + /// process to terminate or become unresponsive before the method can be + /// called. + ErrorCallback? get onError => _onError; + set onError(ErrorCallback? callback) { + _onError = callback; + _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); + } else { + try { + return _onErrorZone!.runBinary(_onError!, error, stackTrace); + } catch (e, s) { + _onErrorZone!.handleUncaughtError(e, s); + return false; + } + } + } + /// The route or path that the embedder requested when the application was /// launched. /// diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc index b9bd6a749fcbb..144b2f5257476 100644 --- a/lib/ui/ui_dart_state.cc +++ b/lib/ui/ui_dart_state.cc @@ -69,6 +69,10 @@ UIDartState::UIDartState( enable_display_list_(enable_display_list), context_(std::move(context)) { AddOrRemoveTaskObserver(true /* add */); + tonic::SetUnhandledExceptionReporter( + [&](const std::string& error, const std::string& stack_trace) { + ReportUnhandledException(error, stack_trace); + }); } UIDartState::~UIDartState() { @@ -134,7 +138,7 @@ std::shared_ptr UIDartState::GetVolatilePathTracker() } void UIDartState::ScheduleMicrotask(Dart_Handle closure) { - if (tonic::LogIfError(closure) || !Dart_IsClosure(closure)) { + if (tonic::CheckAndHandleError(closure) || !Dart_IsClosure(closure)) { return; } diff --git a/third_party/tonic/logging/dart_error.cc b/third_party/tonic/logging/dart_error.cc index e1c4f59e8b45f..26eeddccc010c 100644 --- a/third_party/tonic/logging/dart_error.cc +++ b/third_party/tonic/logging/dart_error.cc @@ -10,18 +10,66 @@ namespace tonic { namespace DartError { const char kInvalidArgument[] = "Invalid argument."; +DartPersistentValue on_error; +UnhandledExceptionReporter log_unhandled_exception = [](const std::string&, + const std::string&) {}; + +void ReportUnhandledException(Dart_Handle exception_handle, + Dart_Handle stack_trace_handle) { + const std::string exception = + tonic::StdStringFromDart(Dart_ToString(exception_handle)); + const std::string stack_trace = + tonic::StdStringFromDart(Dart_ToString(stack_trace_handle)); + DartError::log_unhandled_exception(exception, stack_trace); +} } // namespace DartError -bool LogIfError(Dart_Handle handle) { +void SetErrorHandler(DartState* dart_state, Dart_Handle value) { + DartError::on_error.Clear(); + if (Dart_IsNull(value)) { + return; + } + DartError::on_error.Set(dart_state, value); +} + +void SetUnhandledExceptionReporter( + DartError::UnhandledExceptionReporter reporter) { + DartError::log_unhandled_exception = reporter; +} + +bool CheckAndHandleError(Dart_Handle handle) { + // Specifically handle UnhandledExceptionErrors first. These exclude fatal + // errors that are shutting down the vm and compilation errors in source code. if (Dart_IsUnhandledExceptionError(handle)) { Dart_Handle exception_handle = Dart_ErrorGetException(handle); - const std::string exception = - tonic::StdStringFromDart(Dart_ToString(exception_handle)); Dart_Handle stack_trace_handle = Dart_ErrorGetStackTrace(handle); - const std::string stack_trace = - tonic::StdStringFromDart(Dart_ToString(stack_trace_handle)); - tonic::Log("Dart Unhandled Exception: %s, stack trace: %s", - exception.c_str(), stack_trace.c_str()); + Dart_Handle on_error = DartError::on_error.Get(); + + // Hooks.dart will call the error handler on PlatformDispatcher if it is + // not null. If it is null, returns false, fall into the !handled branch + // below and log. + // If it is not null, defer to the return value of that closure + // to determine whether to report via logging. + bool handled = false; + if (on_error) { + Dart_Handle args[2]; + args[0] = exception_handle; + args[1] = stack_trace_handle; + Dart_Handle on_error_result = Dart_InvokeClosure(on_error, 2, args); + + // An exception was thrown by the exception handler. + if (Dart_IsError(on_error_result)) { + DartError::ReportUnhandledException( + Dart_ErrorGetException(on_error_result), + Dart_ErrorGetStackTrace(on_error_result)); + handled = false; + } else { + handled = DartConverter::FromDart(on_error_result); + } + } + if (!handled) { + DartError::ReportUnhandledException(exception_handle, stack_trace_handle); + } return true; } else if (Dart_IsError(handle)) { tonic::Log("Dart Error: %s", Dart_GetError(handle)); diff --git a/third_party/tonic/logging/dart_error.h b/third_party/tonic/logging/dart_error.h index ede51f41da1c3..392f51d4c17c1 100644 --- a/third_party/tonic/logging/dart_error.h +++ b/third_party/tonic/logging/dart_error.h @@ -7,13 +7,61 @@ #include "third_party/dart/runtime/include/dart_api.h" +#include "tonic/dart_persistent_value.h" + namespace tonic { namespace DartError { +using UnhandledExceptionReporter = + std::function; + extern const char kInvalidArgument[]; +extern DartPersistentValue on_error; +extern UnhandledExceptionReporter log_unhandled_exception; } // namespace DartError -bool LogIfError(Dart_Handle handle); +/// Check if a Dart_Handle is an error or exception. +/// +/// 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 +/// Dart code set in |SetErrorHandler|. If that closure returns false, a +/// fallback is made to the closure in |SetUnhandledExceptionReporter| to log +/// details of the exception and stack. If that closure throws an exception +/// itself, the exception and stack are logged via the fallback method, and the +/// original exception and stack are also logged via the fallback method. +/// +/// The fallback behavior matches the behavior of Flutter applications before +/// the introduction of PlatformDispatcher.onError. +/// +/// If it is not an unhandled exception, it is logged to stderr or some similar +/// mechanism provided by the platform. Depending on which type of error occurs, +/// the process may crash and the Dart isolate may be unusable. Errors that fall +/// into this category include compilation errors, Dart API errors, and unwind +/// errors that will terminate the Dart VM. +/// +/// Historically known as LogIfError. +bool CheckAndHandleError(Dart_Handle handle); + +/// Provides a Dart_Handle to a top level static field closure to invoke when +/// an unhandled exception occurs. +/// +/// The signature of this field must match that of _onError in hooks.dart, +/// namely `bool _onError(Object error, StackTrace? stackTrace)`. +/// +/// This handler will be invoked when an unhandled error occurs. If it returns +/// false or throws an exception, a fallback handler will be invoked. By +/// default, UIDartState registers its ReportUnhandledError and will either call +/// a closure provided in the Shell Settings or simply dump the error to stderr +/// or some similar platform specific mechanism. +void SetErrorHandler(DartState* dart_state, Dart_Handle value); + +/// The fallback mechanism to log errors if |SetErrorHandler| has provided a +/// null value or a closure that returns false. +/// +/// Normally, it UIDartState registers with this method in its constructor. +void SetUnhandledExceptionReporter( + DartError::UnhandledExceptionReporter reporter); enum DartErrorHandleType { kNoError, From f2c935661a028fc36778e46f4444e3ad01d66846 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 16 Mar 2022 15:56:17 -0700 Subject: [PATCH 03/18] tests --- lib/ui/fixtures/ui_test.dart | 53 +++++- lib/ui/window/platform_configuration.cc | 69 +++----- .../platform_configuration_unittests.cc | 156 ++++++++++++++++++ 3 files changed, 230 insertions(+), 48 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index ad560db96ba30..158c0033db0a3 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -12,6 +12,33 @@ void main() {} /// validation. void _finish() native 'Finish'; +@pragma('vm:entry-point') +void customOnErrorTrue() { + 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(); @@ -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.'; } } @@ -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 e, StackTrace? s) { + callbackZone = Zone.current; + expectIdentical(e, error); + expectNotEquals(s, null); + return true; + }; + }); + + _callHook('_onError', 2, error, StackTrace.current); + expectIdentical(originalZone, callbackZone); + }); + test('updateUserSettings can handle an empty object', () { _callHook('_updateUserSettingsData', 1, '{}'); }); diff --git a/lib/ui/window/platform_configuration.cc b/lib/ui/window/platform_configuration.cc index 3cebfb5d10e24..b9094d3765438 100644 --- a/lib/ui/window/platform_configuration.cc +++ b/lib/ui/window/platform_configuration.cc @@ -82,29 +82,6 @@ void SetNeedsReportTimings(Dart_NativeArguments args) { ->SetNeedsReportTimings(value); } -void ReportUnhandledException(Dart_NativeArguments args) { - UIDartState::ThrowIfUIOperationsProhibited(); - - Dart_Handle exception = nullptr; - - auto error_name = - tonic::DartConverter::FromArguments(args, 0, exception); - if (exception) { - Dart_ThrowException(exception); - return; - } - - auto stack_trace = - tonic::DartConverter::FromArguments(args, 1, exception); - if (exception) { - Dart_ThrowException(exception); - return; - } - - UIDartState::Current()->ReportUnhandledException(std::move(error_name), - std::move(stack_trace)); -} - Dart_Handle SendPlatformMessage(Dart_Handle window, const std::string& name, Dart_Handle callback, @@ -197,6 +174,9 @@ PlatformConfiguration::~PlatformConfiguration() {} void PlatformConfiguration::DidCreateIsolate() { Dart_Handle library = Dart_LookupLibrary(tonic::ToDart("dart:ui")); + tonic::SetErrorHandler(tonic::DartState::Current(), + Dart_GetField(library, tonic::ToDart("_onError"))); + update_locales_.Set(tonic::DartState::Current(), Dart_GetField(library, tonic::ToDart("_updateLocales"))); update_user_settings_data_.Set( @@ -237,7 +217,7 @@ void PlatformConfiguration::UpdateLocales( } tonic::DartState::Scope scope(dart_state); - tonic::LogIfError( + tonic::CheckAndHandleError( tonic::DartInvoke(update_locales_.Get(), { tonic::ToDart>(locales), @@ -252,10 +232,10 @@ void PlatformConfiguration::UpdateUserSettingsData(const std::string& data) { } tonic::DartState::Scope scope(dart_state); - tonic::LogIfError(tonic::DartInvoke(update_user_settings_data_.Get(), - { - tonic::StdStringToDart(data), - })); + tonic::CheckAndHandleError(tonic::DartInvoke(update_user_settings_data_.Get(), + { + tonic::StdStringToDart(data), + })); } void PlatformConfiguration::UpdateLifecycleState(const std::string& data) { @@ -265,10 +245,10 @@ void PlatformConfiguration::UpdateLifecycleState(const std::string& data) { return; } tonic::DartState::Scope scope(dart_state); - tonic::LogIfError(tonic::DartInvoke(update_lifecycle_state_.Get(), - { - tonic::StdStringToDart(data), - })); + tonic::CheckAndHandleError(tonic::DartInvoke(update_lifecycle_state_.Get(), + { + tonic::StdStringToDart(data), + })); } void PlatformConfiguration::UpdateSemanticsEnabled(bool enabled) { @@ -280,8 +260,8 @@ void PlatformConfiguration::UpdateSemanticsEnabled(bool enabled) { tonic::DartState::Scope scope(dart_state); UIDartState::ThrowIfUIOperationsProhibited(); - tonic::LogIfError(tonic::DartInvoke(update_semantics_enabled_.Get(), - {tonic::ToDart(enabled)})); + tonic::CheckAndHandleError(tonic::DartInvoke(update_semantics_enabled_.Get(), + {tonic::ToDart(enabled)})); } void PlatformConfiguration::UpdateAccessibilityFeatures(int32_t values) { @@ -292,8 +272,8 @@ void PlatformConfiguration::UpdateAccessibilityFeatures(int32_t values) { } tonic::DartState::Scope scope(dart_state); - tonic::LogIfError(tonic::DartInvoke(update_accessibility_features_.Get(), - {tonic::ToDart(values)})); + tonic::CheckAndHandleError(tonic::DartInvoke( + update_accessibility_features_.Get(), {tonic::ToDart(values)})); } void PlatformConfiguration::DispatchPlatformMessage( @@ -322,7 +302,7 @@ void PlatformConfiguration::DispatchPlatformMessage( pending_responses_[response_id] = response; } - tonic::LogIfError( + tonic::CheckAndHandleError( tonic::DartInvoke(dispatch_platform_message_.Get(), {tonic::ToDart(message->channel()), data_handle, tonic::ToDart(response_id)})); @@ -345,7 +325,7 @@ void PlatformConfiguration::DispatchSemanticsAction(int32_t id, return; } - tonic::LogIfError(tonic::DartInvoke( + tonic::CheckAndHandleError(tonic::DartInvoke( dispatch_semantics_action_.Get(), {tonic::ToDart(id), tonic::ToDart(static_cast(action)), args_handle})); @@ -362,7 +342,7 @@ void PlatformConfiguration::BeginFrame(fml::TimePoint frameTime, int64_t microseconds = (frameTime - fml::TimePoint()).ToMicroseconds(); - tonic::LogIfError( + tonic::CheckAndHandleError( tonic::DartInvoke(begin_frame_.Get(), { Dart_NewInteger(microseconds), Dart_NewInteger(frame_number), @@ -370,7 +350,7 @@ void PlatformConfiguration::BeginFrame(fml::TimePoint frameTime, UIDartState::Current()->FlushMicrotasksNow(); - tonic::LogIfError(tonic::DartInvokeVoid(draw_frame_.Get())); + tonic::CheckAndHandleError(tonic::DartInvokeVoid(draw_frame_.Get())); } void PlatformConfiguration::ReportTimings(std::vector timings) { @@ -394,9 +374,10 @@ void PlatformConfiguration::ReportTimings(std::vector timings) { memcpy(data, timings.data(), sizeof(int64_t) * timings.size()); FML_CHECK(Dart_TypedDataReleaseData(data_handle)); - tonic::LogIfError(tonic::DartInvoke(report_timings_.Get(), { - data_handle, - })); + tonic::CheckAndHandleError( + tonic::DartInvoke(report_timings_.Get(), { + data_handle, + })); } void PlatformConfiguration::CompletePlatformMessageEmptyResponse( @@ -462,8 +443,6 @@ void PlatformConfiguration::RegisterNatives( {"PlatformConfiguration_updateSemantics", UpdateSemantics, 2, true}, {"PlatformConfiguration_setIsolateDebugName", SetIsolateDebugName, 2, true}, - {"PlatformConfiguration_reportUnhandledException", - ReportUnhandledException, 2, true}, {"PlatformConfiguration_setNeedsReportTimings", SetNeedsReportTimings, 2, true}, {"PlatformConfiguration_getPersistentIsolateData", diff --git a/lib/ui/window/platform_configuration_unittests.cc b/lib/ui/window/platform_configuration_unittests.cc index e18c9e466bd6c..530a092d79a26 100644 --- a/lib/ui/window/platform_configuration_unittests.cc +++ b/lib/ui/window/platform_configuration_unittests.cc @@ -115,5 +115,161 @@ TEST_F(ShellTest, PlatformConfigurationWindowMetricsUpdate) { DestroyShell(std::move(shell), std::move(task_runners)); } +TEST_F(ShellTest, PlatformConfigurationOnErrorHandlesError) { + auto message_latch = std::make_shared(); + bool did_throw = false; + + auto finish = [message_latch](Dart_NativeArguments args) { + message_latch->Signal(); + }; + AddNativeCallback("Finish", CREATE_NATIVE_ENTRY(finish)); + + Settings settings = CreateSettingsForFixture(); + settings.unhandled_exception_callback = + [&did_throw](const std::string& exception, + const std::string& stack_trace) -> bool { + did_throw = true; + return false; + }; + + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + std::unique_ptr shell = + CreateShell(std::move(settings), std::move(task_runners)); + + ASSERT_TRUE(shell->IsSetup()); + auto run_configuration = RunConfiguration::InferFromSettings(settings); + run_configuration.SetEntrypoint("customOnErrorTrue"); + + shell->RunEngine(std::move(run_configuration), [&](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch->Wait(); + + // Flush the UI task runner to make sure errors that were triggered had a turn + // to propagate. + task_runners.GetUITaskRunner()->PostTask( + [&message_latch]() { message_latch->Signal(); }); + message_latch->Wait(); + + ASSERT_FALSE(did_throw); + DestroyShell(std::move(shell), std::move(task_runners)); +} + +TEST_F(ShellTest, PlatformConfigurationOnErrorDoesNotHandleError) { + auto message_latch = std::make_shared(); + std::string ex; + std::string st; + size_t throw_count = 0; + + auto finish = [message_latch](Dart_NativeArguments args) { + message_latch->Signal(); + }; + AddNativeCallback("Finish", CREATE_NATIVE_ENTRY(finish)); + + Settings settings = CreateSettingsForFixture(); + settings.unhandled_exception_callback = + [&ex, &st, &throw_count](const std::string& exception, + const std::string& stack_trace) -> bool { + throw_count += 1; + ex = std::move(exception); + st = std::move(stack_trace); + return true; + }; + + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + std::unique_ptr shell = + CreateShell(std::move(settings), std::move(task_runners)); + + ASSERT_TRUE(shell->IsSetup()); + auto run_configuration = RunConfiguration::InferFromSettings(settings); + run_configuration.SetEntrypoint("customOnErrorFalse"); + + shell->RunEngine(std::move(run_configuration), [&](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch->Wait(); + + // Flush the UI task runner to make sure errors that were triggered had a turn + // to propagate. + task_runners.GetUITaskRunner()->PostTask( + [&message_latch]() { message_latch->Signal(); }); + message_latch->Wait(); + + ASSERT_EQ(throw_count, 1ul); + ASSERT_EQ(ex, "Exception: false") << ex; + ASSERT_EQ(st.rfind("#0 customOnErrorFalse", 0), 0ul) << st; + DestroyShell(std::move(shell), std::move(task_runners)); +} + +TEST_F(ShellTest, PlatformConfigurationOnErrorThrows) { + auto message_latch = std::make_shared(); + std::vector errors; + size_t throw_count = 0; + + auto finish = [message_latch](Dart_NativeArguments args) { + message_latch->Signal(); + }; + AddNativeCallback("Finish", CREATE_NATIVE_ENTRY(finish)); + + Settings settings = CreateSettingsForFixture(); + settings.unhandled_exception_callback = + [&errors, &throw_count](const std::string& exception, + const std::string& stack_trace) -> bool { + throw_count += 1; + errors.push_back(std::move(exception)); + errors.push_back(std::move(stack_trace)); + return true; + }; + + TaskRunners task_runners("test", // label + GetCurrentTaskRunner(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + ); + + std::unique_ptr shell = + CreateShell(std::move(settings), std::move(task_runners)); + + ASSERT_TRUE(shell->IsSetup()); + auto run_configuration = RunConfiguration::InferFromSettings(settings); + run_configuration.SetEntrypoint("customOnErrorThrow"); + + shell->RunEngine(std::move(run_configuration), [&](auto result) { + ASSERT_EQ(result, Engine::RunStatus::Success); + }); + + message_latch->Wait(); + + // Flush the UI task runner to make sure errors that were triggered had a turn + // to propagate. + task_runners.GetUITaskRunner()->PostTask( + [&message_latch]() { message_latch->Signal(); }); + message_latch->Wait(); + + ASSERT_EQ(throw_count, 2ul); + ASSERT_EQ(errors.size(), 4ul); + ASSERT_EQ(errors[0], "Exception: throw2") << errors[0]; + ASSERT_EQ(errors[1].rfind("#0 customOnErrorThrow"), 0ul) << errors[1]; + ASSERT_EQ(errors[2], "Exception: throw1") << errors[2]; + ASSERT_EQ(errors[3].rfind("#0 customOnErrorThrow"), 0ul) << errors[3]; + + DestroyShell(std::move(shell), std::move(task_runners)); +} + } // namespace testing } // namespace flutter From fbae10ccc6171a9d65d9d1adc15d88a1e57c07bf Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 16 Mar 2022 21:45:05 -0700 Subject: [PATCH 04/18] web stub --- lib/web_ui/lib/platform_dispatcher.dart | 4 ++++ lib/web_ui/lib/src/engine/platform_dispatcher.dart | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/lib/web_ui/lib/platform_dispatcher.dart b/lib/web_ui/lib/platform_dispatcher.dart index 61ed62a1e288b..7e72bee0c440f 100644 --- a/lib/web_ui/lib/platform_dispatcher.dart +++ b/lib/web_ui/lib/platform_dispatcher.dart @@ -14,6 +14,7 @@ typedef PlatformMessageResponseCallback = void Function(ByteData? data); typedef PlatformMessageCallback = void Function( String name, ByteData? data, PlatformMessageResponseCallback? callback); typedef PlatformConfigurationChangedCallback = void Function(PlatformConfiguration configuration); +typedef ErrorCallback = bool Function(Object exception, StackTrace? stackTrace); abstract class PlatformDispatcher { static PlatformDispatcher get instance => engine.EnginePlatformDispatcher.instance; @@ -104,6 +105,9 @@ abstract class PlatformDispatcher { SemanticsActionCallback? get onSemanticsAction; set onSemanticsAction(SemanticsActionCallback? callback); + ErrorCallback? get onError; + set onError(ErrorCallback? callback); + String get defaultRouteName; FrameData get frameData; diff --git a/lib/web_ui/lib/src/engine/platform_dispatcher.dart b/lib/web_ui/lib/src/engine/platform_dispatcher.dart index c4257dbd15c6f..14607cd0a3d66 100644 --- a/lib/web_ui/lib/src/engine/platform_dispatcher.dart +++ b/lib/web_ui/lib/src/engine/platform_dispatcher.dart @@ -1024,6 +1024,18 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { _onSemanticsAction, _onSemanticsActionZone, id, action, args); } + // TODO(dnfield): make this work on web. + // https://github.com/flutter/flutter/issues/100277 + ui.ErrorCallback? _onError; + Zone? _onErrorZone; + @override + ui.ErrorCallback? get onError => _onError; + @override + set onError(ErrorCallback? callback) { + _onError = callback; + _onErrorZone = Zone.current; + } + /// The route or path that the embedder requested when the application was /// launched. /// From 608a26d799b4b1d61a40a3d571eee16979759b0d Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 16 Mar 2022 22:05:25 -0700 Subject: [PATCH 05/18] missing prefix --- lib/web_ui/lib/src/engine/platform_dispatcher.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/web_ui/lib/src/engine/platform_dispatcher.dart b/lib/web_ui/lib/src/engine/platform_dispatcher.dart index 14607cd0a3d66..3c9f417669180 100644 --- a/lib/web_ui/lib/src/engine/platform_dispatcher.dart +++ b/lib/web_ui/lib/src/engine/platform_dispatcher.dart @@ -1031,7 +1031,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { @override ui.ErrorCallback? get onError => _onError; @override - set onError(ErrorCallback? callback) { + set onError(ui.ErrorCallback? callback) { _onError = callback; _onErrorZone = Zone.current; } From 98f9d71a5573a7abf068ddd02475b3be2caf301e Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 17 Mar 2022 11:13:21 -0700 Subject: [PATCH 06/18] Fix build issues --- lib/web_ui/lib/src/engine/platform_dispatcher.dart | 1 + shell/platform/fuchsia/runtime/dart/utils/BUILD.gn | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/web_ui/lib/src/engine/platform_dispatcher.dart b/lib/web_ui/lib/src/engine/platform_dispatcher.dart index 3c9f417669180..5ad7a4dd23a8f 100644 --- a/lib/web_ui/lib/src/engine/platform_dispatcher.dart +++ b/lib/web_ui/lib/src/engine/platform_dispatcher.dart @@ -1027,6 +1027,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { // TODO(dnfield): make this work on web. // https://github.com/flutter/flutter/issues/100277 ui.ErrorCallback? _onError; + // ignore: unused_field Zone? _onErrorZone; @override ui.ErrorCallback? get onError => _onError; diff --git a/shell/platform/fuchsia/runtime/dart/utils/BUILD.gn b/shell/platform/fuchsia/runtime/dart/utils/BUILD.gn index 6c2feca0abfc7..4537b82a24047 100644 --- a/shell/platform/fuchsia/runtime/dart/utils/BUILD.gn +++ b/shell/platform/fuchsia/runtime/dart/utils/BUILD.gn @@ -94,6 +94,7 @@ executable("dart_utils_unittests") { ":utils", "$fuchsia_sdk_root/pkg:sys_inspect_cpp", "//flutter/testing", + "//third_party/dart/runtime:libdart_jit", ] } From 8cf434fbbb7b3ae8a58fba1780fa7119f61bf264 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 5 Apr 2022 20:32:58 -0700 Subject: [PATCH 07/18] review --- lib/ui/fixtures/ui_test.dart | 6 +-- lib/ui/platform_dispatcher.dart | 6 +-- lib/ui/ui_dart_state.cc | 26 +++++-------- lib/ui/ui_dart_state.h | 1 - lib/ui/window/platform_configuration.cc | 5 ++- third_party/tonic/dart_persistent_value.cc | 2 + third_party/tonic/logging/dart_error.cc | 2 +- third_party/tonic/logging/dart_error.h | 45 +++++++++++++--------- 8 files changed, 49 insertions(+), 44 deletions(-) diff --git a/lib/ui/fixtures/ui_test.dart b/lib/ui/fixtures/ui_test.dart index f1ef5fab0b0cd..032e227b870c3 100644 --- a/lib/ui/fixtures/ui_test.dart +++ b/lib/ui/fixtures/ui_test.dart @@ -403,10 +403,10 @@ void hooksTests() { runZoned(() { originalZone = Zone.current; - PlatformDispatcher.instance.onError = (Object e, StackTrace? s) { + PlatformDispatcher.instance.onError = (Object exception, StackTrace? stackTrace) { callbackZone = Zone.current; - expectIdentical(e, error); - expectNotEquals(s, null); + expectIdentical(exception, error); + expectNotEquals(stackTrace, null); return true; }; }); diff --git a/lib/ui/platform_dispatcher.dart b/lib/ui/platform_dispatcher.dart index 2d4ae32c19b0c..b617044ef8d8b 100644 --- a/lib/ui/platform_dispatcher.dart +++ b/lib/ui/platform_dispatcher.dart @@ -1033,9 +1033,9 @@ class PlatformDispatcher { /// `Settings::unhandled_exception_callback`. /// /// The VM or the process may exit or become unresponsive after calling this - /// method. The method will not be called for exceptions that cause the VM or - /// process to terminate or become unresponsive before the method can be - /// called. + /// 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. ErrorCallback? get onError => _onError; set onError(ErrorCallback? callback) { _onError = callback; diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc index 144b2f5257476..e12ef178e04e7 100644 --- a/lib/ui/ui_dart_state.cc +++ b/lib/ui/ui_dart_state.cc @@ -62,7 +62,6 @@ UIDartState::UIDartState( remove_callback_(std::move(remove_callback)), logger_prefix_(std::move(logger_prefix)), is_root_isolate_(is_root_isolate), - unhandled_exception_callback_(unhandled_exception_callback), log_message_callback_(log_message_callback), isolate_name_server_(std::move(isolate_name_server)), enable_skparagraph_(enable_skparagraph), @@ -70,8 +69,16 @@ UIDartState::UIDartState( context_(std::move(context)) { AddOrRemoveTaskObserver(true /* add */); tonic::SetUnhandledExceptionReporter( - [&](const std::string& error, const std::string& stack_trace) { - ReportUnhandledException(error, stack_trace); + [callback = std::move(unhandled_exception_callback)]( + const std::string& error, const std::string& stack_trace) { + if (callback && 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; }); } @@ -190,19 +197,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_) { diff --git a/lib/ui/ui_dart_state.h b/lib/ui/ui_dart_state.h index 8269ba34c9620..fac14bdcc68f4 100644 --- a/lib/ui/ui_dart_state.h +++ b/lib/ui/ui_dart_state.h @@ -184,7 +184,6 @@ class UIDartState : public tonic::DartState { std::string debug_name_; std::unique_ptr platform_configuration_; tonic::DartMicrotaskQueue microtask_queue_; - UnhandledExceptionCallback unhandled_exception_callback_; LogMessageCallback log_message_callback_; const std::shared_ptr isolate_name_server_; const bool enable_skparagraph_; diff --git a/lib/ui/window/platform_configuration.cc b/lib/ui/window/platform_configuration.cc index b9094d3765438..c8c7e559c2863 100644 --- a/lib/ui/window/platform_configuration.cc +++ b/lib/ui/window/platform_configuration.cc @@ -174,8 +174,9 @@ PlatformConfiguration::~PlatformConfiguration() {} void PlatformConfiguration::DidCreateIsolate() { Dart_Handle library = Dart_LookupLibrary(tonic::ToDart("dart:ui")); - tonic::SetErrorHandler(tonic::DartState::Current(), - Dart_GetField(library, tonic::ToDart("_onError"))); + tonic::SetUnhandledErrorHandler( + tonic::DartState::Current(), + Dart_GetField(library, tonic::ToDart("_onError"))); update_locales_.Set(tonic::DartState::Current(), Dart_GetField(library, tonic::ToDart("_updateLocales"))); diff --git a/third_party/tonic/dart_persistent_value.cc b/third_party/tonic/dart_persistent_value.cc index 4c19b46946ddc..0026d7206a420 100644 --- a/third_party/tonic/dart_persistent_value.cc +++ b/third_party/tonic/dart_persistent_value.cc @@ -40,6 +40,8 @@ void DartPersistentValue::Clear() { auto dart_state = dart_state_.lock(); if (!dart_state) { + // The Dart isolate was collected and the persistent value has been + // collected with it. value_ is a dangling reference. value_ = nullptr; return; } diff --git a/third_party/tonic/logging/dart_error.cc b/third_party/tonic/logging/dart_error.cc index 26eeddccc010c..784636dfdfbc3 100644 --- a/third_party/tonic/logging/dart_error.cc +++ b/third_party/tonic/logging/dart_error.cc @@ -24,7 +24,7 @@ void ReportUnhandledException(Dart_Handle exception_handle, } } // namespace DartError -void SetErrorHandler(DartState* dart_state, Dart_Handle value) { +void SetUnhandledErrorHandler(DartState* dart_state, Dart_Handle value) { DartError::on_error.Clear(); if (Dart_IsNull(value)) { return; diff --git a/third_party/tonic/logging/dart_error.h b/third_party/tonic/logging/dart_error.h index 392f51d4c17c1..62ae33dd47c68 100644 --- a/third_party/tonic/logging/dart_error.h +++ b/third_party/tonic/logging/dart_error.h @@ -16,8 +16,6 @@ using UnhandledExceptionReporter = std::function; extern const char kInvalidArgument[]; -extern DartPersistentValue on_error; -extern UnhandledExceptionReporter log_unhandled_exception; } // namespace DartError /// Check if a Dart_Handle is an error or exception. @@ -25,20 +23,25 @@ extern UnhandledExceptionReporter log_unhandled_exception; /// 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 -/// Dart code set in |SetErrorHandler|. If that closure returns false, a -/// fallback is made to the closure in |SetUnhandledExceptionReporter| to log -/// details of the exception and stack. If that closure throws an exception -/// itself, the exception and stack are logged via the fallback method, and the -/// original exception and stack are also logged via the fallback method. +/// Dart code set in |SetUnhandledErrorHandler|. If that closure returns false +/// or throws an exception, a fallback is made to the closure in +/// |SetUnhandledExceptionReporter| to log details of the exception and stack. +/// If the |SetUnhandledErrorHandler| callback throws an exception, the +/// |SetUnhandledExceptionReporter| will be called with at least two separate +/// exceptions and stacktraces: one for the original exception, and one for the +/// exception thrown in the callback. /// /// The fallback behavior matches the behavior of Flutter applications before /// the introduction of PlatformDispatcher.onError. /// -/// If it is not an unhandled exception, it is logged to stderr or some similar -/// mechanism provided by the platform. Depending on which type of error occurs, -/// the process may crash and the Dart isolate may be unusable. Errors that fall -/// into this category include compilation errors, Dart API errors, and unwind -/// errors that will terminate the Dart VM. +/// Dart has errors that are not considered unhandled excpetions, such as +/// Dart_* API usage errors. In these cases, `Dart_IsUnhandledException` returns +/// false but `Dart_IsError` returns true. Such errors are logged to stderr or +/// some similar mechanism provided by the platform such as logcat on Android. +/// Depending on which type of error occurs, the process may crash and the Dart +/// isolate may be unusable. Errors that fall into this category include +/// compilation errors, Dart API errors, and unwind errors that will terminate +/// the Dart VM. /// /// Historically known as LogIfError. bool CheckAndHandleError(Dart_Handle handle); @@ -49,17 +52,23 @@ bool CheckAndHandleError(Dart_Handle handle); /// The signature of this field must match that of _onError in hooks.dart, /// namely `bool _onError(Object error, StackTrace? stackTrace)`. /// -/// This handler will be invoked when an unhandled error occurs. If it returns -/// false or throws an exception, a fallback handler will be invoked. By +/// This handler will be invoked when an unhandled exception occurs. If it +/// returns false or throws an exception, a fallback handler will be invoked. By /// default, UIDartState registers its ReportUnhandledError and will either call /// a closure provided in the Shell Settings or simply dump the error to stderr /// or some similar platform specific mechanism. -void SetErrorHandler(DartState* dart_state, Dart_Handle value); +/// +/// This must be called from the UI thread for the dart_state. +/// +/// See also: +/// - CheckAndHandleError +/// - SetUnhandledExceptionReporter +void SetUnhandledErrorHandler(DartState* dart_state, Dart_Handle value); -/// The fallback mechanism to log errors if |SetErrorHandler| has provided a -/// null value or a closure that returns false. +/// The fallback mechanism to log errors if |SetUnhandledErrorHandler| has +/// provided a null value or a closure that returns false. /// -/// Normally, it UIDartState registers with this method in its constructor. +/// Normally, UIDartState registers with this method in its constructor. void SetUnhandledExceptionReporter( DartError::UnhandledExceptionReporter reporter); From 1fd91cca15b90d1f76101586d15721d7057b05fb Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 6 Apr 2022 14:58:11 -0700 Subject: [PATCH 08/18] review --- lib/ui/hooks.dart | 6 +++--- lib/ui/platform_dispatcher.dart | 12 ++++++------ lib/ui/window/platform_configuration_unittests.cc | 4 +++- third_party/tonic/logging/dart_error.cc | 2 +- third_party/tonic/logging/dart_error.h | 2 +- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/lib/ui/hooks.dart b/lib/ui/hooks.dart index 0a7a5c776c54e..b2c3619272f71 100644 --- a/lib/ui/hooks.dart +++ b/lib/ui/hooks.dart @@ -117,7 +117,7 @@ void _drawFrame() { @pragma('vm:entry-point') bool _onError(Object error, StackTrace? stackTrace) { - return PlatformDispatcher.instance._dispatchError(error, stackTrace); + return PlatformDispatcher.instance._dispatchError(error, stackTrace ?? StackTrace.empty); } // ignore: always_declare_return_types, prefer_generic_function_type_aliases @@ -125,8 +125,8 @@ typedef _ListStringArgFunction(List args); @pragma('vm:entry-point') void _runMain(Function startMainIsolateFunction, - Function userMainFunction, - List args) { + Function userMainFunction, + List args) { startMainIsolateFunction(() { if (userMainFunction is _ListStringArgFunction) { userMainFunction(args); diff --git a/lib/ui/platform_dispatcher.dart b/lib/ui/platform_dispatcher.dart index b617044ef8d8b..241d5774f09fd 100644 --- a/lib/ui/platform_dispatcher.dart +++ b/lib/ui/platform_dispatcher.dart @@ -61,7 +61,7 @@ typedef PlatformConfigurationChangedCallback = void Function(PlatformConfigurati /// 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); +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; @@ -1027,9 +1027,9 @@ class PlatformDispatcher { /// 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 - /// must return false and a fallback mechanism such as printing to stderr may - /// be used, as configured by the specific platform embedding via + /// 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 @@ -1042,7 +1042,7 @@ class PlatformDispatcher { _onErrorZone = Zone.current; } - bool _dispatchError(Object error, StackTrace? stackTrace) { + bool _dispatchError(Object error, StackTrace stackTrace) { if (_onError == null) { return false; } @@ -1052,7 +1052,7 @@ class PlatformDispatcher { return _onError!(error, stackTrace); } else { try { - return _onErrorZone!.runBinary(_onError!, error, stackTrace); + return _onErrorZone!.runBinary(_onError!, error, stackTrace); } catch (e, s) { _onErrorZone!.handleUncaughtError(e, s); return false; diff --git a/lib/ui/window/platform_configuration_unittests.cc b/lib/ui/window/platform_configuration_unittests.cc index 530a092d79a26..25731556e1a0a 100644 --- a/lib/ui/window/platform_configuration_unittests.cc +++ b/lib/ui/window/platform_configuration_unittests.cc @@ -15,6 +15,8 @@ #include "flutter/shell/common/shell_test.h" #include "flutter/shell/common/thread_host.h" #include "flutter/testing/testing.h" +#include "gmock/gmock.h" +#include "gtest/gtest-matchers.h" namespace flutter { namespace testing { @@ -211,7 +213,7 @@ TEST_F(ShellTest, PlatformConfigurationOnErrorDoesNotHandleError) { ASSERT_EQ(throw_count, 1ul); ASSERT_EQ(ex, "Exception: false") << ex; - ASSERT_EQ(st.rfind("#0 customOnErrorFalse", 0), 0ul) << st; + EXPECT_THAT(st, ::testing::MatchesRegex("^#0.+customOnErrorFalse.+")) << st; DestroyShell(std::move(shell), std::move(task_runners)); } diff --git a/third_party/tonic/logging/dart_error.cc b/third_party/tonic/logging/dart_error.cc index 784636dfdfbc3..e3f77fee78f4b 100644 --- a/third_party/tonic/logging/dart_error.cc +++ b/third_party/tonic/logging/dart_error.cc @@ -51,7 +51,7 @@ bool CheckAndHandleError(Dart_Handle handle) { // If it is not null, defer to the return value of that closure // to determine whether to report via logging. bool handled = false; - if (on_error) { + if (on_error && !Dart_IsNull(on_error)) { Dart_Handle args[2]; args[0] = exception_handle; args[1] = stack_trace_handle; diff --git a/third_party/tonic/logging/dart_error.h b/third_party/tonic/logging/dart_error.h index 62ae33dd47c68..e5aaff20c9fa1 100644 --- a/third_party/tonic/logging/dart_error.h +++ b/third_party/tonic/logging/dart_error.h @@ -34,7 +34,7 @@ extern const char kInvalidArgument[]; /// The fallback behavior matches the behavior of Flutter applications before /// the introduction of PlatformDispatcher.onError. /// -/// Dart has errors that are not considered unhandled excpetions, such as +/// Dart has errors that are not considered unhandled exceptions, such as /// Dart_* API usage errors. In these cases, `Dart_IsUnhandledException` returns /// false but `Dart_IsError` returns true. Such errors are logged to stderr or /// some similar mechanism provided by the platform such as logcat on Android. From 0e49a4ab005cd9f6c33cc8fef5d645fe43091062 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 7 Apr 2022 22:06:51 -0700 Subject: [PATCH 09/18] Make sure the persistent values get collected and make ties to isolate more clear --- lib/ui/ui_dart_state.cc | 57 ++++++++++++++++++--- lib/ui/window/platform_configuration.cc | 5 +- lib/ui/window/platform_configuration.h | 3 ++ shell/common/fixtures/shell_test.dart | 26 ++++++++++ shell/common/shell_unittests.cc | 68 +++++++++++++++++++++++++ third_party/tonic/logging/dart_error.cc | 46 ++--------------- third_party/tonic/logging/dart_error.h | 31 +++-------- 7 files changed, 158 insertions(+), 78 deletions(-) diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc index e12ef178e04e7..3b8597de52b87 100644 --- a/lib/ui/ui_dart_state.cc +++ b/lib/ui/ui_dart_state.cc @@ -24,6 +24,26 @@ using tonic::ToDart; namespace flutter { +namespace { +void LogUnhandledException(const UnhandledExceptionCallback& callback, + Dart_Handle exception_handle, + Dart_Handle stack_trace_handle) { + const std::string error = + tonic::StdStringFromDart(Dart_ToString(exception_handle)); + const std::string stack_trace = + tonic::StdStringFromDart(Dart_ToString(stack_trace_handle)); + + if (callback && 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; +} +} // namespace + UIDartState::Context::Context(const TaskRunners& task_runners) : task_runners(task_runners) {} @@ -70,15 +90,36 @@ UIDartState::UIDartState( AddOrRemoveTaskObserver(true /* add */); tonic::SetUnhandledExceptionReporter( [callback = std::move(unhandled_exception_callback)]( - const std::string& error, const std::string& stack_trace) { - if (callback && callback(error, stack_trace)) { - return; + Dart_Handle exception_handle, Dart_Handle stack_trace_handle) { + // Hooks.dart will call the error handler on PlatformDispatcher if it is + // not null. If it is null, returns false, fall into the !handled branch + // below and log. + // If it is not null, defer to the return value of that closure + // to determine whether to report via logging. + bool handled = false; + auto on_error = + UIDartState::Current()->platform_configuration()->on_error(); + if (on_error && !Dart_IsNull(on_error)) { + Dart_Handle args[2]; + args[0] = exception_handle; + args[1] = stack_trace_handle; + Dart_Handle on_error_result = Dart_InvokeClosure(on_error, 2, args); + + // An exception was thrown by the exception handler. + if (Dart_IsError(on_error_result)) { + LogUnhandledException(callback, + Dart_ErrorGetException(on_error_result), + Dart_ErrorGetStackTrace(on_error_result)); + + handled = false; + } else { + handled = tonic::DartConverter::FromDart(on_error_result); + } + if (!handled) { + LogUnhandledException(callback, exception_handle, + stack_trace_handle); + } } - - // 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; }); } diff --git a/lib/ui/window/platform_configuration.cc b/lib/ui/window/platform_configuration.cc index c8c7e559c2863..ba5febd18ee2d 100644 --- a/lib/ui/window/platform_configuration.cc +++ b/lib/ui/window/platform_configuration.cc @@ -174,10 +174,9 @@ PlatformConfiguration::~PlatformConfiguration() {} void PlatformConfiguration::DidCreateIsolate() { Dart_Handle library = Dart_LookupLibrary(tonic::ToDart("dart:ui")); - tonic::SetUnhandledErrorHandler( - tonic::DartState::Current(), - Dart_GetField(library, tonic::ToDart("_onError"))); + on_error_.Set(tonic::DartState::Current(), + Dart_GetField(library, tonic::ToDart("_onError"))); update_locales_.Set(tonic::DartState::Current(), Dart_GetField(library, tonic::ToDart("_updateLocales"))); update_user_settings_data_.Set( diff --git a/lib/ui/window/platform_configuration.h b/lib/ui/window/platform_configuration.h index d112b2dcc002e..a8f3cf4ab3ac8 100644 --- a/lib/ui/window/platform_configuration.h +++ b/lib/ui/window/platform_configuration.h @@ -420,8 +420,11 @@ class PlatformConfiguration final { /// void CompletePlatformMessageEmptyResponse(int response_id); + Dart_Handle on_error() { return on_error_.Get(); } + private: PlatformConfigurationClient* client_; + tonic::DartPersistentValue on_error_; tonic::DartPersistentValue update_locales_; tonic::DartPersistentValue update_user_settings_data_; tonic::DartPersistentValue update_lifecycle_state_; diff --git a/shell/common/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index e2e9c2f08a0fd..1a831bbce644a 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -13,6 +13,32 @@ void nativeReportTimingsCallback(List timings) native 'NativeReportTimingsC void nativeOnBeginFrame(int microseconds) native 'NativeOnBeginFrame'; void nativeOnPointerDataPacket(List sequences) native 'NativeOnPointerDataPacket'; +@pragma('vm:entry-point') +void onErrorA() { + print('A is starting'); + PlatformDispatcher.instance.onError = (Object error, StackTrace? stack) { + print('A $error'); + notifyErrorA(error.toString()); + return true; + }; + Future.delayed(const Duration(seconds: 2)).then((_) { + throw Exception('I should be coming from A'); + }); +} + +@pragma('vm:entry-point') +void onErrorB() { + print('B is starting'); + PlatformDispatcher.instance.onError = (Object error, StackTrace? stack) { + print('B $error'); + notifyErrorB(error.toString()); + return true; + }; + throw Exception('I should be coming from B'); +} + +void notifyErrorA(String message) native 'NotifyErrorA'; +void notifyErrorB(String message) native 'NotifyErrorB'; @pragma('vm:entry-point') void drawFrames() { diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 34669c28fad9e..51392683e9ea7 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -3721,6 +3721,74 @@ TEST_F(ShellTest, UsesPlatformMessageHandler) { DestroyShell(std::move(shell)); } +TEST_F(ShellTest, SpawnWorksWithOnError) { + auto settings = CreateSettingsForFixture(); + auto shell = CreateShell(settings); + ASSERT_TRUE(ValidateShell(shell.get())); + + auto configuration = RunConfiguration::InferFromSettings(settings); + ASSERT_TRUE(configuration.IsValid()); + configuration.SetEntrypoint("onErrorA"); + + auto second_configuration = RunConfiguration::InferFromSettings(settings); + ASSERT_TRUE(second_configuration.IsValid()); + second_configuration.SetEntrypoint("onErrorB"); + + fml::CountDownLatch latch(2); + + AddNativeCallback( + "NotifyErrorA", CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { + auto string_handle = Dart_GetNativeArgument(args, 0); + const char* c_str; + Dart_StringToCString(string_handle, &c_str); + EXPECT_STREQ(c_str, "Exception: I should be coming from A"); + latch.CountDown(); + })); + + AddNativeCallback( + "NotifyErrorB", CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { + auto string_handle = Dart_GetNativeArgument(args, 0); + const char* c_str; + Dart_StringToCString(string_handle, &c_str); + EXPECT_STREQ(c_str, "Exception: I should be coming from B"); + latch.CountDown(); + })); + + RunEngine(shell.get(), std::move(configuration)); + + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + + PostSync( + shell->GetTaskRunners().GetPlatformTaskRunner(), + [this, &spawner = shell, &second_configuration, &latch]() { + ::testing::NiceMock platform_view_delegate; + auto spawn = spawner->Spawn( + std::move(second_configuration), "", + [&platform_view_delegate](Shell& shell) { + auto result = + std::make_unique<::testing::NiceMock>( + platform_view_delegate, shell.GetTaskRunners()); + ON_CALL(*result, CreateRenderingSurface()) + .WillByDefault(::testing::Invoke([] { + return std::make_unique<::testing::NiceMock>(); + })); + return result; + }, + [](Shell& shell) { return std::make_unique(shell); }); + ASSERT_NE(nullptr, spawn.get()); + ASSERT_TRUE(ValidateShell(spawn.get())); + + // Before destroying the shell, wait for expectations of the spawned + // isolate to be met. + latch.Wait(); + + DestroyShell(std::move(spawn)); + }); + + DestroyShell(std::move(shell)); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} + } // namespace testing } // namespace flutter diff --git a/third_party/tonic/logging/dart_error.cc b/third_party/tonic/logging/dart_error.cc index e3f77fee78f4b..435e7670b83bc 100644 --- a/third_party/tonic/logging/dart_error.cc +++ b/third_party/tonic/logging/dart_error.cc @@ -10,28 +10,15 @@ namespace tonic { namespace DartError { const char kInvalidArgument[] = "Invalid argument."; -DartPersistentValue on_error; -UnhandledExceptionReporter log_unhandled_exception = [](const std::string&, - const std::string&) {}; +UnhandledExceptionReporter log_unhandled_exception = [](Dart_Handle, + Dart_Handle) {}; void ReportUnhandledException(Dart_Handle exception_handle, Dart_Handle stack_trace_handle) { - const std::string exception = - tonic::StdStringFromDart(Dart_ToString(exception_handle)); - const std::string stack_trace = - tonic::StdStringFromDart(Dart_ToString(stack_trace_handle)); - DartError::log_unhandled_exception(exception, stack_trace); + DartError::log_unhandled_exception(exception_handle, stack_trace_handle); } } // namespace DartError -void SetUnhandledErrorHandler(DartState* dart_state, Dart_Handle value) { - DartError::on_error.Clear(); - if (Dart_IsNull(value)) { - return; - } - DartError::on_error.Set(dart_state, value); -} - void SetUnhandledExceptionReporter( DartError::UnhandledExceptionReporter reporter) { DartError::log_unhandled_exception = reporter; @@ -43,33 +30,8 @@ bool CheckAndHandleError(Dart_Handle handle) { if (Dart_IsUnhandledExceptionError(handle)) { Dart_Handle exception_handle = Dart_ErrorGetException(handle); Dart_Handle stack_trace_handle = Dart_ErrorGetStackTrace(handle); - Dart_Handle on_error = DartError::on_error.Get(); - - // Hooks.dart will call the error handler on PlatformDispatcher if it is - // not null. If it is null, returns false, fall into the !handled branch - // below and log. - // If it is not null, defer to the return value of that closure - // to determine whether to report via logging. - bool handled = false; - if (on_error && !Dart_IsNull(on_error)) { - Dart_Handle args[2]; - args[0] = exception_handle; - args[1] = stack_trace_handle; - Dart_Handle on_error_result = Dart_InvokeClosure(on_error, 2, args); - // An exception was thrown by the exception handler. - if (Dart_IsError(on_error_result)) { - DartError::ReportUnhandledException( - Dart_ErrorGetException(on_error_result), - Dart_ErrorGetStackTrace(on_error_result)); - handled = false; - } else { - handled = DartConverter::FromDart(on_error_result); - } - } - if (!handled) { - DartError::ReportUnhandledException(exception_handle, stack_trace_handle); - } + DartError::ReportUnhandledException(exception_handle, stack_trace_handle); return true; } else if (Dart_IsError(handle)) { tonic::Log("Dart Error: %s", Dart_GetError(handle)); diff --git a/third_party/tonic/logging/dart_error.h b/third_party/tonic/logging/dart_error.h index e5aaff20c9fa1..f8052dc8421b6 100644 --- a/third_party/tonic/logging/dart_error.h +++ b/third_party/tonic/logging/dart_error.h @@ -13,7 +13,7 @@ namespace tonic { namespace DartError { using UnhandledExceptionReporter = - std::function; + std::function; extern const char kInvalidArgument[]; } // namespace DartError @@ -23,10 +23,10 @@ extern const char kInvalidArgument[]; /// 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 -/// Dart code set in |SetUnhandledErrorHandler|. If that closure returns false -/// or throws an exception, a fallback is made to the closure in +/// Dart platform configuration's on_error closure. If that closure returns +/// false or throws an exception, a fallback is made to the closure in /// |SetUnhandledExceptionReporter| to log details of the exception and stack. -/// If the |SetUnhandledErrorHandler| callback throws an exception, the +/// If the on_error callback throws an exception, the /// |SetUnhandledExceptionReporter| will be called with at least two separate /// exceptions and stacktraces: one for the original exception, and one for the /// exception thrown in the callback. @@ -46,27 +46,8 @@ extern const char kInvalidArgument[]; /// Historically known as LogIfError. bool CheckAndHandleError(Dart_Handle handle); -/// Provides a Dart_Handle to a top level static field closure to invoke when -/// an unhandled exception occurs. -/// -/// The signature of this field must match that of _onError in hooks.dart, -/// namely `bool _onError(Object error, StackTrace? stackTrace)`. -/// -/// This handler will be invoked when an unhandled exception occurs. If it -/// returns false or throws an exception, a fallback handler will be invoked. By -/// default, UIDartState registers its ReportUnhandledError and will either call -/// a closure provided in the Shell Settings or simply dump the error to stderr -/// or some similar platform specific mechanism. -/// -/// This must be called from the UI thread for the dart_state. -/// -/// See also: -/// - CheckAndHandleError -/// - SetUnhandledExceptionReporter -void SetUnhandledErrorHandler(DartState* dart_state, Dart_Handle value); - -/// The fallback mechanism to log errors if |SetUnhandledErrorHandler| has -/// provided a null value or a closure that returns false. +/// The fallback mechanism to log errors if the platform configuration error +/// handler returns false. /// /// Normally, UIDartState registers with this method in its constructor. void SetUnhandledExceptionReporter( From d78d102080145ad6b18fab98f917a9c968331dd1 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 7 Apr 2022 22:10:18 -0700 Subject: [PATCH 10/18] stray prints --- shell/common/fixtures/shell_test.dart | 4 ---- 1 file changed, 4 deletions(-) diff --git a/shell/common/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index 1a831bbce644a..25cbe8a7b2b7b 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -15,9 +15,7 @@ void nativeOnPointerDataPacket(List sequences) native 'NativeOnPointerDataP @pragma('vm:entry-point') void onErrorA() { - print('A is starting'); PlatformDispatcher.instance.onError = (Object error, StackTrace? stack) { - print('A $error'); notifyErrorA(error.toString()); return true; }; @@ -28,9 +26,7 @@ void onErrorA() { @pragma('vm:entry-point') void onErrorB() { - print('B is starting'); PlatformDispatcher.instance.onError = (Object error, StackTrace? stack) { - print('B $error'); notifyErrorB(error.toString()); return true; }; From 51fe9deabfd402cccc5d29261186e07cf94a007d Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 8 Apr 2022 13:09:49 -0700 Subject: [PATCH 11/18] more review --- lib/ui/ui_dart_state.cc | 70 ++++++++++++------------- lib/ui/ui_dart_state.h | 8 +-- third_party/tonic/logging/dart_error.cc | 14 ++--- third_party/tonic/logging/dart_error.h | 3 +- 4 files changed, 49 insertions(+), 46 deletions(-) diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc index 3b8597de52b87..3d04e1a75b719 100644 --- a/lib/ui/ui_dart_state.cc +++ b/lib/ui/ui_dart_state.cc @@ -25,14 +25,14 @@ using tonic::ToDart; namespace flutter { namespace { -void LogUnhandledException(const UnhandledExceptionCallback& callback, - Dart_Handle exception_handle, +void LogUnhandledException(Dart_Handle exception_handle, Dart_Handle stack_trace_handle) { const std::string error = tonic::StdStringFromDart(Dart_ToString(exception_handle)); const std::string stack_trace = tonic::StdStringFromDart(Dart_ToString(stack_trace_handle)); + auto callback = UIDartState::Current()->unhandled_exception_callback(); if (callback && callback(error, stack_trace)) { return; } @@ -42,6 +42,37 @@ void LogUnhandledException(const UnhandledExceptionCallback& callback, FML_LOG(ERROR) << "Unhandled Exception: " << error << std::endl << stack_trace; } + +void ReportUnhandledException(Dart_Handle exception_handle, + Dart_Handle stack_trace_handle) { + // Hooks.dart will call the error handler on PlatformDispatcher if it is + // not null. If it is null, returns false, fall into the !handled branch + // below and log. + // If it is not null, defer to the return value of that closure + // to determine whether to report via logging. + bool handled = false; + auto on_error = UIDartState::Current()->platform_configuration()->on_error(); + if (on_error) { + FML_DCHECK(!Dart_IsNull(on_error)); + Dart_Handle args[2]; + args[0] = exception_handle; + args[1] = stack_trace_handle; + Dart_Handle on_error_result = Dart_InvokeClosure(on_error, 2, args); + + // An exception was thrown by the exception handler. + if (Dart_IsError(on_error_result)) { + LogUnhandledException(Dart_ErrorGetException(on_error_result), + Dart_ErrorGetStackTrace(on_error_result)); + + handled = false; + } else { + handled = tonic::DartConverter::FromDart(on_error_result); + } + if (!handled) { + LogUnhandledException(exception_handle, stack_trace_handle); + } + } +} } // namespace UIDartState::Context::Context(const TaskRunners& task_runners) @@ -82,45 +113,14 @@ UIDartState::UIDartState( remove_callback_(std::move(remove_callback)), logger_prefix_(std::move(logger_prefix)), is_root_isolate_(is_root_isolate), + unhandled_exception_callback_(unhandled_exception_callback), log_message_callback_(log_message_callback), isolate_name_server_(std::move(isolate_name_server)), enable_skparagraph_(enable_skparagraph), enable_display_list_(enable_display_list), context_(std::move(context)) { AddOrRemoveTaskObserver(true /* add */); - tonic::SetUnhandledExceptionReporter( - [callback = std::move(unhandled_exception_callback)]( - Dart_Handle exception_handle, Dart_Handle stack_trace_handle) { - // Hooks.dart will call the error handler on PlatformDispatcher if it is - // not null. If it is null, returns false, fall into the !handled branch - // below and log. - // If it is not null, defer to the return value of that closure - // to determine whether to report via logging. - bool handled = false; - auto on_error = - UIDartState::Current()->platform_configuration()->on_error(); - if (on_error && !Dart_IsNull(on_error)) { - Dart_Handle args[2]; - args[0] = exception_handle; - args[1] = stack_trace_handle; - Dart_Handle on_error_result = Dart_InvokeClosure(on_error, 2, args); - - // An exception was thrown by the exception handler. - if (Dart_IsError(on_error_result)) { - LogUnhandledException(callback, - Dart_ErrorGetException(on_error_result), - Dart_ErrorGetStackTrace(on_error_result)); - - handled = false; - } else { - handled = tonic::DartConverter::FromDart(on_error_result); - } - if (!handled) { - LogUnhandledException(callback, exception_handle, - stack_trace_handle); - } - } - }); + tonic::SetUnhandledExceptionReporter(&ReportUnhandledException); } UIDartState::~UIDartState() { diff --git a/lib/ui/ui_dart_state.h b/lib/ui/ui_dart_state.h index fac14bdcc68f4..1c05304a8195a 100644 --- a/lib/ui/ui_dart_state.h +++ b/lib/ui/ui_dart_state.h @@ -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. // @@ -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, @@ -184,6 +185,7 @@ class UIDartState : public tonic::DartState { std::string debug_name_; std::unique_ptr platform_configuration_; tonic::DartMicrotaskQueue microtask_queue_; + UnhandledExceptionCallback unhandled_exception_callback_; LogMessageCallback log_message_callback_; const std::shared_ptr isolate_name_server_; const bool enable_skparagraph_; diff --git a/third_party/tonic/logging/dart_error.cc b/third_party/tonic/logging/dart_error.cc index 435e7670b83bc..365e4915acfcd 100644 --- a/third_party/tonic/logging/dart_error.cc +++ b/third_party/tonic/logging/dart_error.cc @@ -10,18 +10,20 @@ namespace tonic { namespace DartError { const char kInvalidArgument[] = "Invalid argument."; -UnhandledExceptionReporter log_unhandled_exception = [](Dart_Handle, - Dart_Handle) {}; +} // namespace DartError +namespace { +DartError::UnhandledExceptionReporter log_unhandled_exception = + [](Dart_Handle, Dart_Handle) {}; void ReportUnhandledException(Dart_Handle exception_handle, Dart_Handle stack_trace_handle) { - DartError::log_unhandled_exception(exception_handle, stack_trace_handle); + log_unhandled_exception(exception_handle, stack_trace_handle); } -} // namespace DartError +} // namespace void SetUnhandledExceptionReporter( DartError::UnhandledExceptionReporter reporter) { - DartError::log_unhandled_exception = reporter; + log_unhandled_exception = reporter; } bool CheckAndHandleError(Dart_Handle handle) { @@ -31,7 +33,7 @@ bool CheckAndHandleError(Dart_Handle handle) { Dart_Handle exception_handle = Dart_ErrorGetException(handle); Dart_Handle stack_trace_handle = Dart_ErrorGetStackTrace(handle); - DartError::ReportUnhandledException(exception_handle, stack_trace_handle); + ReportUnhandledException(exception_handle, stack_trace_handle); return true; } else if (Dart_IsError(handle)) { tonic::Log("Dart Error: %s", Dart_GetError(handle)); diff --git a/third_party/tonic/logging/dart_error.h b/third_party/tonic/logging/dart_error.h index f8052dc8421b6..773097d2aad84 100644 --- a/third_party/tonic/logging/dart_error.h +++ b/third_party/tonic/logging/dart_error.h @@ -12,8 +12,7 @@ namespace tonic { namespace DartError { -using UnhandledExceptionReporter = - std::function; +using UnhandledExceptionReporter = void(*)(Dart_Handle, Dart_Handle); extern const char kInvalidArgument[]; } // namespace DartError From 50984a99942a61fe8e715e0bde50018337a6941f Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 8 Apr 2022 13:19:17 -0700 Subject: [PATCH 12/18] defensive null checks --- lib/ui/ui_dart_state.cc | 16 +++++++++++++--- third_party/tonic/logging/dart_error.h | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc index 3d04e1a75b719..1e3c9a38cc704 100644 --- a/lib/ui/ui_dart_state.cc +++ b/lib/ui/ui_dart_state.cc @@ -32,9 +32,13 @@ void LogUnhandledException(Dart_Handle exception_handle, const std::string stack_trace = tonic::StdStringFromDart(Dart_ToString(stack_trace_handle)); - auto callback = UIDartState::Current()->unhandled_exception_callback(); - if (callback && callback(error, stack_trace)) { - return; + auto state = UIDartState::Current(); + FML_DCHECK(state); + if (state && UIDartState::Current()->unhandled_exception_callback()) { + auto callback = UIDartState::Current()->unhandled_exception_callback(); + if (callback(error, stack_trace)) { + return; + } } // Either the exception handler was not set or it could not handle the @@ -51,6 +55,12 @@ void ReportUnhandledException(Dart_Handle exception_handle, // 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 = UIDartState::Current(); + FML_DCHECK(state); + if (!state || !state->platform_configuration()) { + LogUnhandledException(exception_handle, stack_trace_handle); + return; + } auto on_error = UIDartState::Current()->platform_configuration()->on_error(); if (on_error) { FML_DCHECK(!Dart_IsNull(on_error)); diff --git a/third_party/tonic/logging/dart_error.h b/third_party/tonic/logging/dart_error.h index 773097d2aad84..dccf0601901c8 100644 --- a/third_party/tonic/logging/dart_error.h +++ b/third_party/tonic/logging/dart_error.h @@ -12,7 +12,7 @@ namespace tonic { namespace DartError { -using UnhandledExceptionReporter = void(*)(Dart_Handle, Dart_Handle); +using UnhandledExceptionReporter = void (*)(Dart_Handle, Dart_Handle); extern const char kInvalidArgument[]; } // namespace DartError From 5f713bccd88006b303d2634236776e94ae880f94 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 8 Apr 2022 14:30:12 -0700 Subject: [PATCH 13/18] no regex --- lib/ui/platform_dispatcher.dart | 8 +++++++- lib/ui/ui_dart_state.cc | 2 -- lib/ui/window/platform_configuration_unittests.cc | 4 +--- third_party/tonic/logging/dart_error.cc | 4 +++- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/ui/platform_dispatcher.dart b/lib/ui/platform_dispatcher.dart index 241d5774f09fd..0ae3d6ae49880 100644 --- a/lib/ui/platform_dispatcher.dart +++ b/lib/ui/platform_dispatcher.dart @@ -1025,7 +1025,8 @@ class PlatformDispatcher { ErrorCallback? _onError; Zone? _onErrorZone; - /// A callback that is invoked when an unhandled error occurs in Dart. + /// 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 @@ -1036,6 +1037,11 @@ class PlatformDispatcher { /// 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; diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc index 1e3c9a38cc704..4d9b341731fcf 100644 --- a/lib/ui/ui_dart_state.cc +++ b/lib/ui/ui_dart_state.cc @@ -33,7 +33,6 @@ void LogUnhandledException(Dart_Handle exception_handle, tonic::StdStringFromDart(Dart_ToString(stack_trace_handle)); auto state = UIDartState::Current(); - FML_DCHECK(state); if (state && UIDartState::Current()->unhandled_exception_callback()) { auto callback = UIDartState::Current()->unhandled_exception_callback(); if (callback(error, stack_trace)) { @@ -56,7 +55,6 @@ void ReportUnhandledException(Dart_Handle exception_handle, // to determine whether to report via logging. bool handled = false; auto state = UIDartState::Current(); - FML_DCHECK(state); if (!state || !state->platform_configuration()) { LogUnhandledException(exception_handle, stack_trace_handle); return; diff --git a/lib/ui/window/platform_configuration_unittests.cc b/lib/ui/window/platform_configuration_unittests.cc index 25731556e1a0a..0a1a8954e293d 100644 --- a/lib/ui/window/platform_configuration_unittests.cc +++ b/lib/ui/window/platform_configuration_unittests.cc @@ -15,8 +15,6 @@ #include "flutter/shell/common/shell_test.h" #include "flutter/shell/common/thread_host.h" #include "flutter/testing/testing.h" -#include "gmock/gmock.h" -#include "gtest/gtest-matchers.h" namespace flutter { namespace testing { @@ -213,7 +211,7 @@ TEST_F(ShellTest, PlatformConfigurationOnErrorDoesNotHandleError) { ASSERT_EQ(throw_count, 1ul); ASSERT_EQ(ex, "Exception: false") << ex; - EXPECT_THAT(st, ::testing::MatchesRegex("^#0.+customOnErrorFalse.+")) << st; + ASSERT_EQ(st.rfind("#0 customOnErrorFalse", 0), 0ul) << st; DestroyShell(std::move(shell), std::move(task_runners)); } diff --git a/third_party/tonic/logging/dart_error.cc b/third_party/tonic/logging/dart_error.cc index 365e4915acfcd..fba0a7bd91dfa 100644 --- a/third_party/tonic/logging/dart_error.cc +++ b/third_party/tonic/logging/dart_error.cc @@ -13,8 +13,10 @@ const char kInvalidArgument[] = "Invalid argument."; } // namespace DartError namespace { +void DefaultLogUnhandledException(Dart_Handle, Dart_Handle) {} DartError::UnhandledExceptionReporter log_unhandled_exception = - [](Dart_Handle, Dart_Handle) {}; + DefaultLogUnhandledException; + void ReportUnhandledException(Dart_Handle exception_handle, Dart_Handle stack_trace_handle) { log_unhandled_exception(exception_handle, stack_trace_handle); From d335d0f8d533c50eaad5c833fff6f5bc13a88e9c Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 8 Apr 2022 14:37:40 -0700 Subject: [PATCH 14/18] atomic fn ptr --- third_party/tonic/logging/dart_error.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/third_party/tonic/logging/dart_error.cc b/third_party/tonic/logging/dart_error.cc index fba0a7bd91dfa..c10484ca6de44 100644 --- a/third_party/tonic/logging/dart_error.cc +++ b/third_party/tonic/logging/dart_error.cc @@ -4,6 +4,8 @@ #include "tonic/logging/dart_error.h" +#include + #include "tonic/common/macros.h" #include "tonic/converter/dart_converter.h" @@ -14,18 +16,18 @@ const char kInvalidArgument[] = "Invalid argument."; namespace { void DefaultLogUnhandledException(Dart_Handle, Dart_Handle) {} -DartError::UnhandledExceptionReporter log_unhandled_exception = +std::atomic log_unhandled_exception = DefaultLogUnhandledException; void ReportUnhandledException(Dart_Handle exception_handle, Dart_Handle stack_trace_handle) { - log_unhandled_exception(exception_handle, stack_trace_handle); + log_unhandled_exception.load()(exception_handle, stack_trace_handle); } } // namespace void SetUnhandledExceptionReporter( DartError::UnhandledExceptionReporter reporter) { - log_unhandled_exception = reporter; + log_unhandled_exception.store(reporter); } bool CheckAndHandleError(Dart_Handle handle) { From 96dbb17615dc9bb05c9f3107bff80015a1445e0e Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 8 Apr 2022 14:56:00 -0700 Subject: [PATCH 15/18] lol whitespace --- lib/ui/window/platform_configuration_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ui/window/platform_configuration_unittests.cc b/lib/ui/window/platform_configuration_unittests.cc index 0a1a8954e293d..530a092d79a26 100644 --- a/lib/ui/window/platform_configuration_unittests.cc +++ b/lib/ui/window/platform_configuration_unittests.cc @@ -211,7 +211,7 @@ TEST_F(ShellTest, PlatformConfigurationOnErrorDoesNotHandleError) { ASSERT_EQ(throw_count, 1ul); ASSERT_EQ(ex, "Exception: false") << ex; - ASSERT_EQ(st.rfind("#0 customOnErrorFalse", 0), 0ul) << st; + ASSERT_EQ(st.rfind("#0 customOnErrorFalse", 0), 0ul) << st; DestroyShell(std::move(shell), std::move(task_runners)); } From ac742e23beaab0f328a42a0edde930942207ac25 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 8 Apr 2022 16:37:03 -0700 Subject: [PATCH 16/18] use DartVMInitializer, update docs --- lib/ui/ui_dart_state.cc | 60 ------------------------ runtime/dart_vm_initializer.cc | 64 ++++++++++++++++++++++++++ third_party/tonic/logging/dart_error.h | 32 +++++++++---- 3 files changed, 86 insertions(+), 70 deletions(-) diff --git a/lib/ui/ui_dart_state.cc b/lib/ui/ui_dart_state.cc index 4d9b341731fcf..62befcea4c221 100644 --- a/lib/ui/ui_dart_state.cc +++ b/lib/ui/ui_dart_state.cc @@ -24,65 +24,6 @@ using tonic::ToDart; namespace flutter { -namespace { -void LogUnhandledException(Dart_Handle exception_handle, - Dart_Handle stack_trace_handle) { - const std::string error = - tonic::StdStringFromDart(Dart_ToString(exception_handle)); - const std::string stack_trace = - tonic::StdStringFromDart(Dart_ToString(stack_trace_handle)); - - auto state = UIDartState::Current(); - if (state && UIDartState::Current()->unhandled_exception_callback()) { - auto callback = UIDartState::Current()->unhandled_exception_callback(); - if (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 ReportUnhandledException(Dart_Handle exception_handle, - Dart_Handle stack_trace_handle) { - // Hooks.dart will call the error handler on PlatformDispatcher if it is - // not null. If it is null, returns false, fall into the !handled branch - // below and log. - // 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 = UIDartState::Current(); - if (!state || !state->platform_configuration()) { - LogUnhandledException(exception_handle, stack_trace_handle); - return; - } - auto on_error = UIDartState::Current()->platform_configuration()->on_error(); - if (on_error) { - FML_DCHECK(!Dart_IsNull(on_error)); - Dart_Handle args[2]; - args[0] = exception_handle; - args[1] = stack_trace_handle; - Dart_Handle on_error_result = Dart_InvokeClosure(on_error, 2, args); - - // An exception was thrown by the exception handler. - if (Dart_IsError(on_error_result)) { - LogUnhandledException(Dart_ErrorGetException(on_error_result), - Dart_ErrorGetStackTrace(on_error_result)); - - handled = false; - } else { - handled = tonic::DartConverter::FromDart(on_error_result); - } - if (!handled) { - LogUnhandledException(exception_handle, stack_trace_handle); - } - } -} -} // namespace - UIDartState::Context::Context(const TaskRunners& task_runners) : task_runners(task_runners) {} @@ -128,7 +69,6 @@ UIDartState::UIDartState( enable_display_list_(enable_display_list), context_(std::move(context)) { AddOrRemoveTaskObserver(true /* add */); - tonic::SetUnhandledExceptionReporter(&ReportUnhandledException); } UIDartState::~UIDartState() { diff --git a/runtime/dart_vm_initializer.cc b/runtime/dart_vm_initializer.cc index 5ee62aa2302e2..28148dcbbaf19 100644 --- a/runtime/dart_vm_initializer.cc +++ b/runtime/dart_vm_initializer.cc @@ -9,11 +9,74 @@ #include "flutter/fml/logging.h" #include "flutter/fml/synchronization/shared_mutex.h" #include "flutter/fml/trace_event.h" +#include "flutter/lib/ui/ui_dart_state.h" +#include "flutter/lib/ui/window/platform_configuration.h" +#include "third_party/tonic/converter/dart_converter.h" +#include "third_party/tonic/logging/dart_error.h" +namespace { // Tracks whether Dart has been initialized and if it is safe to call Dart // APIs. static std::atomic gDartInitialized; +void LogUnhandledException(Dart_Handle exception_handle, + Dart_Handle stack_trace_handle) { + const std::string error = + tonic::StdStringFromDart(Dart_ToString(exception_handle)); + const std::string stack_trace = + tonic::StdStringFromDart(Dart_ToString(stack_trace_handle)); + + auto state = flutter::UIDartState::Current(); + if (state && state->unhandled_exception_callback()) { + auto callback = state->unhandled_exception_callback(); + if (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 ReportUnhandledException(Dart_Handle exception_handle, + Dart_Handle stack_trace_handle) { + // Hooks.dart will call the error handler on PlatformDispatcher if it is + // not null. If it is null, returns false, fall into the !handled branch + // below and log. + // 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(); + if (!state || !state->platform_configuration()) { + LogUnhandledException(exception_handle, stack_trace_handle); + return; + } + auto on_error = state->platform_configuration()->on_error(); + if (on_error) { + FML_DCHECK(!Dart_IsNull(on_error)); + Dart_Handle args[2]; + args[0] = exception_handle; + args[1] = stack_trace_handle; + Dart_Handle on_error_result = Dart_InvokeClosure(on_error, 2, args); + + // An exception was thrown by the exception handler. + if (Dart_IsError(on_error_result)) { + LogUnhandledException(Dart_ErrorGetException(on_error_result), + Dart_ErrorGetStackTrace(on_error_result)); + + handled = false; + } else { + handled = tonic::DartConverter::FromDart(on_error_result); + } + if (!handled) { + LogUnhandledException(exception_handle, stack_trace_handle); + } + } +} +} // namespace + void DartVMInitializer::Initialize(Dart_InitializeParams* params) { FML_DCHECK(!gDartInitialized); @@ -26,6 +89,7 @@ void DartVMInitializer::Initialize(Dart_InitializeParams* params) { } fml::tracing::TraceSetTimelineEventHandler(LogDartTimelineEvent); + tonic::SetUnhandledExceptionReporter(&ReportUnhandledException); } void DartVMInitializer::Cleanup() { diff --git a/third_party/tonic/logging/dart_error.h b/third_party/tonic/logging/dart_error.h index dccf0601901c8..8f723d13bc5da 100644 --- a/third_party/tonic/logging/dart_error.h +++ b/third_party/tonic/logging/dart_error.h @@ -21,17 +21,29 @@ extern const char kInvalidArgument[]; /// /// 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 -/// Dart platform configuration's on_error closure. If that closure returns -/// false or throws an exception, a fallback is made to the closure in -/// |SetUnhandledExceptionReporter| to log details of the exception and stack. -/// If the on_error callback throws an exception, the -/// |SetUnhandledExceptionReporter| will be called with at least two separate -/// exceptions and stacktraces: one for the original exception, and one for the -/// exception thrown in the callback. +/// If it is an unhandled error or exception, the closure in +/// |SetUnhandledExceptionReporter| is called. The DartVMInitializer provides +/// that closure, which checks with UIDartState::Current() if it is available +/// and falls back to simply printing the exception and stack to an error log if +/// the settings callback is not provided. /// -/// The fallback behavior matches the behavior of Flutter applications before -/// the introduction of PlatformDispatcher.onError. +/// If UIDartState::Current() is avaialble, it can provide an onError callback +/// that forwards to `PlatformConfiguration.instance.onError`. If that callback +/// is not set, the callback from `Settings.unhandled_exception_callback` is +/// invoked. If that callback is not set, a simple error log is +/// printed. +/// +/// If the PlatformDispatcher callback throws an exception, the at least two +/// separate exceptions and stacktraces will be handled by either the +/// Settings.unhandled_exception_callback or the error printer: one for the +/// original exception, and one for the exception thrown in the callback. If the +/// callback returns false, the original exception and stacktrace are logged. If +/// it returns true, no additional logging is done. +/// +/// Leaving the PlatformDispatcher.instance.onError callback unset or returning +/// false from it matches the behavior of Flutter applications before the +/// introduction of PlatformDispatcher.onError, which is to print to the error +/// log. /// /// Dart has errors that are not considered unhandled exceptions, such as /// Dart_* API usage errors. In these cases, `Dart_IsUnhandledException` returns From 6457bc83a897957706317952c889b86e835205b9 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Sat, 9 Apr 2022 00:30:57 -0700 Subject: [PATCH 17/18] typo and additional null check, test --- third_party/tonic/dart_state.cc | 3 +- third_party/tonic/logging/dart_error.h | 2 +- .../tonic/tests/dart_state_unittest.cc | 30 +++++++++++++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/third_party/tonic/dart_state.cc b/third_party/tonic/dart_state.cc index 3f37685c51be2..fbf7fdd3fd702 100644 --- a/third_party/tonic/dart_state.cc +++ b/third_party/tonic/dart_state.cc @@ -56,7 +56,8 @@ DartState* DartState::From(Dart_Isolate isolate) { DartState* DartState::Current() { auto isolate_data = static_cast*>(Dart_CurrentIsolateData()); - return isolate_data->get(); + // return isolate_data->get(); + return isolate_data ? isolate_data->get() : nullptr; } std::weak_ptr DartState::GetWeakPtr() { diff --git a/third_party/tonic/logging/dart_error.h b/third_party/tonic/logging/dart_error.h index 8f723d13bc5da..5cc5c0d5d81d0 100644 --- a/third_party/tonic/logging/dart_error.h +++ b/third_party/tonic/logging/dart_error.h @@ -27,7 +27,7 @@ extern const char kInvalidArgument[]; /// 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 +/// If UIDartState::Current() is available, it can provide an onError callback /// that forwards to `PlatformConfiguration.instance.onError`. If that callback /// is not set, the callback from `Settings.unhandled_exception_callback` is /// invoked. If that callback is not set, a simple error log is diff --git a/third_party/tonic/tests/dart_state_unittest.cc b/third_party/tonic/tests/dart_state_unittest.cc index 8ce1dfa15af7a..51a072d38c8ed 100644 --- a/third_party/tonic/tests/dart_state_unittest.cc +++ b/third_party/tonic/tests/dart_state_unittest.cc @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "flutter/third_party/tonic/dart_state.h" #include "flutter/common/task_runners.h" #include "flutter/runtime/dart_vm_lifecycle.h" #include "flutter/runtime/isolate_configuration.h" @@ -12,6 +13,35 @@ namespace testing { using DartState = FixtureTest; +TEST_F(DartState, CurrentWithNullDataDoesNotSegfault) { + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + auto settings = CreateSettingsForFixture(); + auto vm_ref = DartVMRef::Create(settings); + ASSERT_TRUE(vm_ref); + auto vm_data = vm_ref.GetVMData(); + ASSERT_TRUE(vm_data); + TaskRunners task_runners(GetCurrentTestName(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner(), // + GetCurrentTaskRunner() // + ); + auto isolate_configuration = + IsolateConfiguration::InferFromSettings(settings); + char* error; + Dart_CreateIsolateGroup( + "main.dart", "main", vm_data->GetIsolateSnapshot()->GetDataMapping(), + vm_data->GetIsolateSnapshot()->GetInstructionsMapping(), nullptr, nullptr, + nullptr, &error); + ASSERT_FALSE(error); + ::free(error); + + ASSERT_FALSE(tonic::DartState::Current()); + + Dart_ShutdownIsolate(); + ASSERT_TRUE(Dart_CurrentIsolate() == nullptr); +} + TEST_F(DartState, IsShuttingDown) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); auto settings = CreateSettingsForFixture(); From 12ebb87fa68fe9c2de7aff04a8f2d6c9b78e1e94 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Sat, 9 Apr 2022 13:23:24 -0700 Subject: [PATCH 18/18] Delete commented code, fix test to handle null safety flags properly --- third_party/tonic/dart_state.cc | 1 - third_party/tonic/tests/dart_state_unittest.cc | 15 +++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/third_party/tonic/dart_state.cc b/third_party/tonic/dart_state.cc index fbf7fdd3fd702..0b4771d08d034 100644 --- a/third_party/tonic/dart_state.cc +++ b/third_party/tonic/dart_state.cc @@ -56,7 +56,6 @@ DartState* DartState::From(Dart_Isolate isolate) { DartState* DartState::Current() { auto isolate_data = static_cast*>(Dart_CurrentIsolateData()); - // return isolate_data->get(); return isolate_data ? isolate_data->get() : nullptr; } diff --git a/third_party/tonic/tests/dart_state_unittest.cc b/third_party/tonic/tests/dart_state_unittest.cc index 51a072d38c8ed..2e4a9c616115b 100644 --- a/third_party/tonic/tests/dart_state_unittest.cc +++ b/third_party/tonic/tests/dart_state_unittest.cc @@ -16,7 +16,9 @@ using DartState = FixtureTest; TEST_F(DartState, CurrentWithNullDataDoesNotSegfault) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); auto settings = CreateSettingsForFixture(); - auto vm_ref = DartVMRef::Create(settings); + auto vm_snapshot = DartSnapshot::VMSnapshotFromSettings(settings); + auto isolate_snapshot = DartSnapshot::IsolateSnapshotFromSettings(settings); + auto vm_ref = DartVMRef::Create(settings, vm_snapshot, isolate_snapshot); ASSERT_TRUE(vm_ref); auto vm_data = vm_ref.GetVMData(); ASSERT_TRUE(vm_data); @@ -28,12 +30,17 @@ TEST_F(DartState, CurrentWithNullDataDoesNotSegfault) { ); auto isolate_configuration = IsolateConfiguration::InferFromSettings(settings); + Dart_IsolateFlags isolate_flags; + Dart_IsolateFlagsInitialize(&isolate_flags); + isolate_flags.null_safety = + isolate_configuration->IsNullSafetyEnabled(*isolate_snapshot); + isolate_flags.snapshot_is_dontneed_safe = isolate_snapshot->IsDontNeedSafe(); char* error; Dart_CreateIsolateGroup( "main.dart", "main", vm_data->GetIsolateSnapshot()->GetDataMapping(), - vm_data->GetIsolateSnapshot()->GetInstructionsMapping(), nullptr, nullptr, - nullptr, &error); - ASSERT_FALSE(error); + vm_data->GetIsolateSnapshot()->GetInstructionsMapping(), &isolate_flags, + nullptr, nullptr, &error); + ASSERT_FALSE(error) << error; ::free(error); ASSERT_FALSE(tonic::DartState::Current());