Bug 216229 - Make TextCodecCJK and TextCodecSingleByte thread-safe and refactor a bit to share code
Summary: Make TextCodecCJK and TextCodecSingleByte thread-safe and refactor a bit to s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-06 13:23 PDT by Darin Adler
Modified: 2020-09-07 10:44 PDT (History)
7 users (show)

See Also:

Attachments
Patch (30.02 KB, patch)
2020-09-06 14:02 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (30.17 KB, patch)
2020-09-06 14:14 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (30.20 KB, patch)
2020-09-06 14:18 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (30.20 KB, patch)
2020-09-06 14:31 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (39.00 KB, patch)
2020-09-06 17:56 PDT, Darin Adler 		sam: review+
Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-09-06 13:23:46 PDT 
Make TextCodecCJK and TextCodecSingleByte thread-safe and refactor a bit to share code
Comment 1 Darin Adler 2020-09-06 14:02:01 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-09-06 14:14:21 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2020-09-06 14:18:44 PDT Comment hidden (obsolete)
Comment 4 Darin Adler 2020-09-06 14:31:38 PDT Comment hidden (obsolete)
Comment 5 Darin Adler 2020-09-06 17:56:49 PDT 
Created attachment 408156 [details]
Patch
Comment 6 EWS Watchlist 2020-09-06 17:57:34 PDT 
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 7 Sam Weinig 2020-09-07 09:49:18 PDT 
Comment on attachment 408156 [details]
Patch

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

> Source/WebCore/platform/text/EncodingTables.h:45
> +// FIXME: Consider moving these somewhere else since they are likely useful for by more than just encoding tables.

Seem like a good fit for wtf/StdLibExtras.h in the future.

> Source/WebCore/platform/text/TextCodecCJK.cpp:132
> +    static JIS0208DecodeIndex* table;
> +    static std::once_flag once;
> +    std::call_once(once, [&] {
> +        table = new JIS0208DecodeIndex;

Not new, but might be useful to have a comment here about why we are choosing to do this as an allocation / lazy sort rather than a compile time content table, which I presume is due to jis0208 already being huge, and not wanting to bloat the WebCore binary even more. Same with eucKREncodingIndex/Big5 below.
Comment 8 Darin Adler 2020-09-07 10:11:49 PDT 
Committed r266703: <https://trac.webkit.org/changeset/266703>
Comment 9 Radar WebKit Bug Importer 2020-09-07 10:12:16 PDT 
<rdar://problem/68468669>
Comment 10 Alex Christensen 2020-09-07 10:26:57 PDT 
Comment on attachment 408156 [details]
Patch

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

> LayoutTests/platform/mac-wk2/TestExpectations:-1112
>  # <rdar://problem/37330377> ASan tests exiting early due to timeouts
> -[ Release ] imported/w3c/web-platform-tests/encoding [ Skip ]

We probably still want to skip these tests in ASan builds, or at least the tests that just test that each code point is correct.  They take too long.
Comment 11 Darin Adler 2020-09-07 10:35:58 PDT 
Comment on attachment 408156 [details]
Patch

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

>> LayoutTests/platform/mac-wk2/TestExpectations:-1112
>> -[ Release ] imported/w3c/web-platform-tests/encoding [ Skip ]
> 
> We probably still want to skip these tests in ASan builds, or at least the tests that just test that each code point is correct.  They take too long.

Oops, yes, I checked this in accidentally.

The reason I un-skipped is that a lot of the encoding tests are skipped in all Debug builds due to an entry in TestExpectations, also due to the fact that they are too slow. Thus, also skipping them in Release builds means never running them at all. Luckily this is (for some reason) specifically Mac Modern WebKit only? If the intent is to skip specifically in ASan builds, we need to find a way to do that. The bug cited just above this was closed in 2018, so I think it’s in a strange state.

What do you think we should do?
Comment 12 Alex Christensen 2020-09-07 10:37:56 PDT 
(In reply to Darin Adler from comment #11)
> What do you think we should do?
I think we should revert this one line.  WK1 release runs all these tests.
Comment 13 Darin Adler 2020-09-07 10:44:29 PDT 
(In reply to Alex Christensen from comment #12)
> I think we should revert this one line.  WK1 release runs all these tests.

OK, done in r266704.