783308 - When SetRemoteDesc is called multiple times streams are added but not replaced or removed.

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Ethan Hugg [:ehugg]]

-- When setRemoteDescription is called more than once the remote streams are created and added to the list, but the older ones are never removed.  This causes the remote stream list to grow each time.

-- The streams in the remote stream table are limited by CC_MAX_TRACKS which is 8, but when new ones are added the array size is never checked.  So stream number 9 gets written off the end.  CC_MAX_TRACKS is also used for the track array.  I imagine the plan is to make them dynamic, but for now I can add a CC_MAX_STREAMS set to some reasonably large number and check each time a stream is added and fail when trying to add off the end 

-- These checks of array indexes should protecte against negative values
  PeerConnectionImpl.cpp:986
  
  if (index >= (int) mRemoteSourceStreams.Length())
    return NULL;

-- This line doesn't protect against a NULL return from GetRemoteStream()  
  PeerConnectionImpl.cpp:136
   
  stream = mPC->GetRemoteStream(streams->media_stream_id)->GetMediaStream();

-- Also I found this initialization of the remote_media_stream_tbl
  gsm_sdp.c:254
 
    *(dcb_p->remote_media_stream_tbl) = g_media_remote_stream_table;

   g_media_remote_stream_table is initialized with all zeros. Unless you know of a reason to do it this way, I'm thinking to using memset on the new remote_media_stream_tbl instead.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: When SetRemoteDesc is called multiple times streams are added but not replaced

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: When SetRemoteDesc is called multiple times streams are added but not replaced

Comment 3

[Ethan Hugg [:ehugg]]

Attachment: When SetRemoteDesc is called multiple times streams are added but not replaced

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 652814 [details] [diff] [review]
When SetRemoteDesc is called multiple times streams are added but not replaced

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

lgtm with one minor suggestion.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4055,5 @@
>          	 * Bubble the stream added event up to the PC UI
>          	 */
>          	if (notify_stream_added) {
>  
> +        	    for (j=0; j < CC_MAX_STREAMS; j++ ) {

Are you sure num_streams and num_tracks != 0 are consistent? Maybe an assert here.

Comment 5

[Ethan Hugg [:ehugg]]

>::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
>@@ +4055,5 @@
>>          	 * Bubble the stream added event up to the PC UI
>>          	 */
>>          	if (notify_stream_added) {
>>  
>> +        	    for (j=0; j < CC_MAX_STREAMS; j++ ) {
>
>Are you sure num_streams and num_tracks != 0 are consistent? Maybe an assert here.

Well, I got rid of num_tracks as it was being incremented off the end of the array.  Instead I memset the the array of streams and then in this loop I look for ones that have been setup by seeing if num_tracks > 0.  Not ideal, but gsmsdp_add_remote_stream currently takes the index as input when adding a remote_stream instead of returning it as output as you might expect.  I assume this is because other places still have the streams somewhat hardcoded to one audio and one video.  I am expecting Enda to change all of this when he implements his plan for multiple a/v streams.

Comment 6

[Ethan Hugg [:ehugg]]

Whoops that first sentence should be 'got rid of num_streams'

Comment 7

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

This landed before or as part of the merge from alder to m-c