1413709 - add tests to detect improper ice restart from answer changing ufrag/pwd

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P2)

Description

[Michael Froman [:mjf]]

In Bug 1405940 I fixed a crash that was ultimately the result of an answer (during renegotiation) containing modified ICE credentials causing an ICE restart when the offerer did not indicate an ICE restart.

Comment 1

[Michael Froman [:mjf]]

Attachment: Bug 1413709 - add tests to detect improper ice restart by answer.

Adding tests that would have shown the issue fixed in Bug 1405940.
If the answer during a renegotiation has modified ICE credentials,
it should cause an error.  These tests check for that error.

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

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8924333 [details]
Bug 1413709 - add tests to detect improper ice restart by answer.

https://reviewboard.mozilla.org/r/195572/#review201042

::: dom/media/tests/mochitest/test_peerConnection_restartIceBadAnswer.html:31
(Diff revision 1)
> +    test.chain.insertAfter("PC_LOCAL_GET_ANSWER",
> +      [
> +        function PC_LOCAL_REWRITE_REMOTE_SDP_ICE_CREDS(test) {
> +          test._remote_answer.sdp =
> +            test._remote_answer.sdp.replace(/a=ice-pwd:.*\r\n/g,
> +                                            "a=ice-pwd:bad-pwd\r\n")
> +                                   .replace(/a=ice-ufrag:.*\r\n/g,
> +                                            "a=ice-ufrag:bad-ufrag\r\n");
> +        }
> +      ], false, 1 // insert after the second PC_LOCAL_GET_ANSWER
> +    );
> +
> +    test.chain.replaceAfter("PC_LOCAL_REWRITE_REMOTE_SDP_ICE_CREDS",
> +      [
> +        function PC_LOCAL_EXPECT_SET_REMOTE_DESCRIPTION_FAIL(test) {
> +          return test.setRemoteDescription(test.pcLocal,
> +                                           test._remote_answer,
> +                                           STABLE)
> +           .then(() => ok(false, "setRemoteDescription must fail"),
> +                 e => is(e.name, "InvalidSessionDescriptionError",
> +                         "setRemoteDescription must fail and did"))
> +         }
> +      ], 0 // replace after first (only) PC_LOCAL_REWRITE_REMOTE_SDP_ICE_CREDS
> +    );

Couldn't we do this with a single replaceAfter?

::: dom/media/tests/mochitest/test_peerConnection_restartIceBadAnswer.html:51
(Diff revision 1)
> +          return test.setRemoteDescription(test.pcLocal,
> +                                           test._remote_answer,
> +                                           STABLE)
> +           .then(() => ok(false, "setRemoteDescription must fail"),
> +                 e => is(e.name, "InvalidSessionDescriptionError",
> +                         "setRemoteDescription must fail and did"))

Do we need a ; here?

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:123
(Diff revision 1)
>    }
>  
>    void
>    AddTransportData(JsepSessionImpl& session, TransportData& tdata)
>    {
>      // Values here semi-borrowed from JSEP draft.

I think this comment needs to move to GenerateNewIceCredentials.

Comment 3

[Michael Froman [:mjf]]

Comment on attachment 8924333 [details]
Bug 1413709 - add tests to detect improper ice restart by answer.

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f3b7e0969c9
add tests to detect improper ice restart by answer. r=bwc

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6f3b7e0969c9