ICU-21383 Fix memory problem in FormattedStringBuilder

This commit is contained in:
Shane F. Carr 2020-11-10 00:44:23 -06:00
parent 916932648d
commit e7f66732f8
9 changed files with 182 additions and 14 deletions

View file

@ -174,8 +174,8 @@ public:
* spanValue: the index of the list item, for example.
* length: the length of the span, used to split adjacent fields.
*/
void appendSpanInfo(int32_t spanValue, int32_t length);
void prependSpanInfo(int32_t spanValue, int32_t length);
void appendSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status);
void prependSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status);
private:
FormattedStringBuilder fString;

View file

@ -219,19 +219,35 @@ bool FormattedValueStringBuilderImpl::nextPositionImpl(ConstrainedFieldPosition&
}
U_ASSERT(currField == kUndefinedField);
// Always set the position to the end so that we don't revisit previous sections
cfpos.setState(
cfpos.getCategory(),
cfpos.getField(),
fString.fLength,
fString.fLength);
return false;
}
void FormattedValueStringBuilderImpl::appendSpanInfo(int32_t spanValue, int32_t length) {
if (spanIndices.getCapacity() <= spanValue) {
spanIndices.resize(spanValue * 2);
void FormattedValueStringBuilderImpl::appendSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status) {
if (U_FAILURE(status)) { return; }
U_ASSERT(spanIndices.getCapacity() >= spanValue);
if (spanIndices.getCapacity() == spanValue) {
if (!spanIndices.resize(spanValue * 2, spanValue)) {
status = U_MEMORY_ALLOCATION_ERROR;
return;
}
}
spanIndices[spanValue] = {spanValue, length};
}
void FormattedValueStringBuilderImpl::prependSpanInfo(int32_t spanValue, int32_t length) {
if (spanIndices.getCapacity() <= spanValue) {
spanIndices.resize(spanValue * 2);
void FormattedValueStringBuilderImpl::prependSpanInfo(int32_t spanValue, int32_t length, UErrorCode& status) {
if (U_FAILURE(status)) { return; }
U_ASSERT(spanIndices.getCapacity() >= spanValue);
if (spanIndices.getCapacity() == spanValue) {
if (!spanIndices.resize(spanValue * 2, spanValue)) {
status = U_MEMORY_ALLOCATION_ERROR;
return;
}
}
for (int32_t i = spanValue - 1; i >= 0; i--) {
spanIndices[i+1] = spanIndices[i];

View file

@ -567,7 +567,7 @@ public:
start,
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD},
status);
data->appendSpanInfo(0, start.length());
data->appendSpanInfo(0, start.length(), status);
}
}
@ -603,7 +603,7 @@ public:
next,
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD},
status);
data->appendSpanInfo(position, next.length());
data->appendSpanInfo(position, next.length(), status);
data->getStringRef().append(
temp.tempSubString(offsets[1]),
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD},
@ -622,7 +622,7 @@ public:
next,
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD},
status);
data->prependSpanInfo(position, next.length());
data->prependSpanInfo(position, next.length(), status);
data->getStringRef().insert(
0,
temp.tempSubStringBetween(0, offsets[1]),

View file

@ -237,6 +237,8 @@ void IntlTestWithFieldPosition::checkMixedFormattedValue(
}
UBool afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"A after loop: " + CFPosToUnicodeString(cfpos), afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"A after loop again: " + CFPosToUnicodeString(cfpos), afterLoopResult);
// Check nextPosition constrained over each category one at a time
for (int32_t category=0; category<UFIELD_CATEGORY_COUNT+1; category++) {
@ -266,6 +268,8 @@ void IntlTestWithFieldPosition::checkMixedFormattedValue(
}
UBool afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"B after loop @ " + CFPosToUnicodeString(cfpos), afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"B after loop again @ " + CFPosToUnicodeString(cfpos), afterLoopResult);
}
// Check nextPosition constrained over each field one at a time
@ -300,6 +304,8 @@ void IntlTestWithFieldPosition::checkMixedFormattedValue(
}
UBool afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"C after loop: " + CFPosToUnicodeString(cfpos), afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos, status);
assertFalse(baseMessage + u"C after loop again: " + CFPosToUnicodeString(cfpos), afterLoopResult);
}
}

