[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: RFR: 5080391: ArrayIndexOutOfBounds during "undo" of Right-to-Left text insert
From:       Phil Race <prr () openjdk ! org>
Date:       2022-09-30 20:46:19
Message-ID: svg5hS5K-OQR_85YRGgk-FHqujyV3X1R5Sj2CLfegac=.23cfd45c-56b0-44e3-ae11-832d05c07397 () github ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]