WebRTC: RTCIceCandidate init dictionary don't handle explicit null or undefined values correctly

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Bergkvist]

For example

new RTCIceCandidate({ candidate: "foo", sdpMid: null })

succeeds and sdpMid becomes the string "null".

[Adam Bergkvist]

Created attachment 281562 [details]
Proposed patch

[Eric Carlson]

Comment on attachment 281562 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:8
> +        Prevent explicit null and undefined values to be converted to "null" and "undefined" strings.

Nit: "values to be converted to" -> "values from being converted to"

[Alejandro G. Castro]

Comment on attachment 281562 [details]
Proposed patch

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

> Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:59
> +    if (dictionary.getWithUndefinedOrNullCheck("sdpMLineIndex", sdpMLineIndexString)) {
> +        if (!sdpMLineIndexString.isNull()) {

I think you can join the 2 ifs in just one with an and condition. Also check if getWithUndefinedOrNullCheck returns false if undefined or null, maybe we do not need the second condition.

[Adam Bergkvist]

Created attachment 281642 [details]
Updated patch

[Adam Bergkvist]

(In reply to comment #2)
> Comment on attachment 281562 [details]
> Proposed patch

Thanks for the reviews
 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281562&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Prevent explicit null and undefined values to be converted to "null" and "undefined" strings.
> 
> Nit: "values to be converted to" -> "values from being converted to"

Fixed.

(In reply to comment #3)
> Comment on attachment 281562 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281562&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:59
> > +    if (dictionary.getWithUndefinedOrNullCheck("sdpMLineIndex", sdpMLineIndexString)) {
> > +        if (!sdpMLineIndexString.isNull()) {
> 
> I think you can join the 2 ifs in just one with an and condition. Also check
> if getWithUndefinedOrNullCheck returns false if undefined or null, maybe we
> do not need the second condition.

Dug a bit deeper and getWithUndefinedOrNullCheck() returns false if the dictionary member was undefined or false; rendering the second check is unnecessary. Fixed.

[Alejandro G. Castro]

Comment on attachment 281642 [details]
Updated patch

LGTM, required to handle the null values correctly for this nullable attributes.

[Adam Bergkvist]

Comment on attachment 281642 [details]
Updated patch

Thanks Alex and Eric!

[WebKit Commit Bot]

Comment on attachment 281642 [details]
Updated patch

Rejecting attachment 281642 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 281642, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
LayoutTests
:040000 040000 53d9c5daad840e7c8d4b08e899f964d25323f0c8 f3f1d4fa1265a983a518943e7e3a0f794477e6bd M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/1536289

[Adam Bergkvist]

Comment on attachment 281642 [details]
Updated patch

Let's try cq again (now that it's fixed)

[WebKit Commit Bot]

Comment on attachment 281642 [details]
Updated patch

Clearing flags on attachment: 281642

Committed r202267: <http://trac.webkit.org/changeset/202267>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.