FIX: ISXB-1637 Crash on memcpy when calling Press() in an InputTestFixture #2254
Conversation
…thin a test fixture context would lead to exception or crash.
…and added throw if detecting use of device control helpers being used within UnityTests in editor assemblies.
…gies/InputSystem into isxb-1637-crash-on-memcpy
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@ ## develop #2254 +/- ## =========================================== - Coverage 76.70% 76.68% -0.03% =========================================== Files 465 468 +3 Lines 87919 88172 +253 =========================================== + Hits 67442 67614 +172 - Misses 20477 20558 +81 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Packages/com.unity.inputsystem/InputSystem/Events/DeltaStateEvent.cs Outdated Show resolved Hide resolved
Packages/com.unity.inputsystem/Tests/TestFixture/InputTestFixture.cs Outdated Show resolved Hide resolved
Packages/com.unity.inputsystem/Tests/TestFixture/InputTestFixture.cs Outdated Show resolved Hide resolved
…ent.cs Co-authored-by: Anthony Yakovlev <anthony.yakovlev@gmail.com>
…ure.cs Co-authored-by: Anthony Yakovlev <anthony.yakovlev@gmail.com>
…ure.cs Co-authored-by: Anthony Yakovlev <anthony.yakovlev@gmail.com>
Co-authored-by: Anthony Yakovlev <anthony.yakovlev@gmail.com>
…sing check for SetTouch. Minor refactoring to reduce code bloat.
…gies/InputSystem into isxb-1637-crash-on-memcpy
| @K-Tone Thanks for the feedback. It was very valuable since I realised I had missed the same problem with touch APIs. It's not addressed and have been committed with additional tests to cover touch APIs as well as Vector2 API. |
| Interesting that CI behaves differently on Windows than on other platforms, some new editor tests failing. Will need to investigate on Windows I guess. |
| I am not understanding these failures. They are all green when I execute them locally on Windows 10. |
… DeltaStateEvent so that check in InputTestFixture is more direct.
| I updated the approach taken here to a more direct approach after I realised push/pop in Input manager isn't consistently pushing/popping devices and also to not manage dangling references in such devices. This is likely another area that needs attention (test scope only). |
| Re-requesting review on this one since the previous approach had flaws. Se commits from ef7b23d and forward. |
3 participants
Description
This PR addresses user reported issue: https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1637 in the following ways:
DeltaStateEvent.cswhere pointer arithmetic would be incorrect and lead to exception or crash when offsetting a nullptr.InputTestFixture.cs(via new internal propertyInputControl.hasState) where accessing a device from "outside" test context would lead to the same symptoms as above. This is now instead checked and throws exception if this is attempted.InputTestFixtureSet operations being used in an editor-assembly with [UnityTest] tests. Documented this and previously undocumented exceptions in xmldoc.InputTestFixtureTeststhat verifies normal behavior inInputTestFixtureas well as inappropriate use similar to the reported repro project and verifies that expected exceptions are thrown for these scenarios.The problem in the provided repro project is two-fold:
[OneTimeSetup]and[OneTimeTearDown]to manage test devices. This will not work as intended since the device will be added to the real Input System context, then removed as part ofInputTestFixture.Setup(). To reduce chance of similar accidental situations, I have added information recommending against this toInputTestFixturexmldoc. In addition, if this is attempted it should result in exception which is verified in the added tests.InputTestFixturein edit-mode tests[UnityTest]which isn't fully supported it seems.Notice that I decided to block use of
InputTestFixtureand its Set-family of operations in editor-assembly tests. The reason for this is thatInputManagerwill not process such updates, it will switch to editor buffers and then early out. I do not see any scenario where this may cause problems but if you do let me know and we update accordingly. The changes on this PR is intended to detect and inform about unintended or unsupported use of a test fixture.Testing status & QA
Added multiple automated edit-mode tests into
InputTestFixtureTestsOverall Product Risks
Comments to reviewers
Good to check that you are aligned on this clarification and blocking certain behavior.
Checklist
Before review:
Changed,Fixed,Addedsections.InputTestFixtureTestsDuring merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: