8303026: [TextField] IOOBE on setting text with control characters that replaces existing text #1043

jperedadnr · 2023-02-21T23:11:52Z

This PR fixes a regression after JDK-8212102.

When a TextField control has some previous content, and new text with only invalid characters (0x7F, new line, tabs, <0x20) is set, TextInputControl::filterInput is used to strip them out, and the new text has length 0.

During the refactoring in JDK-8212102, the checks:

int length = txt.length();
if (end > start + length) end = length;
if (start > length-1) start = end = 0;

were removed.

In this particular case, when the new filtered text has length 0, we need those checks, so start and end are set to 0, and txt.substring(start, end) doesn't throw the IOOBE anymore.

A test is added to verify this scenario. It fails with the proposed patch, passes with it.

Progress

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

Issue

JDK-8303026: [TextField] IOOBE on setting text with control characters that replaces existing text

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Committer)
Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1043

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

Using diff file

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

bridgekeeper · 2023-02-21T23:23:40Z

👋 Welcome back jpereda! 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.

mlbridge · 2023-02-21T23:28:02Z

Webrevs

01: Full - Incremental (cc94040c)
00: Full (0163b014)

modules/javafx.controls/src/main/java/javafx/scene/control/TextInputControl.java

andy-goryachev-oracle · 2023-02-21T23:35:32Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextFieldTest.java

@@ -543,6 +543,13 @@ public Integer fromString(String value) {
        assertEquals("200", txtField.getText());
    }

+    @Test public void stripInvalidCharacters() {


+1 for unit test!

andy-goryachev-oracle

the fix is verified with the MonkeyTester application.

arapte · 2023-03-02T09:26:45Z

The regression bug-id mentioned in PR description is incorrect: JDK-8212102 is a similar bug. and, JDK-8176270 is the regression causing bug, which correctly mentioned in JBS. I would recommend to correct the description for correct record.

The fix itself looks good.

openjdk · 2023-03-02T09:28:25Z

@jperedadnr 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:

8303026: [TextField] IOOBE on setting text with control characters that replaces existing text

Reviewed-by: angorya, 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 11 new commits pushed to the master branch:

00f1aca: Merge
17ea057: 8302470: Change JBS version in .jcheck/conf to jfxNN[.0.MM]
e44e0dc: 8302470: Change JBS version in .jcheck/conf to jfxNN[.0.MM]
0de0837: 8290866: Apple Color Emoji turns gray after JavaFX version 18
a916629: Merge
1dd0a59: 8303019: cssref.html incorrect internal link in Path
7bf2372: 8293587: Fix mistakes in FX API docs
14883a2: 8301022: Video distortion is observed while playing youtube video
a3c2205: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height
25e4ab9: 8090647: Mnemonics : on windows we should cancel the underscore latch when an app loses focus.
... and 1 more: https://git.openjdk.org/jfx/compare/21aa630a0cb956de9641574676adfe114609f666...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

jperedadnr · 2023-03-02T15:50:18Z

/integrate

openjdk · 2023-03-02T15:51:22Z

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

00f1aca: Merge
17ea057: 8302470: Change JBS version in .jcheck/conf to jfxNN[.0.MM]
e44e0dc: 8302470: Change JBS version in .jcheck/conf to jfxNN[.0.MM]
0de0837: 8290866: Apple Color Emoji turns gray after JavaFX version 18
a916629: Merge
1dd0a59: 8303019: cssref.html incorrect internal link in Path
7bf2372: 8293587: Fix mistakes in FX API docs
14883a2: 8301022: Video distortion is observed while playing youtube video
a3c2205: 8173321: TableView: Click on right trough has no effect when cell height is higher than viewport height
25e4ab9: 8090647: Mnemonics : on windows we should cancel the underscore latch when an app loses focus.
... and 1 more: https://git.openjdk.org/jfx/compare/21aa630a0cb956de9641574676adfe114609f666...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-02T15:51:36Z

@jperedadnr Pushed as commit 9df6039.

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

Restore start, end checks in updateSelectedText

0163b01

openjdk bot added the rfr Ready for review label Feb 21, 2023

andy-goryachev-oracle reviewed Feb 21, 2023

View reviewed changes

add curly braces

cc94040

kevinrushforth requested a review from arapte February 22, 2023 12:55

andy-goryachev-oracle approved these changes Feb 22, 2023

View reviewed changes

arapte approved these changes Mar 2, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Mar 2, 2023

openjdk bot added the integrated Pull request has been integrated label Mar 2, 2023

openjdk bot closed this Mar 2, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8303026: [TextField] IOOBE on setting text with control characters that replaces existing text #1043

8303026: [TextField] IOOBE on setting text with control characters that replaces existing text #1043

jperedadnr commented Feb 21, 2023 •

edited by openjdk bot

bridgekeeper bot commented Feb 21, 2023

mlbridge bot commented Feb 21, 2023 •

edited

andy-goryachev-oracle Feb 21, 2023

andy-goryachev-oracle left a comment

arapte commented Mar 2, 2023

openjdk bot commented Mar 2, 2023

jperedadnr commented Mar 2, 2023

openjdk bot commented Mar 2, 2023

openjdk bot commented Mar 2, 2023

8303026: [TextField] IOOBE on setting text with control characters that replaces existing text #1043

8303026: [TextField] IOOBE on setting text with control characters that replaces existing text #1043

Conversation

jperedadnr commented Feb 21, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 21, 2023

mlbridge bot commented Feb 21, 2023 • edited

Webrevs

andy-goryachev-oracle Feb 21, 2023

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

arapte commented Mar 2, 2023

openjdk bot commented Mar 2, 2023

jperedadnr commented Mar 2, 2023

openjdk bot commented Mar 2, 2023

openjdk bot commented Mar 2, 2023

jperedadnr commented Feb 21, 2023 •

edited by openjdk bot

mlbridge bot commented Feb 21, 2023 •

edited