On Fri, 26 Apr 2024 22:58:37 GMT, Oliver Kopp <d...@openjdk.org> wrote:
>> Fixes https://bugs.openjdk.org/browse/JDK-8330462. >> >> If the parameter `maxLength` is larger than `Integer.MAX_VALUE - start`, >> then an addition of `start` to it leads to a negative value. This is "fixed" >> by using `Math.max` comparing the `maxLength` and `maxLength + start`. > > Oliver Kopp has updated the pull request incrementally with one additional > commit since the last revision: > > Revert using utility method In addition to the evaluation added by @jperedadnr to [JDK-8330462](https://bugs.openjdk.org/browse/JDK-8330462) - The UI Automation framework offers two sets of APIs. 1. Provider APIs : to be implemented by UI providers ( JavaFX ) 2. Client APIs : to be implemented by the Client applications like (Screen readers, DeepL) - Both providers(JavaFX) and client application should follow their respective specification. - For this scenario 1. Client Application should follow : [IUIAutomationTextRange::GetText method](https://learn.microsoft.com/en-us/windows/win32/api/uiautomationclient/nf-uiautomationclient-iuiautomationtextrange-gettext#parameters) 2. JavaFX should follow : [ITextRangeProvider::GetText](https://learn.microsoft.com/en-us/windows/win32/api/uiautomationcore/nf-uiautomationcore-itextrangeprovider-gettext#parameters) - As per both above spec, the length variable could be either -1 or a valid value. - The check we had earlier: `length != -1` was based on the spec, but seems like we need to validate against out of range check i.e. `length <= end - start`. - It seems in this scenario DeepL invokes the GetText call with INT_MAX value, which is the root cause. The check `length <= end - start` should handle that scenario. - and `length < 0` would future protect us from any negative value though as per spec `length != -1` should suffice Testing: - I could reproduce the issue with DeepL - Issue gets fixed with this fix and also with the change that I suggesting - A couple tests of negative values for start and end should be removed. modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java line 104: > 102: int length = text.length(); > 103: start = Utils.clamp(0, start, length); > 104: end = Utils.clamp(start, end, length); This is only cleanup and not required for this fix. modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java line 124: > 122: // In case there was an overflow, return the maximum end index > 123: if (res < 0) return maxEndIndex; > 124: return res; In this class, - The index **start** and **end** are guaranteed to be +ve values such that, `0 <= start <= end <= length` (length is the total length of text in the text component). : please check the method `validateRange()` - So we only need to validate that the variable `length` in this helper method is valid i.e. 1. `length > 0` and 2. `length <= end - start` /** * In the context of substrings, this is a helper method to get the end offset index * from the start index for a given length of a substring. * * @param startIndex The start index of the range in the string. Needs to be 0 or more (not checked in the code). * @param length The requested length of a substring in the range when starting from "start". * Invalid values are treated as full length. * @param endIndex The end index of the range in the string. Needs to be equal or greater than startIndex * (not checked in the code). */ static int getEndOffsetIndex(int startIndex, int length, int endIndex) { if (length < 0 || (endIndex - startIndex) <= length) { return endIndex; } return startIndex + length; } - With this change 1. The start and end index values are always positive and hence negative value tests should be removed. 2. The name of method is changed so, the Shim class would need a change too. 3. The fix could be as simple as below one line correction in the ternary statement in the GetText() method, but then we won't be able to add the test. So keeping the helper method would be good idea. `int endOffset = (length < 0 || (endIndex - startIndex) <= length) ? end : start + maxLength;` tests/system/src/test/java/test/com/sun/glass/ui/win/WinTextRangeProviderTest.java line 93: > 91: > 92: // No range check for maxEndIndex > 93: Arguments.of(-1, -1, 2, -1), negative valued tests for `start` or `end` are not required for this particular scenario as `0 <= start <= end <= total length of text` is guaranteed. ------------- Changes requested by arapte (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1442#pullrequestreview-2028389977 PR Review Comment: https://git.openjdk.org/jfx/pull/1442#discussion_r1583158450 PR Review Comment: https://git.openjdk.org/jfx/pull/1442#discussion_r1583019536 PR Review Comment: https://git.openjdk.org/jfx/pull/1442#discussion_r1583159253
