Bug 145429 – Crash under ICU with ASAN during editing/selection/move-by-word-visually-crash-test-5.html

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Crash under ICU with ASAN during editing/selection/move-by-word-visually-crash-test-5.html

[Myles C. Maxfield]

Created attachment 253836 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/20992218>

[Alexey Proskuryakov]

Comment on attachment 253836 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253836&action=review

> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:108
> -            uText->chunkOffset = uText->chunkLength;
> +            uText->chunkOffset = static_cast<int32_t>(index - uText->chunkNativeStart);
>              return FALSE;

Did you test both code paths? It's not obvious to me what the expectation is when FALSE is returned.

Notably, the "Already at the beginning; can't go any farther" case below sets the offset to 0.

[Myles C. Maxfield]

Comment on attachment 253836 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253836&action=review

>> Source/WebCore/platform/text/icu/UTextProviderLatin1.cpp:108
>>              return FALSE;
> 
> Did you test both code paths? It's not obvious to me what the expectation is when FALSE is returned.
> 
> Notably, the "Already at the beginning; can't go any farther" case below sets the offset to 0.

The documentation states "Returns True if the requested index could be accessed.  The chunk will contain the requested text. False value if a chunk cannot be accessed." According to the code, chunkOffset seems to be disregarded in the False case. I updated the code here for consistency with below.

[Myles C. Maxfield]

Committed r184965: <http://trac.webkit.org/changeset/184965>

[Alexey Proskuryakov]

This broke an API test: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20(Tests)?numbuilds=50

FAIL WebKit1.StringTruncator

/Volumes/Data/slave/yosemite-release/build/Tools/TestWebKitAPI/Tests/mac/StringTruncator.mm:36
Value of: "abcdef…tuvwxyz"
Expected: [[WebStringTruncator centerTruncateString:@"abcdefghijklmnopqrstuvwxyz" toWidth:100 withFont:[NSFont fontWithName:@"Helvetica" size:12]] UTF8String]
Which is: "abcdefg…tuvwxyz"

[Alexey Proskuryakov]

I landed the new results in <http://trac.webkit.org/r184975>. However, I don't know if the new results are correct, or if they are telling us about an off by one mistake in this patch.

Myles, could you please take a look?

[Myles C. Maxfield]

(In reply to comment #7)
> I landed the new results in <http://trac.webkit.org/r184975>. However, I
> don't know if the new results are correct, or if they are telling us about
> an off by one mistake in this patch.
> 
> Myles, could you please take a look?

Verified that this is a progression.

[Myles C. Maxfield]

You beat me! r184976