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

[Phil Race]

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.

There's some inconsistency in the bug report and what you pasted from it above.
I'm pretty sure this is all about RIGHT-TO-LEFT and the test is clearly inserting Arabic.

What concerns me here is that whilst using Math.abs() to make sure everything is positive is one thing, but
it isn't clear to me that the end results of the calculations are always correct.
There are various parts to this .. elements, indices into the text .. and the relationship and equivalence isn't clear to me.
If @tprinzing can take a look that would be great.

And test - which appears to be a straight copy of the sample code in the bug report is sufficient to show there's
a bug here and that you are fixing it for that one case of ONE unicode code point but I think the test needs to
be more comprehensive - inserting and then undoing at the beginning and end of some existing text.
I suspect we have no existing test which would uncover a regression being introduced here, so this test needs
to be more comprehensive.

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

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