Remove most uses of CompareInBounds
#13244
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
lhecker
reviewed
Jun 8, 2022
|
ok, this works really well. |
lhecker
reviewed
Jun 10, 2022
lhecker
approved these changes
Jun 10, 2022
ghost
requested review from
DHowett
requested changes
Jun 10, 2022
ghost
added
the
Needs-Author-Feedback
Created the 1.14 port version in #13313 |
ghost
removed
the
Needs-Author-Feedback
DHowett
reviewed
Jun 30, 2022
DHowett
approved these changes
Jun 30, 2022
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Will this make it to SV2 as well? Any stability fixes will be good to help strengthen our case for nvaccess/nvda#10964 making it into the next release! |
zadjii-msft
added
the
Needs-Discussion
|
Marking for discussion to see if this can be backported. I'm guessing that the servicing bar is too high for this 😕 but we'll discuss. |
|
This definitely can't make it into SV2; the boarding doors are closed for sure. Sorry. |
|
Makes sense, thanks for letting me know. |
|
🎉 Handy links: |
zadjii-msft
removed
the
Needs-Discussion
carlos-zamora
added a commit
that referenced
this pull request
Sep 1, 2022
This reverts commit f785168.
ghost
pushed a commit
that referenced
this pull request
Sep 2, 2022
This reverts commit f785168 (PR #13244) The error logged to NVDA was caused by the following line of code in `_getTextValue()`: `THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));` NVDA would expand a text range to encompass the document in the alt buffer. This means that the "end" would be set to the dangling "endExclusive" point (x = left, y = one past the end of the buffer). This is a valid range! However, upon extracting the text, we would hit the code above. The exclusive end doesn't actually point to anything in the buffer, so we would falsly throw `E_FAIL`. Though this could be fixed by adding a special check for the `endExclusive` in the line above, I suspect there are other places throughout the UIA code that hit this problem too. The safest course of action is to revert this commit entirely since it was a code health commit (it doesn't actually close an issue). Closes #13866
DHowett
pushed a commit
that referenced
this pull request
Sep 6, 2022
This reverts commit f785168 (PR #13244) The error logged to NVDA was caused by the following line of code in `_getTextValue()`: `THROW_HR_IF(E_FAIL, !bufferSize.IsInBounds(_start) || !bufferSize.IsInBounds(_end));` NVDA would expand a text range to encompass the document in the alt buffer. This means that the "end" would be set to the dangling "endExclusive" point (x = left, y = one past the end of the buffer). This is a valid range! However, upon extracting the text, we would hit the code above. The exclusive end doesn't actually point to anything in the buffer, so we would falsly throw `E_FAIL`. Though this could be fixed by adding a special check for the `endExclusive` in the line above, I suspect there are other places throughout the UIA code that hit this problem too. The safest course of action is to revert this commit entirely since it was a code health commit (it doesn't actually close an issue). Closes #13866 (cherry picked from commit bfd5248) Service-Card-Id: 85405833 Service-Version: 1.15
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
Replaces most uses of
Viewport::CompareInBounds()withtil::point's<and>operators.CompareInBoundshas been the cause of a bunch of UIA crashes over the years. Replacing them entirely ensures that theFAILFAST_IFisn't ever touched.Unfortunately, we still need
IncrementInBoundsandDecrementInBoundsto have support for that exclusive end.References
#13183