Fix the CanvasPath::CreateNew signature to avoid unnecessary Dart API::NewError calls #32791
Conversation
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Is there some way we can test this?
|
Not sure what sort of test would catch this. The error returned by Dart_GetNativeArgument is hidden inside Tonic, which is why this was not discovered until now. Tonic contains many Dart API calls without checks for errors. This has been an issue in Tonic since day one, but we don't currently have a strategy for replacing Tonic or fixing its error handling. |
|
Can we make the tonic error fatal in this case? That should fail existing tests but pass with this change |
…::NewError calls Tonic's DartCallConstructor calls Dart_GetNativeArgument based on the indices of the arguments in the native function. If the argument count of the native function does not match that of the peer Dart function, then this will result in failing Dart_GetNativeArgument calls. The Dart runtime will then do potentially expensive work to construct an error object that will be ignored.
jason-simmons
force-pushed
the
path_dart_ctor_signature
branch
from
07a89f9 to
3aea6e1
Compare
|
Added an assert to the |
|
test-exempt: optimization |
|
@Hixie fwiw I think this should count as a test. The dcheck added would cause test failures without the compensating change. It's just not directly touching a file with test in the name. |
|
(accurately detecting this as a test would be very difficult by bot though) |
jason-simmons
added
the
waiting for tree to go green
…Dart API::NewError calls (flutter/engine#32791)
…2929) * 'Update Dart SDK to e5fa9de' * Fix the CanvasPath::CreateNew signature to avoid unnecessary Dart API::NewError calls (#32791) * Update license hash Co-authored-by: Jason Simmons <[email protected]>
4 participants
Tonic's DartCallConstructor calls Dart_GetNativeArgument based on the
indices of the arguments in the native function. If the argument
count of the native function does not match that of the peer Dart
function, then this will result in failing Dart_GetNativeArgument calls.
The Dart runtime will then do potentially expensive work to construct
an error object that will be ignored.