8330462: StringIndexOutOfBoundException when typing anything into TextField #1442

koppor · 2024-04-18T12:01:52Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8330462: StringIndexOutOfBoundException when typing anything into TextField (Bug - P2)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)
Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1442/head:pull/1442
$ git checkout pull/1442

Update a local copy of the PR:
$ git checkout pull/1442
$ git pull https://git.openjdk.org/jfx.git pull/1442/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1442

View PR using the GUI difftool:
$ git pr show -t 1442

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1442.diff

Webrev

Link to Webrev Comment

…tField

bridgekeeper · 2024-04-18T12:02:22Z

👋 Welcome back koppor! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-18T12:02:41Z

@koppor This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8330462: StringIndexOutOfBoundException when typing anything into TextField

Reviewed-by: angorya, kcr, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 8 new commits pushed to the master branch:

35880ce: 8331616: ChangeListener is not triggered when the InvalidationListener is removed
36e65e8: 8331214: Doc: update spec for SpinnerFactory classes
5400512: 8271865: SortedList::getViewIndex behaves not correctly for some index values
aa9907a: 8325591: [Mac] DRAG_DONE reports null transferMode when destination is external
cac81b5: 8331319: IME Window breaks after popup menu
7294849: 8320563: Remove D3D9 code paths in favor of D3D9Ex
398f104: 8314215: Trailing Spaces before Line Breaks Affect the Center Alignment of Text
c23ac74: 8273657: TextField: all text content must be selected initially

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@andy-goryachev-oracle, @kevinrushforth, @arapte) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mlbridge · 2024-04-18T12:06:35Z

Webrevs

andy-goryachev-oracle · 2024-04-18T14:42:13Z

/reviewers 2

openjdk · 2024-04-18T14:43:02Z

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

andy-goryachev-oracle · 2024-04-18T14:44:12Z

would this fix need to be backported? how far back?

andy-goryachev-oracle

The code looks right. I can't verify the fix with the deepl app.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java

kevinrushforth · 2024-04-18T15:17:54Z

In addition to the comment Andy made about possible other places for the check, I'd like to see a test, if possible. Also, this doesn't follow the usual pattern of checking for integer overflow, so that could be done in the utility method that Andy suggested.

/reviewers 2

openjdk · 2024-04-18T15:18:36Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

koppor · 2024-04-18T17:57:42Z

I'd like to see a test, if possible.

Where can I find hints on this? I would use TestFX, fire up some minimal app and try to hit the branches of WinTextRangeProvider. However, I did not find TestFX as used. And there is no javafx.graphics/src/test/java/test/com/sun/glass/ui/win. There is also no javafx.graphics/src/test/java/test/com/sun/javafx/scene/control for testing a TextField.

I assume, I need to create some class in test.javafx.scene.input?

Based on my experience, I would create a test for WinTextRangeProvider directly and supply some values to it to check that the "right" branches are covered.

koppor · 2024-04-18T18:20:46Z

would this fix need to be backported? how far back?

Yes - JavaFX 22 would be helpful for me.

8u20-b12 was the first tag appearing with that code. -- I checked "git blame" https://github.com/openjdk/jfx/blame/master/modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java. The commit d66a6dd#diff-17176994e11e44400ad30a7092a1fd738b268811b275884b946e11ce2448c379 changed the implementation from a dummy (return null;) to something more meaningful. - I did not find the exact releases.

Since EOL of JavaFX in Java 8 is less than 12 months from now, I would not do it.

kevinrushforth

I left one inline comment.

I'd like @arapte to review this, since he is familiar with this code, having fixed other IOOBE bugs in it.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java

kevinrushforth · 2024-04-18T22:08:35Z

Reviewers: @arapte @andy-goryachev-oracle

johanvos · 2024-04-19T07:29:54Z

Based on my experience, I would create a test for WinTextRangeProvider directly and supply some values to it to check that the "right" branches are covered.

Based on what I see from the stacktrace, this sounds good. We now know that the methods in WinTextRangeProvider can be invoked with bogus values. A good test would invoke those methods with bogus values and detect a non-catched Exception before, and no Exception after the patch. This can easily be done with a unit test, no need to fire up a minimal app.

koppor · 2024-04-19T09:59:37Z

This can easily be done with a unit test,

How to create an instance of WinTextRangeProvider with a fake WinAccessible? I need WinAccessible.getAttribute(...). Normally, I would have used Mockito... WinAccessible is final. -- I do not dare to remove final just for test subclassing ^^.

koppor · 2024-04-19T11:03:31Z

I am new to testing in the JFX project. It seems that test. is required as package prefix. Thus, I did not use the approach of package-private methods and classes, but needed to make the class and the tested method public.

However, now I get the (expected) error that the module javafx.graphics does not export com.sun.glass.ui.win to unnamed module @0x1e6454ec.

class test.com.sun.glass.ui.win.WinTextRangeProviderTest (in unnamed module @0x1e6454ec) cannot access class com.sun.glass.ui.win.WinTextRangeProvider (in module javafx.graphics) because module javafx.graphics does not export com.sun.glass.ui.win to unnamed module @0x1e6454ec
java.lang.IllegalAccessError: class test.com.sun.glass.ui.win.WinTextRangeProviderTest (in unnamed module @0x1e6454ec) cannot access class com.sun.glass.ui.win.WinTextRangeProvider (in module javafx.graphics) because module javafx.graphics does not export com.sun.glass.ui.win to unnamed module @0x1e6454ec
	at test.com.sun.glass.ui.win.WinTextRangeProviderTest.getValidStringIndex

kevinrushforth

The test gets an error on Mac and Linux. See inline comments.

kevinrushforth · 2024-05-07T16:29:33Z

tests/system/src/test/java/test/com/sun/glass/ui/win/WinTextRangeProviderTest.java

+
+    @AfterAll
+    static void shutdown() {
+        Util.shutdown();


This fails on Mac and Linux because the initialization (and consequently all tests) are skipped, but the call to Util.shutdown is not skipped. You will either need to also add assumeTrue(PlatformUtil.isWindows()); here or else move the assumeTrue to the test method.

is there a good way to skip the whole file entirely, e.g. via junit annotation?

maybe
https://stackoverflow.com/questions/26115139/junit-ignore-or-skip-entire-test-class
?

We still want to use assumeTrue as the pattern (for flexibility), so the question is whether there is a better place besides a method annotated with @BeforeAll to put it (which also means duplicating the assumption in the @AfterAll method). I'm not aware of one, but maybe someone else is?

My source: https://stackoverflow.com/q/30112317/873282, but I did totally forget about @AfterAll. I was assuming that that would also have been skipped if the @BeforeAll was skipped. 🙈

Think, for users coming from JUnit4, it is better to put the statements in each test method, even though not executed.

Junit 5 has an EnabledOnOs annotation https://junit.org/junit5/docs/5.2.0/api/org/junit/jupiter/api/condition/EnabledOnOs.html

EnabledOnOs

thank you @Siedlerchr for this info.
I wonder what criteria are set for determining the OS value, and do they match jfx ones?

They just read the os.name with `System.getProperty("os.name") but that only includes the Standard values.
https://github.com/junit-team/junit5/blob/db47616ab4ccf38ff63e8bff41050d5102c9ff15/junit-jupiter-api/src/main/java/org/junit/jupiter/api/condition/OS.java#L110

This only includes the standard OS values. However, an alternative could be to use @EnabledIf("isWindows") with a method of that name that returns a boolean

boolean isWindows() { return PlatformUtil.isWindows() }

the logic is different, but both variants should be ok for testing purposes.

for example,
junit: mac = toLowercase(ENGLISH).contains("mac")
jfx: mac = startsWith("Mac")

I prefer to stick with assumptions since that's what we use everywhere else. Also, it gives us more control.

So while we could do something else in the future, this PR isn't the place to do it.

andy-goryachev-oracle

The code changes look good to me; don't see any problems testing with Narrator on win11. I do have Deepl installed (though it is not connected due to proxy). See no issues in TextField, PasswordField, TextArea in the test app as well as the Monkey Tester.

(once the tests are fixed, I'll re-approve)

openjdk · 2024-05-07T18:48:18Z

⚠️ @koppor the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout patch-1
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

andy-goryachev-oracle · 2024-05-07T18:50:01Z

please do not integrate before tests are fixed...

kevinrushforth

Looks good. I confirm that the test passes on Windows and is skipped on macOS.

andy-goryachev-oracle

LGTM

kevinrushforth · 2024-05-08T12:23:17Z

@koppor This is now ready for you to /integrate.

koppor · 2024-05-08T12:24:24Z

/integrate

openjdk · 2024-05-08T12:25:55Z

@koppor
Your change (at version 603eb7d) is now ready to be sponsored by a Committer.

kevinrushforth · 2024-05-08T12:47:23Z

/sponsor

openjdk · 2024-05-08T12:48:03Z

Going to push as commit d3da033.
Since your change was applied there have been 8 commits pushed to the master branch:

35880ce: 8331616: ChangeListener is not triggered when the InvalidationListener is removed
36e65e8: 8331214: Doc: update spec for SpinnerFactory classes
5400512: 8271865: SortedList::getViewIndex behaves not correctly for some index values
aa9907a: 8325591: [Mac] DRAG_DONE reports null transferMode when destination is external
cac81b5: 8331319: IME Window breaks after popup menu
7294849: 8320563: Remove D3D9 code paths in favor of D3D9Ex
398f104: 8314215: Trailing Spaces before Line Breaks Affect the Center Alignment of Text
c23ac74: 8273657: TextField: all text content must be selected initially

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-08T12:48:11Z

@kevinrushforth @koppor Pushed as commit d3da033.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

koppor · 2024-05-08T13:56:43Z

@arapte @andy-goryachev-oracle @kevinrushforth Thank you for your ongoing support, deep analysis (both the issue and the algorithms needed), and code pointers and guidance. Looking forward to the next JavaFX release where this fix is included!

andy-goryachev-oracle · 2024-05-08T14:26:54Z

Thank you @koppor for fixing the issue!

kevinrushforth · 2024-05-08T14:31:36Z

Looking forward to the next JavaFX release where this fix is included!

It should be available in the next EA build of JavaFX 23. @arapte will backport it to jfx22u for JavaFX 22.0.2.

8330462: StringIndexOutOfBoundException when typing anything into Tex…

e81712c

…tField

koppor mentioned this pull request Apr 18, 2024

Hotfix maxlength koppor/jfx#2

Open

openjdk bot added the rfr Ready for review label Apr 18, 2024

This was referenced Apr 18, 2024

Uncaught exception occurred in Thread[#53,JavaFX Application Thread,5,main] (-> Run JabRef as Admin as a fix) JabRef/jabref#11151

Closed

Try JavaFX hotfix JabRef/jabref#11212

Closed

andy-goryachev-oracle approved these changes Apr 18, 2024

View reviewed changes

andy-goryachev-oracle reviewed Apr 18, 2024

View reviewed changes

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java Outdated Show resolved Hide resolved

koppor added 4 commits April 18, 2024 21:37

Introduce getValidStringIndex

ff54534

Fix handling of requestedSteps < 0

45c10e1

Streamline code

9a45303

start should be at most maxEnd

e7502d6

kevinrushforth reviewed Apr 18, 2024

View reviewed changes

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinTextRangeProvider.java Outdated Show resolved Hide resolved

kevinrushforth requested a review from arapte April 18, 2024 22:07

Revert change in validateRange(String)

561b4ae

koppor added 2 commits April 19, 2024 12:30

Add test for getValidStringIndex

76e80aa

Fix test package

f421eab

kevinrushforth requested changes May 7, 2024

View reviewed changes

andy-goryachev-oracle approved these changes May 7, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label May 7, 2024

More "assumeTrue" statements

a7436f2

openjdk bot removed the ready Ready to be integrated label May 7, 2024

Update full name

603eb7d

kevinrushforth approved these changes May 7, 2024

View reviewed changes

kevinrushforth requested review from andy-goryachev-oracle and arapte May 7, 2024 23:01

andy-goryachev-oracle approved these changes May 7, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label May 7, 2024

arapte approved these changes May 8, 2024

View reviewed changes

openjdk bot added the sponsor Ready to sponsor label May 8, 2024

openjdk bot added the integrated Pull request has been integrated label May 8, 2024

openjdk bot closed this May 8, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels May 8, 2024

koppor deleted the patch-1 branch May 8, 2024 13:09

8330462: StringIndexOutOfBoundException when typing anything into TextField #1442

8330462: StringIndexOutOfBoundException when typing anything into TextField #1442

Conversation

koppor commented Apr 18, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 18, 2024

openjdk bot commented Apr 18, 2024 • edited Loading

mlbridge bot commented Apr 18, 2024 • edited Loading

Webrevs

andy-goryachev-oracle commented Apr 18, 2024

openjdk bot commented Apr 18, 2024

andy-goryachev-oracle commented Apr 18, 2024

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

kevinrushforth commented Apr 18, 2024

openjdk bot commented Apr 18, 2024

koppor commented Apr 18, 2024

koppor commented Apr 18, 2024

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth commented Apr 18, 2024

johanvos commented Apr 19, 2024

koppor commented Apr 19, 2024

koppor commented Apr 19, 2024

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

openjdk bot commented May 7, 2024

andy-goryachev-oracle commented May 7, 2024

kevinrushforth left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

kevinrushforth commented May 8, 2024

koppor commented May 8, 2024

openjdk bot commented May 8, 2024

kevinrushforth commented May 8, 2024

openjdk bot commented May 8, 2024

openjdk bot commented May 8, 2024

koppor commented May 8, 2024

andy-goryachev-oracle commented May 8, 2024

kevinrushforth commented May 8, 2024

koppor commented Apr 18, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 18, 2024 •

edited

Loading

mlbridge bot commented Apr 18, 2024 •

edited

Loading

Siedlerchr May 7, 2024 •

edited

Loading