On Tue, 27 Sep 2022 11:16:36 GMT, Prasanta Sadhukhan <psadhuk...@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
