829761 - WebRTC should accept stated direction for ip=0.0.0.0, port != 0

Status: RESOLVED FIXED

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

Description

[Adam Roach [:abr]]

In order to facilitate trickle ice as it's currently being used, we need the ability to receive SDP with an IP of 0.0.0.0 and still treat the stream as active (sendonly, recvonly, or sendrecv, as appropriate), so long as the corresponding m= section has a non-zero port.

This is necessary due to the fact that the RTCWEB and Trickle ICE work is still in flight. This may or may not be the final behavior that we want to exhibit.

Comment 1

[Adam Roach [:abr]]

The code in question is in gsm_sdp.c:

    /*
     * To support legacy way of signaling remote hold, we will interpret
     * c=0.0.0.0 to be a=inactive
     */
    if (dest_addr->type == CPR_IP_ADDR_IPV4 &&
        dest_addr->u.ip4 == 0) {

        direction = SDP_DIRECTION_INACTIVE;
    } else {

        //todo IPv6: reject the request.
    }

Comment 2

[Adam Roach [:abr]]

Attachment: Remove inactive check for sdpmode if port != 0

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 701942 [details] [diff] [review]
Remove inactive check for sdpmode if port != 0

Review of attachment 701942 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +591,5 @@
>  
>      // Now call CreateOffer as JS would
>      pObserver->state = TestObserver::stateNoResponse;
>      ASSERT_EQ(pc->CreateOffer(constraints), NS_OK);
> +    ASSERT_TRUE_WAIT(pObserver->state != TestObserver::stateNoResponse,

Can you explain why this is needed?

@@ +745,5 @@
>  
>  private:
>    void SDPSanityCheck(std::string sdp, uint32_t flags, bool offer)
>    {
> +    ASSERT_TRUE(pObserver->state == TestObserver::stateSuccess);

I thought ASSERT_TRUE() did a return if it failed.

Comment 4

[Adam Roach [:abr]]

(In reply to Eric Rescorla (:ekr) from comment #3)
> Comment on attachment 701942 [details] [diff] [review]
> Remove inactive check for sdpmode if port != 0
> 
> Review of attachment 701942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +591,5 @@
> >  
> >      // Now call CreateOffer as JS would
> >      pObserver->state = TestObserver::stateNoResponse;
> >      ASSERT_EQ(pc->CreateOffer(constraints), NS_OK);
> > +    ASSERT_TRUE_WAIT(pObserver->state != TestObserver::stateNoResponse,
> 
> Can you explain why this is needed?

It used to only check/wait for it to change to success. This allows it to return immediately if the state changes from "no response" to "failed."


> @@ +745,5 @@
> >  
> >  private:
> >    void SDPSanityCheck(std::string sdp, uint32_t flags, bool offer)
> >    {
> > +    ASSERT_TRUE(pObserver->state == TestObserver::stateSuccess);
> 
> I thought ASSERT_TRUE() did a return if it failed.

Ah, yes. This is overkill. I'll remove the second check.

Comment 5

[Adam Roach [:abr]]

Attachment: Remove inactive check for sdpmode if port != 0,

Comment 6

[Adam Roach [:abr]]

Comment on attachment 701988 [details] [diff] [review]
Remove inactive check for sdpmode if port != 0,

Carrying forward r=ekr

Comment 7

[Ethan Hugg [:ehugg]]

Comment on attachment 701988 [details] [diff] [review]
Remove inactive check for sdpmode if port != 0,


https://hg.mozilla.org/integration/mozilla-inbound/rev/b93357d16604

Comment 8

[Ed Morley [:emorley]]

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