Bug 184481

Summary: Non-ASCII characters are not correctly handled in data channel.
Product: WebKit Reporter: jianjun.zhu
Component: WebRTCAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews105 for mac-sierra-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch none

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.