View file

@ -551,6 +551,60 @@ void ListFormatterTest::TestFormattedValue() {
expectedFieldPositions,
UPRV_LENGTHOF(expectedFieldPositions));
}
{
LocalPointer<ListFormatter> fmt(ListFormatter::createInstance("en", ULISTFMT_TYPE_UNITS, ULISTFMT_WIDTH_SHORT, status));
if (status.errIfFailureAndReset()) { return; }
const char16_t* message = u"ICU-21383 Long list";
const char16_t* expectedString = u"a, b, c, d, e, f, g, h, i";
const UnicodeString inputs[] = {
u"a",
u"b",
u"c",
u"d",
u"e",
u"f",
u"g",
u"h",
u"i",
};
FormattedList result = fmt->formatStringsToValue(inputs, UPRV_LENGTHOF(inputs), status);
static const UFieldPositionWithCategory expectedFieldPositions[] = {
// field, begin index, end index
{UFIELD_CATEGORY_LIST_SPAN, 0, 0, 1},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 0, 1},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 1, 3},
{UFIELD_CATEGORY_LIST_SPAN, 1, 3, 4},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 3, 4},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 4, 6},
{UFIELD_CATEGORY_LIST_SPAN, 2, 6, 7},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 6, 7},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 7, 9},
{UFIELD_CATEGORY_LIST_SPAN, 3, 9, 10},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 9, 10},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 10, 12},
{UFIELD_CATEGORY_LIST_SPAN, 4, 12, 13},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 12, 13},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 13, 15},
{UFIELD_CATEGORY_LIST_SPAN, 5, 15, 16},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 15, 16},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 16, 18},
{UFIELD_CATEGORY_LIST_SPAN, 6, 18, 19},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 18, 19},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 19, 21},
{UFIELD_CATEGORY_LIST_SPAN, 7, 21, 22},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 21, 22},
{UFIELD_CATEGORY_LIST, ULISTFMT_LITERAL_FIELD, 22, 24},
{UFIELD_CATEGORY_LIST_SPAN, 8, 24, 25},
{UFIELD_CATEGORY_LIST, ULISTFMT_ELEMENT_FIELD, 24, 25},
};
checkMixedFormattedValue(
message,
result,
expectedString,
expectedFieldPositions,
UPRV_LENGTHOF(expectedFieldPositions));
}
}
void ListFormatterTest::DoTheRealListStyleTesting(Locale locale,

View file

@ -226,6 +226,12 @@ public class FormattedValueStringBuilderImpl {
}
assert currField == null;
// Always set the position to the end so that we don't revisit previous sections
cfpos.setState(
cfpos.getField(),
cfpos.getFieldValue(),
self.length,
self.length);
return false;
}

View file

@ -295,7 +295,9 @@ public class ConstrainedFieldPosition {
*/
public void setState(Field field, Object value, int start, int limit) {
// Check matchesField only as an assertion (debug build)
assert matchesField(field, value);
if (field != null) {
assert matchesField(field, value);
}
fField = field;
fValue = value;

View file

@ -215,6 +215,8 @@ public class FormattedValueTest {
}
boolean afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
// Check nextPosition constrained over each class one at a time
for (Class<?> classConstraint : uniqueFieldClasses) {
@ -238,6 +240,8 @@ public class FormattedValueTest {
}
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
}
// Check nextPosition constrained over an unrelated class
@ -267,6 +271,8 @@ public class FormattedValueTest {
}
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop: " + cfpos, afterLoopResult);
afterLoopResult = fv.nextPosition(cfpos);
assertFalse(baseMessage + "after loop again: " + cfpos, afterLoopResult);
}
}

View file

