Fix invalid usage of typed data detected via dart_debug=true#31745
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
The presubmit test failures appear related to the change. |
a-siva
left a comment
There was a problem hiding this comment.
How do you ensure that there are no calls to Dart_ThrowException before the calls to Release() ?
|
@a-siva - when I build dart with I fixed releasing the positions typed data too soon, tests should pass on this commit. |
|
Infra CL with this patch and dart_debug = true https://luci-milo.appspot.com/raw/build/logs.chromium.org/flutter/led/dnfield_google.com/19264520801b624c8481b0625f1a6a814efe309737d92186c4e9137f63b27240/+/build.proto Took 33 with dart_debug minutes, whereas this PR took about 32 without dart_debug. |
|
|
||
| if runtime_mode == 'debug': | ||
| gn_args['dart_runtime_mode'] = 'develop' | ||
| gn_args['dart_debug'] = args.unoptimized |
There was a problem hiding this comment.
Could we do this only on linux?
There was a problem hiding this comment.
I thought I removed this. We don't need it here, only on the bot. Localnruns can opt in
| for (size_t i = 0; i < uniform_count; i++) { | ||
| uniform_floats[i] = uniforms[i]; | ||
| } | ||
| uniforms.Release(); |
There was a problem hiding this comment.
Should we always be doing an explicit Release() on typed data types when they're passed like this? If so, maybe instead of a reference type, they could be passed in such a way that their destructor complains if there wasn't an explicit Release() somewhere?
As it stands with this PR, we're relying on debug unopt unit tests having good coverage.
There was a problem hiding this comment.
We need to do an explicit release before we call any other Dart API. But if we don't call any Dart API, it's fine to not explicitly release the data.
IMHO, the tonic typed data class(es) are poorly designed, mainly because the API they are wrapping is difficult to use correctly. One thing we could try to do is avoid having automatic conversion of these in tonic, since misuse tends to happen where it's unclear that you have to free the passed in argument before you do any other dart_api calls that may allocate anything.
There was a problem hiding this comment.
Or, put differently: we could refactor the tonic API to not automatically acquire the typed data, and instead just have some way of copying the data to the desired structure since that's what we typically do with this (e.g. SkMatrix, SkData, etc.).
There was a problem hiding this comment.
I've done a couple experiments and filed flutter/flutter#99431. I think it's worth refactoring this further but it should probably be done incrementally and is a bigger task than I can take on at the moment.
zanderso
left a comment
There was a problem hiding this comment.
lgtm. In offline discussion it seemed like a good path forward is to fix the existing bug, and file an issue for a better API.
|
build_and_test_linux_unopt_debug appears to be a known flake. |
Part of flutter/flutter#99347
These errors are detected if you do a build with
--dart-debug=true. I would like to try to get this running on CI@mraleph @mkustermann @a-siva @rmacnak-google fyi
This has no tests, but the plan is to test it by adding a flag in the
linux_unoptrecipe that will cause the Dart VM to assert if these are used incorrectly. Without these changes, existing tests fail whendart_debug=truein the gn args.