842154 - peerconnection.removeStream(null) crashes

Status: RESOLVED WONTFIX

(Core :: WebRTC, defect)

Description

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

Simple error-checking needed on the inputs

I advise testing as part of large API input test for correct response in a mochitest, not a crashtest.

#5  0x00007f4384edc2c7 in sipcc::PeerConnectionMedia::RemoveStream (this=0x4b268f0, aMediaStream=0x0, stream_id=
    0x7fff32a61918) at ../../../../../media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:198
        stream = 0x7fff32a618f0
        __FUNCTION__ = "RemoveStream"
#6  0x00007f4384ed386b in sipcc::PeerConnectionImpl::RemoveStream (this=0x1c3fec0, aMediaStream=0x0)
    at ../../../../../media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:986
        stream_id = 849746424
        res = NS_OK
        stream = 0x0
        hints = 1

Hit while using the social api mobile demo and not sharing my camera on one side, then quitting.

Comment 1

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

Attachment: trivial protection against RemoveStream(null)

we may want a better fix, and also the API probably demands a JS-level error of some sort

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The spec seems to be avoiding any language about throwing here: http://dev.w3.org/2011/webrtc/editor/webrtc.html#methods - It says:

 "2. If stream is not in the RTCPeerConnection object's local streams set, then abort these steps.

My guess would be because anything beyond a null-check might be hard to do synchronously. Making it throw on null is trivial though if we want. Want me to add that patch?

The next question might be: What happens if you call the method with a stream that's not in the set? Do we get an error callback? If yes, then maybe using the same error-callback for null may make more sense?

Comment 3

[Maire Reavy [:mreavy]]

Since Bug 844295 will stub removeStream and is blocking-webrtc+, we don't need this fix for initial release.

Comment 4

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

RemoveStream() will be going away, so WONTFIX