@ -217,9 +217,8 @@ public class ListFormatterTest extends TestFmwk {
@Test
public void TestFormattedValue() {
ListFormatter fmt = ListFormatter.getInstance(ULocale.ENGLISH);
{
ListFormatter fmt = ListFormatter.getInstance(ULocale.ENGLISH);
String message = "Field position test 1";
String expectedString = "hello, wonderful, and world";
String[] inputs = {
@ -244,6 +243,85 @@ public class ListFormatterTest extends TestFmwk {
expectedString,
expectedFieldPositions);
}
{
ListFormatter fmt = ListFormatter.getInstance(ULocale.CHINESE, Type.UNITS, Width.SHORT);
String message = "Field position test 2 (ICU-21340)";
String expectedString = "aabbbbbbbccc";
String inputs[] = {
"aa",
"bbbbbbb",
"ccc"
};
FormattedList result = fmt.formatToValue(Arrays.asList(inputs));
Object[][] expectedFieldPositions = {
// field, begin index, end index
{ListFormatter.SpanField.LIST_SPAN, 0, 2, 0},
{ListFormatter.Field.ELEMENT, 0, 2},
{ListFormatter.SpanField.LIST_SPAN, 2, 9, 1},
{ListFormatter.Field.ELEMENT, 2, 9},
{ListFormatter.SpanField.LIST_SPAN, 9, 12, 2},
{ListFormatter.Field.ELEMENT, 9, 12}};
if (!logKnownIssue("21351", "Java still coalesces adjacent elements")) {
FormattedValueTest.checkFormattedValue(
message,
result,
expectedString,
expectedFieldPositions);
}
}
{
ListFormatter fmt = ListFormatter.getInstance(ULocale.ENGLISH, Type.UNITS, Width.SHORT);
String message = "ICU-21383 Long list";
String expectedString = "a, b, c, d, e, f, g, h, i";
String inputs[] = {
"a",
"b",
"c",
"d",
"e",
"f",
"g",
"h",
"i",
};
FormattedList result = fmt.formatToValue(Arrays.asList(inputs));
Object[][] expectedFieldPositions = {
// field, begin index, end index
{ListFormatter.SpanField.LIST_SPAN, 0, 1, 0},
{ListFormatter.Field.ELEMENT, 0, 1},
{ListFormatter.Field.LITERAL, 1, 3},
{ListFormatter.SpanField.LIST_SPAN, 3, 4, 1},
{ListFormatter.Field.ELEMENT, 3, 4},
{ListFormatter.Field.LITERAL, 4, 6},
{ListFormatter.SpanField.LIST_SPAN, 6, 7, 2},
{ListFormatter.Field.ELEMENT, 6, 7},
{ListFormatter.Field.LITERAL, 7, 9},
{ListFormatter.SpanField.LIST_SPAN, 9, 10, 3},
{ListFormatter.Field.ELEMENT, 9, 10},
{ListFormatter.Field.LITERAL, 10, 12},
{ListFormatter.SpanField.LIST_SPAN, 12, 13, 4},
{ListFormatter.Field.ELEMENT, 12, 13},
{ListFormatter.Field.LITERAL, 13, 15},
{ListFormatter.SpanField.LIST_SPAN, 15, 16, 5},
{ListFormatter.Field.ELEMENT, 15, 16},
{ListFormatter.Field.LITERAL, 16, 18},
{ListFormatter.SpanField.LIST_SPAN, 18, 19, 6},
{ListFormatter.Field.ELEMENT, 18, 19},
{ListFormatter.Field.LITERAL, 19, 21},
{ListFormatter.SpanField.LIST_SPAN, 21, 22, 7},
{ListFormatter.Field.ELEMENT, 21, 22},
{ListFormatter.Field.LITERAL, 22, 24},
{ListFormatter.SpanField.LIST_SPAN, 24, 25, 8},
{ListFormatter.Field.ELEMENT, 24, 25},
};
FormattedValueTest.checkFormattedValue(
message,
result,
expectedString,
expectedFieldPositions);
}
}
@Test