<Swing Dev> RFR JDK-8241291: JCK test javax_swing/text/DefaultStyledDocument/ElementSpec/ESpecCtor.html fails

[Alexey Ivanov]

On 20/03/2020 08:45, Prasanta Sadhukhan wrote:
>
> Yes, logically it's possible. I am not sure how in that case, we can 
> cater to both OOM and JCK issue so backing out the fix
>
> http://cr.openjdk.java.net/~psadhukhan/8241291/webrev.1/
>
> I am not sure if JCK test is testing correctly, as the check it has is
>
>  int lengthes[] = { 0, 1, 10 };
>         int offs[] = { 0, 1, 6 };
>         String text = "The text";
>         for (int i = 0; i < types.length; i++) {
>             for (int j = 0; j < lengthes.length; j++) {
>                 for (int k = 0; k < offs.length; k++) {
>                     es = new DefaultStyledDocument.ElementSpec(a, 
> types[i], text.toCharArray(),
>                             offs[k], lengthes[j]);
>                     if (!(es != null && es.getLength() == lengthes[j]
>                             && es.getType() == types[i] 
> &&*es.getOffset() *>= 0
>                             &&*text.equals(new String(es.getArray()))*
>                             && es.getAttributes() == a)) {
>
> so it does not take into account the offset that is passed to 
> constructor, so even if I store 0 (and not 1, 6 as shown in 2nd and 
> 3rd offset) as offset in ElementSpec constructor (and dont return the 
> passed offset) the jck test pass. I guess it should test es.getOffset 
> == offs[k]
>
> Also, getArray() expects the full text irrespective of offset it pass 
> to ElementSpec.constructor, so if offset is 1, I believe we should 
> store "he text" in ElementSpec.data (in which case OOM fix would have 
> been fine) but JCK still expects "The text"
>

I agree, the JCK test should not expect the entire array to be returned. 
However, the test was correct: the ElementSpec didn't copy the chars but 
store the reference. (The test could be stricter in comparing the 
references of the passed array.)

Shall we submit a bug against JCK?


Is it a problem to store the reference to char array in ElementSpec? As 
far as I understand, ElementSpec is a short-lived object that is used to 
construct Element tree for a styled document, or an HTML document; the 
text stored in ElementSpec usually comes from a parser. As soon as the 
changes to the (HTML)Document (structure) are processed, the text is 
copied into the document itself.

If ElementSpec is long-lived object, we should copy the array, or the 
part of it; if it's a temporary object, we can store the array reference 
(as it was done before JDK-8173123). I strongly believe it's the latter. 
If the array content is changed, the “stored” text in the ElementSpec 
object is also changed, which shouldn't be a problem because the 
ElementSpec object is not used after the spec it carries is incorporated 
into the Document.


Regards,
Alexey

> Regards
> Prasanta
> On 20-Mar-20 10:27 AM, Sergey Bylokhov wrote:
>> On 3/19/20 8:15 pm, Prasanta Sadhukhan wrote:
>>> Not exactly. We are still copying the internal data before exposing 
>>> to the user/external world in ElementSpec.getArray() so user does 
>>> not get hold of our internal data structures.
>>
>> It will be possible to modify the array after passing to the 
>> constructor so the "get" method will return different results per call.
>>
>>>
>>> Regards
>>> Prasanta
>>> On 20-Mar-20 6:22 AM, Sergey Bylokhov wrote:
>>>>> Fix is to copy the full text into ElementSpec.data and store the 
>>>>> correct offset.
>>>>>
>>>>> It fixes the JCK issue and also 8241078 regression testcase. All 
>>>>> JCK javax_swing/text/DefaultStyledDocument/ testcases pass after 
>>>>> this fix.
>>>>
>>>> But this will break JDK-8173123, isn't it?
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/swing-dev/attachments/20200320/bb33414e/attachment.htm>