Bug 184481 - Non-ASCII characters are not correctly handled in data channel.
Summary: Non-ASCII characters are not correctly handled in data channel.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-10 18:11 PDT by jianjun.zhu
Modified: 2018-04-11 08:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.55 KB, patch)
2018-04-10 18:16 PDT, jianjun.zhu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-sierra-wk2 (3.00 MB, application/zip)
2018-04-10 19:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.35 MB, application/zip)
2018-04-10 19:49 PDT, Build Bot
no flags Details
Patch (4.74 KB, patch)
2018-04-10 22:39 PDT, jianjun.zhu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jianjun.zhu 2018-04-10 18:11:00 PDT
Non-ASCII characters are not correctly handled in data channel.
Comment 1 jianjun.zhu 2018-04-10 18:16:25 PDT
Created attachment 337665 [details]
Patch
Comment 2 Build Bot 2018-04-10 18:17:42 PDT
Attachment 337665 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 jianjun.zhu 2018-04-10 18:25:38 PDT
This issue can be reproduced by sending emoji or Chinese characters between Safari and Chrome or between two PeerConnections in Safari.

A similar issue can be found in Apple forum: https://discussions.apple.com/thread/8100630
Comment 4 youenn fablet 2018-04-10 18:59:04 PDT
Thanks Jianjun, would you be able to provide a test case in this patch.
Comment 5 youenn fablet 2018-04-10 19:07:22 PDT
Comment on attachment 337665 [details]
Patch

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

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:51
> +    return m_channel->Send({ rtc::CopyOnWriteBuffer(utf8Text.data(), utf8Text.length()), false });

Utf8Text is not really needed, can we remove it?

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:100
>          // FIXME: Ensure this is correct by adding some tests with non-ASCII characters.

We should remove the fixme
Comment 6 Build Bot 2018-04-10 19:27:28 PDT
Comment on attachment 337665 [details]
Patch

Attachment 337665 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7276821

New failing tests:
imported/w3c/web-platform-tests/webrtc/RTCDataChannel-send.html
Comment 7 Build Bot 2018-04-10 19:27:30 PDT
Created attachment 337668 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 8 Build Bot 2018-04-10 19:48:59 PDT
Comment on attachment 337665 [details]
Patch

Attachment 337665 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7276836

New failing tests:
imported/w3c/web-platform-tests/webrtc/RTCDataChannel-send.html
Comment 9 Build Bot 2018-04-10 19:49:00 PDT
Created attachment 337669 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 10 youenn fablet 2018-04-10 20:00:17 PDT
Great, no need for new tests apparently. You might just need to update the corresponding -expected.txt file
Comment 11 jianjun.zhu 2018-04-10 20:01:59 PDT
Comment on attachment 337665 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:51
>> +    return m_channel->Send({ rtc::CopyOnWriteBuffer(utf8Text.data(), utf8Text.length()), false });
> 
> Utf8Text is not really needed, can we remove it?

We need to access text.utf8() twice if utf8Text is removed. I'm afraid converting character encoding one more time may cause extra overhead.

>> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp:100
>>          // FIXME: Ensure this is correct by adding some tests with non-ASCII characters.
> 
> We should remove the fixme

I'll remove it after test cases were added for non-ASCII characters.
Comment 12 youenn fablet 2018-04-10 20:39:29 PDT
> We need to access text.utf8() twice if utf8Text is removed. I'm afraid
> converting character encoding one more time may cause extra overhead.

Oh right!
You can use auto instead of CString.
Comment 13 jianjun.zhu 2018-04-10 22:39:19 PDT
Created attachment 337679 [details]
Patch
Comment 14 youenn fablet 2018-04-11 08:02:06 PDT
Comment on attachment 337679 [details]
Patch

Thanks!
Comment 15 WebKit Commit Bot 2018-04-11 08:28:43 PDT
Comment on attachment 337679 [details]
Patch

Clearing flags on attachment: 337679

Committed r230524: <https://trac.webkit.org/changeset/230524>
Comment 16 WebKit Commit Bot 2018-04-11 08:28:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2018-04-11 08:29:26 PDT
<rdar://problem/39348158>
Comment 18 jianjun.zhu 2018-04-11 08:47:11 PDT
Thanks for reviewing this patch.