Bug 145429 - Crash under ICU with ASAN during editing/selection/move-by-word-visually-crash-test-5.html
Summary: Crash under ICU with ASAN during editing/selection/move-by-word-visually-cras...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-05-27 22:58 PDT by Myles C. Maxfield
Modified: 2015-05-28 21:31 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.27 KB, patch)
2015-05-27 23:12 PDT, Myles C. Maxfield
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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