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

[Alexey Ivanov]

On Tue, 11 Oct 2022 05:40:40 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.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Test update

This does **corrupt the document structure**.

The negative indexes are the sign that the undoable edit contradicts to the current document structure and cannot be applied therefore. Ignoring that is a call for trouble in another place.

What happens in the provided test case?

The document before the Arabic character is inserted:

--- empty ---
<paragraph>
  <content>
    [0,1][
]
<bidi root>
  <bidi level
    bidiLevel=0
  >
    [0,1][
]


This is the document dump obtained via [`AbstractDocument.dump`](https://docs.oracle.com/en/java/javase/17/docs/api/java.desktop/javax/swing/text/AbstractDocument.html#dump(java.io.PrintStream)).

After the Arabic character is inserted:

--- one char ---
<paragraph>
  <content>
    [0,2][ر
]
<bidi root>
  <bidi level
    bidiLevel=1
  >
    [0,1][ر]
  <bidi level
    bidiLevel=0
  >
    [1,2][
]


Now the text component orientation is changed to RTL:


--- orientation changed ---
<paragraph>
  <content>
    [0,2][ر
]
<bidi root>
  <bidi level
    bidiLevel=1
  >
    [0,2][ر
]


The most important change is that bidi-root now contains only one element with level 1 which contains the entire document content whereas before the orientation was changed it had contained two elements with level 1 and 0.

The undoable edit stores the data to restore the bidi-root from two leaf elements to one leaf. Yet now the bidi-root has only one leaf element. That's why the indexes in `replace` are negative: the undoable edit cannot be applied correctly.

Let's continue, as the indexes are positive now and the undo can be performed.

--- undone ---
<paragraph>
  <content>
    [0,1][
]
<bidi root>

This results in bidi-root which has no elements. This is _**not** a valid document_ any more. I [mentioned above](https://github.com/openjdk/jdk/pull/10446#discussion_r985670722) that `bidiElem` in `isLeftToRight` must never be `null`. It *is* `null` because converting negative indexes to positive in `replace` breaks the structure.

The proposed fix must not be integrated.

Is there a solution? I'm afraid there's none.

The best approach would be to invalidate all the undo information in the `UndoManager`. *Nothing can be undone* after the orientation of a text component is changed. This is true even if the edit doesn't throw an exception.

For example, if the document contains the following text: "start <Arabic-char> end". The insertion of an Arabic character is the last operation, it will be undone when `undo` is called. The component orientation was changed to RTL after inserting that Arabic character, which changes the levels in bidi-root from 0-1-0 to 2-1-2. Calling `undo` undoes the changes and restores the levels which had been before the orientation was changed, that is to 0 instead of the expected 2. This results in wrong rendering of bidirectional text.

I came up with a test case which throws `ArrayIndexOutOfBounds` before the fix is applied. With the fix applied, it throws an exception on EDT:

Exception in thread "AWT-EventQueue-0" java.lang.IllegalArgumentException: offsetLimit must be after current position
	at java.desktop/java.awt.font.LineBreakMeasurer.nextOffset(LineBreakMeasurer.java:356)
	at java.desktop/javax.swing.text.TextLayoutStrategy.getLimitingOffset(TextLayoutStrategy.java:286)
	at java.desktop/javax.swing.text.TextLayoutStrategy.createView(TextLayoutStrategy.java:194)
	at ...


What can we recommend? Set the component orientation *before* editing text and capturing undoable edits.

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

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