Non-ASCII characters are not correctly handled in data channel.

• Component: WebRTC
• Status: RESOLVED FIXED

Non-ASCII characters are not correctly handled in data channel.

Created attachment 337665 [details]
Patch

[EWS Watchlist]

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.

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

[youenn fablet]

Thanks Jianjun, would you be able to provide a test case in this patch.

[youenn fablet]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

Great, no need for new tests apparently. You might just need to update the corresponding -expected.txt file

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.

[youenn fablet]

> 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.

Created attachment 337679 [details]
Patch

[youenn fablet]

Comment on attachment 337679 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 337679 [details]
Patch

Clearing flags on attachment: 337679

Committed r230524: <https://trac.webkit.org/changeset/230524>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39348158>

Thanks for reviewing this patch.