peerconnection.removeStream(null) crashes

RESOLVED WONTFIX

Status

()

Core
WebRTC
--
critical
RESOLVED WONTFIX
5 years ago
3 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

({crash})

Trunk
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webrtc][blocking-webrtc-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 715011 [details] [diff] [review]
trivial protection against RemoveStream(null)

we may want a better fix, and also the API probably demands a JS-level error of some sort
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?
Since Bug 844295 will stub removeStream and is blocking-webrtc+, we don't need this fix for initial release.
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc-]
(Assignee)

Comment 4

3 years ago
RemoveStream() will be going away, so WONTFIX
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.