Bug 145429

Summary: Crash under ICU with ASAN during editing/selection/move-by-word-visually-crash-test-5.html
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch ap: review+

Description Myles C. Maxfield 2015-05-27 22:58:34 PDT
Crash under ICU with ASAN during editing/selection/move-by-word-visually-crash-test-5.html
Comment 1 Myles C. Maxfield 2015-05-27 23:12:38 PDT
Created attachment 253836 [details]
Patch
Comment 2 Myles C. Maxfield 2015-05-27 23:13:45 PDT
<rdar://problem/20992218>
Comment 3 Alexey Proskuryakov 2015-05-28 11:16:05 PDT
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.
Comment 4 Myles C. Maxfield 2015-05-28 11:54:02 PDT
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.
Comment 5 Myles C. Maxfield 2015-05-28 15:41:42 PDT
Committed r184965: <http://trac.webkit.org/changeset/184965>
Comment 6 Alexey Proskuryakov 2015-05-28 18:07:56 PDT
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"
Comment 7 Alexey Proskuryakov 2015-05-28 21:07:12 PDT
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?
Comment 8 Myles C. Maxfield 2015-05-28 21:25:16 PDT
(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.
Comment 9 Myles C. Maxfield 2015-05-28 21:31:00 PDT
You beat me! r184976