RFR: 5080391: ArrayIndexOutOfBounds during "undo" of Right-to-Left text insert

[Alexey Ivanov]

On Tue, 27 Sep 2022 11:16:36 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

> javax.swing.text.AbstractDocument$BranchElement.replace(...) method throws an `ArrayIndexOutOfBoundsException: arraycopy: length -1 is negative` when using an UndoManager on the default document of a JTextArea and you try to undo the insertion of a LEFT-TO-RIGHT language (e.g. Arabic) that is immediately followed by setting the component orientation on the JTextArea.
> 
> This is because System.arrayCopy() is called with -ve length because of the calculation done in AbstractDocment.replace where `src` is of 2bytes because of unicode text causing `nmove` to become -ve if `nchildren` is 1 (an unicode character is inserted)
> 
> System.arrayCopy throws `IndexOutOfBoundsException if:
> 
>     The srcPos argument is negative.
>     The destPos argument is negative.
>     The length argument is negative
> 
> 
> so the fix is made to make  nmove, src, dest +ve
> Also, Element.getElement() can return null which is not checked, causing NPE, if deletion of text is done which results in no element, which is also fixed in the PR
> 
> All jtreg testsuite tests are run without any regression.

src/java.desktop/share/classes/javax/swing/text/AbstractDocument.java line 958:

> 956:                 int index = bidiRoot.getElementIndex(p0);
> 957:                 Element bidiElem = bidiRoot.getElement(index);
> 958:                 if (bidiElem != null && bidiElem.getEndOffset() >= p1) {

Is it possible that `bidiElem` is `null`? It should never be. If it is, it is a bug in the code and throwing NPE seems good — it will be the indication of the bug.

Since the NPE has never been thrown from this code, I'd rather leave it unchanged here.

src/java.desktop/share/classes/javax/swing/text/AbstractDocument.java line 2325:

> 2323:             int src = Math.abs(offset + length);
> 2324:             int nmove = Math.abs(nchildren - src);
> 2325:             int dest = Math.abs(src + delta);

I'm rather unsure why any of these could become negative. Would it be possible that using `Math.abs` corrupts the document or leads to a data loss?

The thing is the `UndoableEdit` was produced while the document and the component didn't have `bidi` set to `true`. Inserting a right-to-left text changes that situation, and `bidiRoot` starts to contain elements. I assume changing the component orientation also modifies the contents of `bidiRoot`. As such, performing the undo operation may become impossible because the recorded state in the `UndoableEdit` is inconsistent with the current state of the document.

test/jdk/javax/swing/text/AbstractDocument/TestUndoError.java line 39:

> 37: import javax.swing.undo.UndoManager;
> 38: 
> 39: public class TestUndoError {

`TestUndoInsertArabicText`? It's more specific this way.

test/jdk/javax/swing/text/AbstractDocument/TestUndoError.java line 42:

> 40: 
> 41:     private static JTextArea textArea_;
> 42:     private static UndoManager manager_;

Is the underscore necessary at the end of the field names?

test/jdk/javax/swing/text/AbstractDocument/TestUndoError.java line 51:

> 49:                 textArea_ = new JTextArea();
> 50:                 manager_ = new UndoManager();
> 51:                 frame = new JFrame("Undo - Redo Error");

Is `frame` even required? Using `textArea` should be enough.

-------------

PR: https://git.openjdk.org/jdk/pull/10446