Check the matrix in pushTransform #8758
Conversation
|
Please add a compositing_test.dart in https://github.com/flutter/engine/tree/master/testing/dart that would have caught this regression. |
lib/ui/compositing.dart
Outdated
| throw new ArgumentError('"matrix4" argument cannot be null'); | ||
| if (matrix4.length != 16) | ||
| throw new ArgumentError('"matrix4" must have 16 entries.'); | ||
| for (double number in matrix4) { |
There was a problem hiding this comment.
if (matrix4.any((double value) => value.isInfinite || value.isNaN))
throw new ArgumentError('"matrix4" cannot have infinite or NaN elements');
liyuqian
left a comment
There was a problem hiding this comment.
Unit test added. Thanks for the suggestion of using any!
lib/ui/compositing.dart
Outdated
| throw new ArgumentError('"matrix4" argument cannot be null'); | ||
| if (matrix4.length != 16) | ||
| throw new ArgumentError('"matrix4" must have 16 entries.'); | ||
| for (double number in matrix4) { |
testing/dart/compositing_test.dart
Outdated
| matrix4[0] = double.nan; | ||
| expect( | ||
| () => builder.pushTransform(matrix4), | ||
| throwsA(TypeMatcher<ArgumentError>()) |
There was a problem hiding this comment.
nit: const TypeMatcher<ArgumentError> and below
There was a problem hiding this comment.
Done in the new version. I just realized that this comment was made before I uploaded the new patch...
lib/ui/compositing.dart
Outdated
| if (matrix4.length != 16) | ||
| throw new ArgumentError('"matrix4" must have 16 entries.'); | ||
| if (matrix4.any((double value) => value.isInfinite || value.isNaN)) | ||
| throw new ArgumentError('"matrix4" cannot have infinite or NaN elements'); |
There was a problem hiding this comment.
Can we add this to the _validateMatrix4 in painting.dart, and just call that from here?
Even though it's private, it's available here because this is all part of dart:ui
There was a problem hiding this comment.
This is outdated. We're now calling _throwIfMarix4IsInvalid.
| assert(matrix4 != null, 'Matrix4 argument was null.'); | ||
| assert(matrix4.length == 16, 'Matrix4 must have 16 entries.'); | ||
| assert(matrix4.every((double value) => value.isFinite), 'Matrix4 entries must be finite.'); | ||
| return true; |
There was a problem hiding this comment.
Wont this mean that in profile and release modes, the engine will still end up with invalid matrices?
There was a problem hiding this comment.
Yes, but as a general principle, we do a ton of validation in debug mode only via asserts.
There was a problem hiding this comment.
Yes, but we're calling _throwIfMarix4IsInvalid in pushTransform.
There was a problem hiding this comment.
This was already true - if someone decided to pass null for example, or use NaN in a Rect or Offset.
| assert(matrix4 != null, 'Matrix4 argument was null.'); | ||
| assert(matrix4.length == 16, 'Matrix4 must have 16 entries.'); | ||
| assert(matrix4.every((double value) => value.isFinite), 'Matrix4 entries must be finite.'); | ||
| return true; |
There was a problem hiding this comment.
Yes, but as a general principle, we do a ton of validation in debug mode only via asserts.
lib/ui/compositing.dart
Outdated
| throw new ArgumentError('"matrix4" argument cannot be null'); | ||
| if (matrix4.length != 16) | ||
| throw new ArgumentError('"matrix4" must have 16 entries.'); | ||
| _throwIfMatrix4IsInvalid(matrix4); |
There was a problem hiding this comment.
Rather than creating _throwIfMatrix4IsInvalid() which duplicates the checks in _matrix4IsValid(), I think we should either (a) just do an if check on _matrix4IsValid() here and throw ArgumentError saying the matrix is invalid, or (b) just call assert(_matrix4IsValid(matrix4)) here.
B is more in keeping with our existing data validation in the framework, which is to catch these types of problems at development time and optimize out the checks in release builds.
lib/ui/compositing.dart
Outdated
| /// See [pop] for details about the operation stack. | ||
| EngineLayer pushTransform(Float64List matrix4) { | ||
| _throwIfMatrix4IsInvalid(matrix4); | ||
| _matrix4IsValid(matrix4); |
There was a problem hiding this comment.
assert(_matrix4IsValid(matrix4)) just to make it clear (which works since the method also returns a bool)
c4028ef to
7bf3c86
Compare
| throwsA(const TypeMatcher<AssertionError>()), | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
(trivial nit: looks like some indentation in this file is off. maybe the finals are one space too much indented?)
|
Bonus points if you can figure out where on the wiki we should be documenting this pattern (and other things we discussed in chat). |
flutter/engine@7df8b7a...50de469 git log 7df8b7a..50de469 --no-merges --oneline 50de469 Check the matrix in pushTransform (flutter/engine#8758) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (liyuqian@google.com), and stop the roller if necessary.
Fixes flutter/flutter#31650