Implement "replacement" codec according to spec: https://encoding.spec.whatwg.org/#replacement
<rdar://problem/26015178>
Created attachment 282190 [details] Patch
Created attachment 282191 [details] Patch
Comment on attachment 282191 [details] Patch Attachment 282191 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1583047 New failing tests: imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html
Created attachment 282197 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 282191 [details] Patch Attachment 282191 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1583104 New failing tests: imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html
Created attachment 282198 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 282191 [details] Patch Attachment 282191 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1583103 New failing tests: imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization.html
Created attachment 282201 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 282205 [details] Patch
Created attachment 282255 [details] Patch
Comment on attachment 282205 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282205&action=review I think this looks great, but I'm concerned about your string handling routine. Please correct that, and I think this will be ready to go. r- to fix the build and to fix the string handling. > Source/WebCore/ChangeLog:24 > + This change refers some of the Blink changes: This change is based on the following Blink changes: > Source/WebCore/ChangeLog:28 > + * WebCore.xcodeproj/project.pbxproj: You also need to add your new TextCodecReplacement.cpp file to CMakeLists.txt > Source/WebCore/platform/text/TextEncoding.cpp:52 > + m_name = 0; m_name = nullptr; > Source/WebCore/platform/text/TextEncoding.cpp:61 > + m_name = 0; m_name = nullptr; > Source/WebCore/platform/text/TextEncodingRegistry.cpp:273 > + return alias && !strcasecmp(alias, "replacement"); strcasecmp is pretty unsafe, since it will always attempt to read 11 bytes of alias, unless there is a proper null termination in the memory address. You do check for null, but what if alias was allocated as 2 bytes with no null terminator? We'd buffer overrun. At minimum, it seems like you need to confirm that 'alias' is 11 characters, and return false if it doesn't. if (!alias) return false; if (strlen(alias) != 11) return false; return !strcasecmp(alias, "replacement"); > Source/WebCore/platform/text/TextEncodingRegistry.cpp:278 > + return alias == "replacement"; Here you do a case-sensitive comparison. Is that correct? If 'strcasecmp' was correct above, then this should be something like 'equalLettersIgnoringASCIICase'. > LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Document-characterSet-normalization-expected.txt:-655 > -FAIL Name "replacement" has label "iso-2022-kr" (charset) assert_equals: expected "replacement" but got "ISO-2022-KR" Oh, interesting! So these had not been handled properly by us ever! Nice fix.
Comment on attachment 282255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282255&action=review I think this looks great, but I'm concerned about your string handling routine. Please correct that, and I think this will be ready to go. r- to fix the string handling. > Source/WebCore/ChangeLog:24 > + This change refers some of the Blink changes: This change is based on the following Blink changes: > Source/WebCore/platform/text/TextEncoding.cpp:52 > + m_name = 0; m_name = nullptr; > Source/WebCore/platform/text/TextEncoding.cpp:61 > + m_name = 0; Ditto. > Source/WebCore/platform/text/TextEncodingRegistry.cpp:273 > + return alias && !strcasecmp(alias, "replacement"); strcasecmp is pretty unsafe, since it will always attempt to read 11 bytes of alias, unless there is a proper null termination in the memory address. You do check for null, but what if alias was allocated as 2 bytes with no null terminator? We'd buffer overrun. At minimum, it seems like you need to confirm that 'alias' is 11 characters, and return false if it doesn't. if (!alias) return false; if (strlen(alias) != 11) return false; return !strcasecmp(alias, "replacement"); > Source/WebCore/platform/text/TextEncodingRegistry.cpp:278 > + return alias == "replacement"; Here you do a case-sensitive comparison. Is that correct? If 'strcasecmp' was correct above, then this should be something like 'equalLettersIgnoringASCIICase'.
Comment on attachment 282255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282255&action=review >> Source/WebCore/ChangeLog:24 >> + This change refers some of the Blink changes: > > This change is based on the following Blink changes: Fixed. >> Source/WebCore/platform/text/TextEncoding.cpp:52 >> + m_name = 0; > > m_name = nullptr; Fixed. >> Source/WebCore/platform/text/TextEncoding.cpp:61 >> + m_name = 0; > > Ditto. Fixed. >> Source/WebCore/platform/text/TextEncodingRegistry.cpp:273 >> + return alias && !strcasecmp(alias, "replacement"); > > strcasecmp is pretty unsafe, since it will always attempt to read 11 bytes of alias, unless there is a proper null termination in the memory address. > > You do check for null, but what if alias was allocated as 2 bytes with no null terminator? We'd buffer overrun. At minimum, it seems like you need to confirm that 'alias' is 11 characters, and return false if it doesn't. > > if (!alias) > return false; > > if (strlen(alias) != 11) > return false; > > return !strcasecmp(alias, "replacement"); Fixed. >> Source/WebCore/platform/text/TextEncodingRegistry.cpp:278 >> + return alias == "replacement"; > > Here you do a case-sensitive comparison. Is that correct? If 'strcasecmp' was correct above, then this should be something like 'equalLettersIgnoringASCIICase'. This is definitely not correct. Fixed.
Created attachment 282291 [details] Patch
Created attachment 282293 [details] Patch
Comment on attachment 282293 [details] Patch R=me
Comment on attachment 282293 [details] Patch Clearing flags on attachment: 282293 Committed r202599: <http://trac.webkit.org/changeset/202599>
All reviewed patches have been landed. Closing bug.
Committed r202635: <http://trac.webkit.org/changeset/202635> Committed r202636: <http://trac.webkit.org/changeset/202636> Committed r202643: <http://trac.webkit.org/changeset/202643> Attempts to fix the ASAN build.