Bug 235413 - [fast-cq] [macOS] Various tests hit debug assertions under `SearchBuffer::search` after system ICU changes
Summary: [fast-cq] [macOS] Various tests hit debug assertions under `SearchBuffer::sea...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-20 11:18 PST by Wenson Hsieh
Modified: 2022-01-20 15:10 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.15 KB, patch)
2022-01-20 11:31 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2022-01-20 11:18:00 PST
rdar://87423185
Comment 1 Wenson Hsieh 2022-01-20 11:31:05 PST
Created attachment 449598 [details]
Patch
Comment 2 Darin Adler 2022-01-20 11:43:12 PST
Comment on attachment 449598 [details]
Patch

I think it’s likely we have other places where we are checking against U_ZERO_ERROR where we really mean U_SUCCESS.
Comment 3 Wenson Hsieh 2022-01-20 11:58:35 PST
Thanks for the review!

(In reply to Darin Adler from comment #2)
> Comment on attachment 449598 [details]
> Patch
> 
> I think it’s likely we have other places where we are checking against
> U_ZERO_ERROR where we really mean U_SUCCESS.

Yes — it's certainly possible. I locally only ran a subset of layout tests that use text iterators (`accessibility`, `fast`, and `editing`), and didn't see any other ICU-related crashes/assertions, but did not run the full suite.

I think I'll land this change sooner to stop Mac layout tests from exiting early, and then see if there are any more places where we need similar treatment w.r.t. status codes from ICU.
Comment 4 Darin Adler 2022-01-20 13:01:08 PST
(In reply to Wenson Hsieh from comment #3)
> I think I'll land this change sooner to stop Mac layout tests from exiting
> early, and then see if there are any more places where we need similar
> treatment w.r.t. status codes from ICU.

Yes, I was talking about future proofing. Completely agree about landing just this. Also, since this change is inside an assertion only, we can rest easy that it only affects debug builds.
Comment 5 EWS 2022-01-20 14:09:36 PST
Found 1 new test failure: imported/w3c/web-platform-tests/html/canvas/element/manual/imagebitmap/createImageBitmap-flipY.html
Comment 6 EWS 2022-01-20 15:10:26 PST
Committed r288327 (246239@main): <https://commits.webkit.org/246239@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449598 [details].