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?
