1113030 - Disabled m-lines cause problems

Status: NEW

(Core :: WebRTC, defect, P5)

Description

[Nils Ohlmeier [:drno]]

When the answerer rejects the data channel portion the data channel by setting the port in the m-line to zero the data channel still gets established.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: data_channel_reject_test.patch

A test which sets the port for m=application in the answer to zero, but still the data channel gets established and data can be send through the data channel.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Note: this is directly modifying the SDP; in a real rejection no datachannel could be established as there'd be no port open at the answer side to connect DTLS to.  For that matter: how could this possibly be happening here?  Even if the Offerer ignores the port 0 in the answer, it has no port to connect to - so I think the editing of the SDP here must not be valid - either it's already been installed behind your back, or the editing isn't affecting the copy that get used by the Offerer side.

Please check what's getting installed at the Offerer side (signaling:5 I presume, or via gdb)

Comment 3

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #2)
> Note: this is directly modifying the SDP; in a real rejection no datachannel
> could be established as there'd be no port open at the answer side to
> connect DTLS to.  For that matter: how could this possibly be happening
> here? 

What about with BUNDLE?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Eric Rescorla (:ekr) from comment #3)
> (In reply to Randell Jesup [:jesup] from comment #2)
> > Note: this is directly modifying the SDP; in a real rejection no datachannel
> > could be established as there'd be no port open at the answer side to
> > connect DTLS to.  For that matter: how could this possibly be happening
> > here? 
> 
> What about with BUNDLE?

That would be a question, but as this test doesn't use BUNDLE, it strongly implies that the SDP modification never took affect in the first place (i.e. the bug is in the test framework, not the code).

Comment 5

[Byron Campen [:bwc]]

Does this happen on Nightly? I would expect this to be fixed by the sdparta rewrite.

Comment 6

[Nils Ohlmeier [:drno]]

Attachment: data_channel_reject_test.patch

Test video rejection as well.
This test sets the video m-line port to zero in the offer and drops ICE candidates for the video m-line so the answerer doesn't know video ports on the offerers side.

Comment 7

[Nils Ohlmeier [:drno]]

Randel is right, that this was without Bundle. I'm going to test it later with Bundle.

But I extended the test a little further and disabled the m-line in the offer and the answer. Even though the answerer gets an disabled m-line the answer contains an active m-line. We also happily accept ICE candidates for disabled m-lines (without any errors in the logs - so this is not problem that candidates errors are not reported back to JS land). Further more the new test also drops the ICE candidates for the dis-abled m-line. So the answerer does not know where to send video to. And even though the video m-line also gets disabled in the answer as well the video stream gets established.

So I think we seem to have a bigger issue with disabled m-lines.

Comment 8

[Nils Ohlmeier [:drno]]

So with Byron fix in bug 1016476 the behavior changes quite a bit here. A disabled m-line gets reflected as disabled (with a not ideal rtpmap, see bug 1113443 for that). And there does not seem to connection for the disabled m-section.

Remaining issues:
- we still happily accept ice candidates for disabled m-lines. I tend to think we should return errors for that. But I could get convinced otherwise.
- we still generate ice candidates for disabled m-lines.
- onAddStream never fires on the answerer side if the offer contained a disabled m-section.

Comment 9

[Bulk Bug Changes for mreavy's org]

Mass change P4->P5 to align with new Mozilla triage process.