490371 - Concatenating strings of different widths can clobber dependent strings

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

Attachment: Patch

String::append doesn't calculate the start correctly for dependent strings if k16 or wider. (see attachment)

Bug was found in Flash integration and is hard to replicate from pure AS3. Michael, if you agree with the suggested fix, can you suggest an acceptance test?

Comment 1

[Lars T Hansen]

(In reply to comment #0)
> 
> Bug was found in Flash integration and is hard to replicate from pure AS3.

But it is probably the kind of thing that should be straightforward to test from the selftest harness, and the acceptance test could be written on that level perhaps.

Comment 2

[Michael Daumling]

Comment on attachment 374825 [details] [diff] [review]
Patch

It should have been / thisWidth instead of * thisWidth to determine the character offset position. Anyway, the fix only enables the creation of a substring that spans the master plus additional characters. I was unable to find a test case that favored the bug, because the bug always caused the code to fall back to creating a dynamic copy. I believe that the bug is elsewhere, but for this, I would need more data. Is there a SWF that repros the bug?

Comment 3

[Steven Johnson]

Attachment: SWF that repros the bug

Enclosed is a SWF that demonstrates the issue. To repro, run the attached SWF.  Put your breakpoint at line 103 in TextElementGlue.cpp, in the method DoReplaceText.  On the 10th hit, beginIndex is 318 and you can see the bug happen. (Note, this only happens reliably on Windows; due to device-font issues the rendering is different on different platforms.)

Comment 4

[Michael Daumling]

Attachment: Start offset for dependent strings was incorrect

Comment 5

[Michael Daumling]

Attachment: Testcase

The problem was not the fix, but the testcase. This testcase reproduces the scenario inside the SWF, and it shows the bug clearly.

Comment 6

[Steven Johnson]

Comment on attachment 376506 [details] [diff] [review]
Start offset for dependent strings was incorrect

nice, let's push the acceptance test along with this

Comment 7

[Steven Johnson]

pushed to redux as changeset:   1860:f68ec0f2da1e

Comment 8

[Brent Baker]

testcase has been running in all builders, verifying fix

Comment 9

[Michael Daumling]

Comment on attachment 375047 [details]
SWF that repros the bug

Because the bug is fixed, the review is obsolete.