TextDecoder('utf-8') doesn’t match spec #16894

Activity

1. srl295

   added

   i18n-apiIssues and PRs related to Node.js internationalization support.Issues and PRs related to Node.js internationalization support.

   on Nov 9, 2017
2. srl295 commented on Nov 9, 2017

   

   srl295

   on Nov 9, 2017

   MemberAuthor

   More actions

   in v8: https://bugs.chromium.org/p/v8/issues/detail?id=7055
3. jungshik commented on Nov 9, 2017

   

   jungshik

   on Nov 9, 2017

   More actions

   The code snippet above in web browsers(implementing TextDecoder API) should read as following:

   new TextDecoder('utf-8')
    .decode(new Uint8Array([0xF0, 0x80, 0x80])).length
   
4. srl295 commented on Nov 9, 2017

   

   srl295

   on Nov 9, 2017

   MemberAuthor

   More actions

   @jungshik oops… exactly right. I fixed it.
5. srl295 commented on Nov 9, 2017

   

   srl295

   on Nov 9, 2017

   Last edited by srl295

   MemberAuthor

   More actions

   ^ already fixed in v8 (see link above)

   Yea I recently fixed this in V8. The fix is in V8 6.3.
   The relevant CL is https://chromium-review.googlesource.com/671020
   See also https://bugs.chromium.org/p/chromium/issues/detail?id=765608
6. srl295

   added

   v8 engineIssues and PRs related to the V8 dependency.Issues and PRs related to the V8 dependency.

   on Nov 9, 2017
7. tniessen commented on Nov 10, 2017

   

   tniessen

   on Nov 10, 2017

   Member

   More actions

   The relevant commit is v8/v8@6389b7e. Should we cherry-pick this for node 8?

   👍React with 👍1Reacted by Steven R. Loomis
8. srl295 commented on Nov 10, 2017

   

   srl295

   on Nov 10, 2017

   MemberAuthor

   More actions

   @tniessen seems like a good idea—means the other UTF-8 handling (v8 internal, and via ICU) will be in line with TextDecoder
9. TimothyGu commented on Nov 10, 2017

   

   TimothyGu

   on Nov 10, 2017

   Last edited by TimothyGu

   Member

   More actions

   This issue really has nothing to do with V8, since TextDecoder is backed by ICU. On master, this has been fixed by #16876, but not in v9.x or earlier. It seems the original issue (TextDecoder) only affects released versions of Node.js. We'll also have to make the same fix for require('string_decoder').StringDecoder, if we were to go all in.
10. TimothyGu

    added

    c++Issues and PRs that require attention from people who are familiar with C++.Issues and PRs that require attention from people who are familiar with C++.

    and removed

    v8 engineIssues and PRs related to the V8 dependency.Issues and PRs related to the V8 dependency.

    on Nov 10, 2017
11. TimothyGu commented on Nov 10, 2017

    

    TimothyGu

    on Nov 10, 2017

    Last edited by TimothyGu

    Member

    More actions

    I guess we just need to backport

    • http://bugs.icu-project.org/trac/changeset/40445/trunk/icu4c/ and
    • http://bugs.icu-project.org/trac/changeset/40455/trunk/icu4c/

    to v8.x, where the TextDecoder API is actually stable; or we can just upgrade ICU to 60 in v8.x LTS. We'll also need tests, as it seems WPT does not currently contain any tests for this behavior. /cc @jasnell

    😕React with 😕1Reacted by Steven R. Loomis
12. srl295 commented on Nov 11, 2017

    

    srl295

    on Nov 11, 2017

    MemberAuthor

    More actions

    No please do not backport those commits. V8 and not icu is used for utf -8. Check the code. @jasnell
13. TimothyGu commented on Nov 11, 2017

    

    TimothyGu

    on Nov 11, 2017

    Member

    More actions

    @srl295 I have just backported the commits in my local branch, and this issue was fixed. Can you explain how V8 is used for UTF-8? If you mean

    node/lib/internal/encoding.js

    Line 299 in e624503

    return encodeUtf8String(`${input}`);

    , that is for TextEncoder, not TextDecoder.
14. srl295 commented on Nov 11, 2017

    

    srl295

    on Nov 11, 2017 via email

    MemberAuthor

    More actions

    Maybe we are looking at different things. The issue I saw is there even with no icu El El vie, nov. 10, 2017 a las 6:49 p. m., Timothy Gu < notifications@github.com> escribió:

    …

    @srl295 <https://github.com/srl295> I have just backported the commits in my local branch, and this issue was fixed. Can you explain how V8 is used for UTF-8? If you mean https://github.com/nodejs/node/blob/e6245030b193db7de9daad1e455f1fcb7ce4daac/lib/internal/encoding.js#L299, that is for Text*En*coder, not Text*De*coder. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16894 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0Msz5nNG1k08_rsokgYJWXCrFDPTWxks5s1QtVgaJpZM4QXRQa> .
15. TimothyGu commented on Nov 11, 2017

    

    TimothyGu

    on Nov 11, 2017

    Member

    More actions

    @srl295

    The issue I saw is there even with no icu

    no-icu uses string_decoder.StringDecoder, which as I noted in #16894 (comment) handles this wrongly as well.
16. srl295 commented on Nov 16, 2017

    

    srl295

    on Nov 16, 2017 via email

    MemberAuthor

    More actions

    @TimothyGu As I said please proceed how you think best if you want to take this issue.
17. jasnell commented on Aug 11, 2018

    

    jasnell

    on Aug 11, 2018

    Member

    More actions

    appears to be fixed in 10.x and master
18. jasnell

    closed this as completedon Aug 11, 2018