Moves all argument checking of Vertices() and Vertices.raw() to asserts, Adds arg checking to all web renderers -Fixes #123085

[timmaffett]

This PR (now) moves all argument checking of Vertices() and Vertices.raw() into asserts. It also implements the checking (within asserts) on all web renderers (previously the web renderers did not check the arguments). The consensus on #123085 seemed to be that this was a change worth doing and it will definitely reduce cpu load looping over all indices in release builds. Originally this was only an optimization of the indices check, but after evaluating the code and noticing that all web renderers were never checking the arguments it seemed perfectly reasonable to move the argument checking to asserts and additionally adding the assert argument checking to all of the web renderers. It also adds tests to all platforms to verify that the assert argument checking is working correctly.

~~This PR is the most conservative thing we can do to address some of https://github.com/flutter/flutter/issues/123085 This reduces the number of ops needed to check the indices[n]*2 <= positions.length by avoiding the *2 by simply comparing indices[n] < halfPositionsLength.~~

~~Additionally, this PR also fixes the indices[] checking within the canvaskit web_ui version of Vertices.raw() (CkVertices.raw()) This routine was incorrectly testing indices[n] < positions.length instead of indices[n]*2 < positions.length. It was also missing a check to verify that positions.length was even (because it holds pairs of positions for the raw()). One note is that the checks within CkVertice() and CkVertice.raw() are using indices.any() instead of looping, and their error messages are less informative their Vertices() counterparts because of this. They also check for indices<0 where Vertices() does not. Should these be changed to be identical to Vertices() ?~~

~~I also added tests of the CkVertices() and CkVertices.raw() parameter checking - this was completely missing and is how the ckVertices.raw() was getting away with doing incorrect parameter checking. (These are essentially identical to the test that are done against Vertices() within testing\dart\painting_test.dart with the exception of the less informative error messages mentioned above).~~

~~https://github.com/flutter/flutter/issues/123085 is further discussing placing all of the Vertices.raw() parameter checks behind an assert() . Removing all checks for non-debug mode has additional considerations, while this change only optimizes operations with no change to the verification being performed.~~

~~I would be happy to add the additional assert() wrapper discussed in https://github.com/flutter/flutter/issues/123085 to the checks for a real additional performance gain :wink:~~ This has now been implemented.

Pre-launch Checklist

• [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• [X] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
• [X] I listed at least one issue that this PR fixes in the description above.
• [X] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
• [X] I updated/added relevant documentation (doc comments with ///).
• [X] I signed the [CLA].
• [X] All existing and new tests are passing.

[skyne98]

Amazing stuff, thanks a lot! Though, I would still insist on giving an option to remove the checks completely (via putting behind an assert, or parameter to disable them) because once the source of Verts can guarantee that the data is correct after some iterations, it just becomes pure overhead, for which there is no way to avoid it. And this is a pretty critical, hot path if you deal with lots of verts (lots of custom 2D meshes, for example) 👀

[timmaffett]

As it exists currently this PR is intended to optimize performance at zero change to the existing validity checks.

I agree that wrapping them in an assert like you illustrate in https://github.com/flutter/flutter/issues/123085 would be where the real performance gain would be. It sounds like at least some of the team is on board - but it does take away the check entirely. I checked within the engine and the indices array is assumed to be valid and never checked again. This would be a little different then some assert() uses however, in that it even in release mode it would be conceivable that programmatically generated indices arrays could be buggy and contain invalid indices. Therefor removing the assert does leave the underlying engine 'vulnerable' to possibly receiving invalid indices data. In practice this probably does not matter, but is probably a decision best left to the coders of the internal engine routines to weigh in on.

[skyne98]

Can we get their opinion on this somehow? I am not aware of the internal Flutter team structure, unfortunately, and for me the change sounds simple enough to follow through with just a "let's go" or two from the internal team.

[flutter-dashboard[bot]]

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

[mdebbar]

The code itself looks good to me. I'm not familiar enough though to tell if this is a change we want to make or not.

cc @goderbauer @chinmaygarde @yjbanov @hterkelsen

[timmaffett]

I have modified this PR to move the argument checking to asserts and add the assert argument checking to all of the web renderers. (Previously the web renderers never checked the arguments.)

In the discussion of https://github.com/flutter/flutter/issues/123085 both @chinmaygarde and @goderbauer seemed agree that this would be a worthwhile change. The performance of Vertices.raw() will improve for non web renderers and be identical for the web renderers. (and all web renderers now get argument checking parity with non web platforms).

[Hixie]

Is the engine resilient to bad data here, or will bad data cause a crash?

cc @chinmaygarde for a final decision on landing this. We should either decide to go for it or close the PR.

[kevmoo]

@timmaffett – we have a bit of distance to go to get this landed...I hope we do...but either way, this effort is AMAZING. Super thoughtful and thorough. "WE" (the Flutter ecosystem) are so lucky to have folks like you!

[timmaffett]

@timmaffett – we have a bit of distance to go to get this landed...I hope we do...but either way, this effort is AMAZING. Super thoughtful and thorough. "WE" (the Flutter ecosystem) are so lucky to have folks like you!

Thank you. I thought @dnfield 's fields idea to actually attempt to use the invalid vertices objects (which would be created when asserts were not enabled) would be a interesting way to possibly answer @Hixie 's question concerning about what would happen if invalid vertices objects are actually used.

[zanderso]

From PR triage: The comments about tests from @dnfield are still relevant and need to be addressed before this PR can land. Adding myself as a reviewer to route to an engine team member when the tests are updated.

[jtmcdole]

Still looking at one comment in the test file by Dan.

[jonahwilliams]

That depends on if it segfaults on real devices if the sizes or indices are mismatched.

[jonahwilliams]

If the colors or texture coordinates is shorter than positions, then we'll read past the bounds of input data. This is a potential UB. We need to validate array lengths.

https://github.com/flutter/engine/blob/main/lib/ui/painting/vertices.cc#L58-L64

If the lenght of colors or texture coordinates is too long then we just ignore the extras.

I'm not able to trigger any crashes in the index code though, I think we might be safe to make that assert only.

[jtmcdole]

We shouldn't ship this. Replacing all of the bounds checks with asserts means mismatched positions/colors could cause segfaults or other UB in profile/release builds.

Would a compile time const to move these to asserts afford someone the option of "unsafe optimizations"?

[jonahwilliams]

We removed the expensive runtime checks in https://github.com/flutter/engine/pull/53558 . The bounds checks should remain as they are not expensive (compared to copying the data into the native heap) and are required for memory safety.