MediaStream API: Add toString to RTCIceCandidate and RTCSessionDescription

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Tommy Widenflycht]

Also fix a few review comments from an earlier patch.

[Tommy Widenflycht]

Created attachment 160139 [details]
Patch

[Adam Barth]

Comment on attachment 160139 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:93
> +    String result = "({candidate:\"" + candidate() + "\",sdpMid:\"" + sdpMid() + "\",sdpMLineIndex:" + String::number(sdpMLineIndex()) + "})";

Is this meant to be JSON?  (This isn't valid JSON.)  Also sdpMid() can be an arbitrary string, right?  That means it might include a " and break the format of the string.

Perhaps we should use InspectorValue to create valid JSON?

[Tommy Widenflycht]

Comment on attachment 160139 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/RTCIceCandidate.cpp:93
>> +    String result = "({candidate:\"" + candidate() + "\",sdpMid:\"" + sdpMid() + "\",sdpMLineIndex:" + String::number(sdpMLineIndex()) + "})";
> 
> Is this meant to be JSON?  (This isn't valid JSON.)  Also sdpMid() can be an arbitrary string, right?  That means it might include a " and break the format of the string.
> 
> Perhaps we should use InspectorValue to create valid JSON?

No
Yes I know
Yes

For some reason this is exactly what the standardization committee wants.

[Eric Rescorla]

Tommy, I suspect this is simply an error on the part of the specification writers, who for some reason seem to think it's a great idea to specify this stuff in terms of U+ code points rather than in terms of "JSON". Note that they also require "U+007D RIGTH CURLY BRACKET"

It's probably worth raising this on the mailing list.

[Tommy Widenflycht]

(In reply to comment #4)
> Tommy, I suspect this is simply an error on the part of the specification writers, who for some reason seem to think it's a great idea to specify this stuff in terms of U+ code points rather than in terms of "JSON". Note that they also require "U+007D RIGTH CURLY BRACKET"
> 
> It's probably worth raising this on the mailing list.

As far as I know this is not supposed to be JSON but I will raise this again.
And btw the code adds the right curly bracket.

[Eric Rescorla]

Yes, my point was that you don't add a "rihgt curly bracket" which suggests that perhaps the spec may not be being as carefully edited as one might like.

At a higher level, I don't recall the WG deciding explicitly that the inner string was not supposed to be JSON, and if it did, IMO that decision was wrong.

[Tommy Widenflycht]

(In reply to comment #6)
> Yes, my point was that you don't add a "rihgt curly bracket" which suggests that perhaps the spec may not be being as carefully edited as one might like.

Ahh :) 

> At a higher level, I don't recall the WG deciding explicitly that the inner string was not supposed to be JSON, and if it did, IMO that decision was wrong.

I am against producing JSON (since the objects should be transparent to how they are sent/received) but happily!? I am not part of the standardization committee. I have sent an email to the list about this.

[Adam Barth]

It seems really silly to have something that is almost-but-not-quite JSON in a way that will XSS you if you try to parse it with eval.

[Tommy Widenflycht]

Closing this bug since it is likely the stringifier will be removed from the spec, at least for now.