WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
179416
Remove support for iOS-only softbank-sjis encoding if possible
https://bugs.webkit.org/show_bug.cgi?id=179416
Summary
Remove support for iOS-only softbank-sjis encoding if possible
Maciej Stachowiak
Reported
2017-11-08 00:02:52 PST
Remove support for iOS-only softbank-sjis encoding if possible. I'm not entirely sure how to check. There are apparently also docomo-sjis and kddi-sjis, but we don't support those in WebKit. See
bug #19309
for more info.
Attachments
Patch
(3.63 KB, patch)
2017-11-08 17:03 PST
,
Maciej Stachowiak
no flags
Details
Formatted Diff
Diff
Patch, doesn't yet address request to avoid depending on parsing details
(5.69 KB, patch)
2017-11-08 18:33 PST
,
Maciej Stachowiak
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews103 for mac-elcapitan
(2.19 MB, application/zip)
2017-11-08 19:28 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
(3.30 MB, application/zip)
2017-11-08 19:32 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews126 for ios-simulator-wk2
(2.40 MB, application/zip)
2017-11-08 20:01 PST
,
Build Bot
no flags
Details
Archive of layout-test-results from ews117 for mac-elcapitan
(3.22 MB, application/zip)
2017-11-08 20:07 PST
,
Build Bot
no flags
Details
Patch with test that works, and done the way Alexey suggested
(6.64 KB, patch)
2017-11-08 23:22 PST
,
Maciej Stachowiak
no flags
Details
Formatted Diff
Diff
Patch with further tweaks to the regression test
(6.36 KB, patch)
2017-11-09 00:05 PST
,
Maciej Stachowiak
no flags
Details
Formatted Diff
Diff
Patch for landing
(6.36 KB, patch)
2017-11-09 10:05 PST
,
Maciej Stachowiak
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Maciej Stachowiak
Comment 1
2017-11-08 00:03:51 PST
I meant
bug #179309
Maciej Stachowiak
Comment 2
2017-11-08 00:44:28 PST
By my reading of the code, it seems like the softbank-sjis text codec can't possibly actually work, which might be evidence that it's actually not needed. The code looks like this: #if PLATFORM(IOS) // See comment above in registerEncodingNames(). int32_t i = 0; for (const char* alias = textCodecMacAliases[i]; alias; alias = textCodecMacAliases[++i]) registrar(alias, create, 0); #endif But that will result in a TextCodecICU with a null m_canonicalConverterName, which will therefore always fail to create the ICU converter, because it will pass null to ucnv_open.
Darin Adler
Comment 3
2017-11-08 09:09:08 PST
(In reply to Maciej Stachowiak from
comment #2
)
> But that will result in a TextCodecICU with a null m_canonicalConverterName, > which will therefore always fail to create the ICU converter, because it > will pass null to ucnv_open.
I think you are right that it doesn’t work. From my reading of the code, it won’t fail, but also won’t do anything useful. From the ucnv_open documentation: "If this parameter is NULL, the default converter will be used." So it will create ICU's "default converter", whatever that is; reading the documentation doesn’t really make it all that clear to me what it is, although it can be changed with ucnv_setDefaultName. And also, it seems that TextCodecICU::createICUConverter will crash with a null dereference calling strcmp, if this is tried again after the converter is cached. So, in summary, should almost certainly remove, highly like it does not do anything useful!
Alexey Proskuryakov
Comment 4
2017-11-08 09:33:13 PST
Indeed, <meta charset="softbank-sjis"> doesn't work! May be good to also check if it cannot be specified using WebKit API or SPI by a client.
Darin Adler
Comment 5
2017-11-08 09:38:50 PST
I can’t see any way it would work given the way the code is written.
Maciej Stachowiak
Comment 6
2017-11-08 11:24:48 PST
(In reply to Alexey Proskuryakov from
comment #4
)
> Indeed, <meta charset="softbank-sjis"> doesn't work! May be good to also > check if it cannot be specified using WebKit API or SPI by a client.
Do you know how I could make a test case for this? I'm not sure what the softbank-sjis encoding is supposed to look like.
Maciej Stachowiak
Comment 7
2017-11-08 17:03:05 PST
Created
attachment 326408
[details]
Patch
Alexey Proskuryakov
Comment 8
2017-11-08 17:51:22 PST
Comment on
attachment 326408
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=326408&action=review
This patch only has LayoutTests changes, nothing in WebCore.
> LayoutTests/fast/encoding/charset-softbank-sjis.html:5 > + <meta charset=utf-8>
Pretty sure that default encoding is well defined in DRT/WKTR, so I'd omit this line. I feel like we are unnecessarily relying on parser details here. The small downside is that the test will fail locally. One way to correct for that is to check that document.charset is not softbank-sjis (or doesn't include "jis", for some resilience). Or you could print the actual charset and have text explaining that the test passes as long as it's not softbank-sjis.
> LayoutTests/fast/encoding/charset-softbank-sjis.html:9 > + if (window.testRunner) > + testRunner.dumpAsText();
js-test-pre.js enables dumpAsText. I recommend using js-test.js in new tests instead, as is saves one line from boilerplate.
Maciej Stachowiak
Comment 9
2017-11-08 18:29:16 PST
(In reply to Alexey Proskuryakov from
comment #8
)
> Comment on
attachment 326408
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=326408&action=review
> > This patch only has LayoutTests changes, nothing in WebCore.
Oops, I'll upload a new one with the actual code change.
> > > LayoutTests/fast/encoding/charset-softbank-sjis.html:5 > > + <meta charset=utf-8> > > Pretty sure that default encoding is well defined in DRT/WKTR, so I'd omit > this line. I feel like we are unnecessarily relying on parser details here. > > The small downside is that the test will fail locally. One way to correct > for that is to check that document.charset is not softbank-sjis (or doesn't > include "jis", for some resilience). Or you could print the actual charset > and have text explaining that the test passes as long as it's not > softbank-sjis.
This was the only way I could figure out to get the test to behave consistently between DRT/WKTR and when opened in Safari. DTR/WKTR defaults to windows-1252, while Safari defaults to utf-8. I could make a test for encoding name that doesn't depend on the exact encoding chosen as you suggest. But I am not sure how I could do the specific character test, where I check that a UTF-8 e with acute accent is decoded as such. Can you think of a way to do that that's less weird than depending on HTML rules for parsing <meta charset>?
> > > LayoutTests/fast/encoding/charset-softbank-sjis.html:9 > > + if (window.testRunner) > > + testRunner.dumpAsText(); > > js-test-pre.js enables dumpAsText. > > I recommend using js-test.js in new tests instead, as is saves one line from > boilerplate.
Will do.
Maciej Stachowiak
Comment 10
2017-11-08 18:33:15 PST
Created
attachment 326415
[details]
Patch, doesn't yet address request to avoid depending on parsing details
Alexey Proskuryakov
Comment 11
2017-11-08 18:53:13 PST
> But I am not sure how I could do the specific character test, where I check that a UTF-8 e with acute accent is decoded as such.
What is the reason why you want to decode a character? I think that checking document.charset is a good test for this fix.
Maciej Stachowiak
Comment 12
2017-11-08 19:05:45 PST
(In reply to Alexey Proskuryakov from
comment #11
)
> > But I am not sure how I could do the specific character test, where I check that a UTF-8 e with acute accent is decoded as such. > > What is the reason why you want to decode a character? I think that checking > document.charset is a good test for this fix.
Some of our other encoding tests do that, and it seems worthwhile to check that the document decodes as expected in addition to checking the exposed encoding name.
Build Bot
Comment 13
2017-11-08 19:28:56 PST
Comment on
attachment 326415
[details]
Patch, doesn't yet address request to avoid depending on parsing details
Attachment 326415
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/5157050
New failing tests: fast/encoding/charset-softbank-sjis.html
Build Bot
Comment 14
2017-11-08 19:28:57 PST
Created
attachment 326416
[details]
Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Build Bot
Comment 15
2017-11-08 19:32:42 PST
Comment on
attachment 326415
[details]
Patch, doesn't yet address request to avoid depending on parsing details
Attachment 326415
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/5157002
New failing tests: fast/encoding/charset-softbank-sjis.html
Build Bot
Comment 16
2017-11-08 19:32:44 PST
Created
attachment 326417
[details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Alexey Proskuryakov
Comment 17
2017-11-08 19:48:31 PST
I personally think that it's enough to check the exposed name, but if you want to test end to end, the workaround is to test in a subframe - the encoding of the top frame is inherited as default for the child frame, as long as they are same origin.
Build Bot
Comment 18
2017-11-08 20:01:36 PST
Comment on
attachment 326415
[details]
Patch, doesn't yet address request to avoid depending on parsing details
Attachment 326415
[details]
did not pass ios-sim-ews (ios-simulator-wk2): Output:
http://webkit-queues.webkit.org/results/5157104
New failing tests: fast/encoding/charset-softbank-sjis.html
Build Bot
Comment 19
2017-11-08 20:01:37 PST
Created
attachment 326421
[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.12.6
Build Bot
Comment 20
2017-11-08 20:07:17 PST
Comment on
attachment 326415
[details]
Patch, doesn't yet address request to avoid depending on parsing details
Attachment 326415
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/5157100
New failing tests: fast/encoding/charset-softbank-sjis.html
Build Bot
Comment 21
2017-11-08 20:07:19 PST
Created
attachment 326423
[details]
Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Maciej Stachowiak
Comment 22
2017-11-08 23:02:35 PST
I accidentally took out the test for encoding name. I'll upload a new patch with that fixed, plus Alexey's suggestion to use an iframe for consistent encoding results.
Maciej Stachowiak
Comment 23
2017-11-08 23:22:50 PST
Created
attachment 326431
[details]
Patch with test that works, and done the way Alexey suggested
Alexey Proskuryakov
Comment 24
2017-11-08 23:51:16 PST
Comment on
attachment 326431
[details]
Patch with test that works, and done the way Alexey suggested View in context:
https://bugs.webkit.org/attachment.cgi?id=326431&action=review
> LayoutTests/fast/encoding/charset-softbank-sjis-expected.txt:5 > +TEST COMPLETE > +Test PASSED if this renders as an accented e: âéâ.
This is not right.
> LayoutTests/fast/encoding/resources/softbank-sjis-iframe.html:5 > +<script src="../../../resources/js-test.js"></script>
I think that this should be in the main frame. Otherwise, it will call notifyDone once the subframe is done loading, which may be before the main frame is done.
> LayoutTests/fast/encoding/resources/softbank-sjis-iframe.html:11 > +Test PASSED if this renders as an accented e: âéâ.
I think that it would work better if this were at the top of <body> (and I'd make it an explicit element). But also, why not put it into a script as a shouldBe? I think that this should do it: shouldBe("'\xe9'", "'é'");
Maciej Stachowiak
Comment 25
2017-11-09 00:05:19 PST
Created
attachment 326433
[details]
Patch with further tweaks to the regression test
Alexey Proskuryakov
Comment 26
2017-11-09 09:40:10 PST
Comment on
attachment 326433
[details]
Patch with further tweaks to the regression test View in context:
https://bugs.webkit.org/attachment.cgi?id=326433&action=review
Great!
> Source/WebCore/ChangeLog:9 > + (WebCore::TextCodecICU::registerEncodingNames): Remove support for softbanj-sjis
"softbank"
Maciej Stachowiak
Comment 27
2017-11-09 10:05:28 PST
Created
attachment 326461
[details]
Patch for landing
WebKit Commit Bot
Comment 28
2017-11-09 10:37:13 PST
Comment on
attachment 326461
[details]
Patch for landing Clearing flags on attachment: 326461 Committed
r224636
: <
https://trac.webkit.org/changeset/224636
>
WebKit Commit Bot
Comment 29
2017-11-09 10:37:15 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 30
2017-11-15 09:39:56 PST
<
rdar://problem/35562183
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug