1432936 - Rust SDP Parser fails to produce an error on NewSdpTest.ParseInvalidSimulcastNotReceiving

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P3)

Description

[Dan Minor [:dminor]]

[ RUN      ] RoundTripSerialize/NewSdpTest.ParseInvalidSimulcastNotReceiving/0
version: 0
origin: -, 4294967296, 2, 127.0.0.1
session: SIP Call
connection: 198.51.100.7
bandwidth: CT, 5000
timing: 0, 0
media: Video, 56436, Rtp/Savpf, [120]
/home/dminor/src/firefox/media/webrtc/signaling/gtest/sdp_unittests.cpp:3798: Failure
Expected: ("") != (GetParseErrors()), actual: "" vs ""
[  FAILED  ] RoundTripSerialize/NewSdpTest.ParseInvalidSimulcastNotReceiving/0, where GetParam() = (false, false) (0 ms)

Comment 1

[Johannes Willbold]

Attachment: Bug 1432936: Added sanity check for sendonly attribute.

Moved RustSdpAttributeType and renamaed it to SdpAttributeType
Replaced the has_extmap_attribute functions by a generic has_attribute function
Added a sanity check, that checks for sendonly and if simulcast defines receive options

Review commit: https://reviewboard.mozilla.org/r/249028/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249028/

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8983175 [details]
Bug 1432936: Added sanity check for sendonly attribute.

https://reviewboard.mozilla.org/r/249028/#review255534

::: media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs:613
(Diff revision 1)
> +    for msection in &session.media {
> +        if msection.get_attribute(SdpAttributeType::Sendonly).is_some() {
> +            if let Some(&SdpAttribute::Simulcast(ref x)) = msection.get_attribute(SdpAttributeType::Simulcast) {
> +                if x.receive.len() > 0 {
> +                    return Err(SdpParserError::Sequence {
> +                        message: "Simulcast can't define send parameters for recvonly".to_string(),

Isn't this the other way around?

Comment 3

[Johannes Willbold]

Comment on attachment 8983175 [details]
Bug 1432936: Added sanity check for sendonly attribute.

https://reviewboard.mozilla.org/r/249028/#review255534

> Isn't this the other way around?

Yes absolutely

Comment 4

[Johannes Willbold]

Comment on attachment 8983175 [details]
Bug 1432936: Added sanity check for sendonly attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249028/diff/1-2/

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8983175 [details]
Bug 1432936: Added sanity check for sendonly attribute.

https://reviewboard.mozilla.org/r/249028/#review255576

Comment 6

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 6ee80e70c3e754cbd834923504861e650e680a62 -d c9ae7e47a6ef: rebasing 467229:6ee80e70c3e7 "Bug 1432936: Added sanity check for sendonly attribute. r=bwc" (tip)
merging media/webrtc/signaling/gtest/sdp_unittests.cpp
merging media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
merging media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
warning: conflicts while merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 7

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 6ee80e70c3e754cbd834923504861e650e680a62 -d 033f5f2b2582: rebasing 467268:6ee80e70c3e7 "Bug 1432936: Added sanity check for sendonly attribute. r=bwc" (tip)
merging media/webrtc/signaling/gtest/sdp_unittests.cpp
merging media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
merging media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
warning: conflicts while merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 8

[Johannes Willbold]

Comment on attachment 8983175 [details]
Bug 1432936: Added sanity check for sendonly attribute.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249028/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e383e893aa70
Added sanity check for sendonly attribute. r=bwc

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/e383e893aa70