Concatenating strings of different widths can clobber dependent strings

VERIFIED FIXED

Status

VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: stejohns, Assigned: daumling)

Tracking

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Created attachment 374825 [details] [diff] [review]
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?
Attachment #374825 - Flags: review?(daumling)

Comment 1

10 years ago
(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.
(Assignee)

Comment 2

10 years ago
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?
Attachment #374825 - Flags: review?(daumling) → review-
(Reporter)

Comment 3

10 years ago
Created attachment 375047 [details]
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.)
Attachment #375047 - Flags: review?(daumling)
(Assignee)

Comment 4

10 years ago
Created attachment 376506 [details] [diff] [review]
Start offset for dependent strings was incorrect
Attachment #374825 - Attachment is obsolete: true
Attachment #376506 - Flags: review?(stejohns)
(Assignee)

Comment 5

10 years ago
Created attachment 376507 [details]
Testcase

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

Updated

10 years ago
Attachment #376507 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 6

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

nice, let's push the acceptance test along with this
Attachment #376506 - Flags: review?(stejohns) → review+
(Reporter)

Comment 7

10 years ago
pushed to redux as changeset:   1860:f68ec0f2da1e
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 8

9 years ago
testcase has been running in all builders, verifying fix
Status: RESOLVED → VERIFIED
(Assignee)

Comment 9

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

Because the bug is fixed, the review is obsolete.
Attachment #375047 - Flags: review?(daumling)
You need to log in before you can comment on or make changes to this bug.