Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Getting : TypeError: undefined is not an object (evaluating 'stream.id') when removing a shared screen #268

Closed
magestican opened this issue Mar 30, 2017 · 11 comments
Assignees
Labels
Milestone

Comments

@magestican
Copy link

Whenever i remove a shared screen stream on chrome in windows, i get this exception on my ios app built with iosrtc :

callbackFromNative — cordova.js:314 TypeError: undefined is not an object (evaluating 'stream.id').

I have gone through the source code and there are 3 or 4 situations when source.id is being used by iosrtc, could a check be added anywhere stream.id is being used so this doesnt happen?

@ibc
Copy link
Collaborator

ibc commented Apr 3, 2017

I have gone through the source code and there are 3 or 4 situations when source.id is being used by iosrtc, could a check be added anywhere stream.id is being used so this doesnt happen?

I think it's better to find out why the error happens (rather than just checking conditions with no knowledge of why they could fail). BTW: have you identified the exact line in which such a stream.id fails?

@saghul
Copy link
Collaborator

saghul commented Apr 4, 2017

Agreed with @ibc here. Adding a check is a stopgap at best.

@justindujardin
Copy link

justindujardin commented Jun 14, 2017

We've run into this issue as well, I can elaborate. The use-case is a long-lived peer connection where remote streams are frequently added/removed. On removal the stream is not found in remoteStreams, and an exception is generated when stream.id is used.

The root cause appears to be #164 where a unique ID was added to the stream ids that are tracked internally. The generated ids are not properly communicated to the javascript layer, and so the cleanup logic fails when a remote stream is removed.

We originally tried removing the unique IDs because we don't use Kurento, but this resulted in some media failures, so I later patched the javascript to look for a substring match. This works fine because we generate unique ids, but would fail for the Kurento case.

I think that a more robust fix would be to make sure that the randomly generated ID is communicated to the javascript layer properly from Swift. I looked at doing this myself, but my Swift-fu is weak, and I couldn't figure out how to communicate the id properly in a short period of time.

@ibc
Copy link
Collaborator

ibc commented Jun 14, 2017

@justindujardin I don't understand. #164 was already fixed so long ago, and random stream ID generated in Swift land is properly exposed/propagated to JavaScript 1, 2.

@justindujardin
Copy link

@ibc I setup a test to show the problem happening in the debugger. The issue, I now recall, is that when the removestream event is generated, it does not have the correct id.

During the stream add, the id is setup properly:
add-stream-id-correct

During removestream, the UUID is not present in the streamId:
remove-stream-id-wrong

@ibc
Copy link
Collaborator

ibc commented Jun 14, 2017

The bug is here:

When the pc.onaddstream fires, the Swift code creates a PluginMediaStream wrapper that has a id property defined as follows:

self.id = rtcMediaStream.label + "-" + UUID().uuidString

Such a id value is propagated to JS becoming the stream.id property.

However, when the onremovestream fires, the swift codes does not internally get its corresponding Swift PluginMediaStream but just fires an event to the JS by just reporting the rtcMediaStream.label (being rtcMediaStream the ObjC RTCMediaStream instance), as you can see here:

self.eventListener([
	"type": "removestream",
	"streamId": rtcMediaStream.label
])

Obviously, rtcMediaStream.label does not match rtcMediaStream.label + "-" + UUID().uuidString, so here the problem.

This bug is a regression produced by #164, which indeed solves a different issue, but creates this one.

iosrtcPlugin.swift stores the PluginMediaStream.swift instances in a map indexed by their id above, so when a remote stream is removed and the PluginRTCPeerConnection.swift invokes this code it does not even remove the stream from the internal map. So the issue happens in both JS and Swift lands.

Since the ObjC RTCMediaStream.h does not have a seteable id property, the plugin can not set the created id into it. So the solution coming to my mind is the following:

  • PluginRTCPeerConnection.swift should hold a map with:
    • Key: The ObjC RTCMediaStream instance.
    • Value: The id as created here.
  • When onremovestream is fired within PluginRTCPeerConnection.swift, it should retrieve its corresponding id from the previous map and propagate it to the JS (via the removestream event) rather than the rtcMediaStream.label.

Said that:

  • I don't know if Swift allows having a "map" with class instances as keys, but for sure a similar approach can be achieved.
  • I don't have a iOS device so I cannot do this.
  • I hope the info given here can help others to create a PR that fixes the issue.

@mariuszbeltowski
Copy link

Did any of you guys manage to fix it? @magestican @ibc @saghul @justindujardin

@l7s
Copy link

l7s commented Jun 24, 2018

Hi, sorry can't do PR (as got only internal git repos behind firewall), but here is patch which fixes the issue for us:

--- a/src/PluginRTCPeerConnection.swift
+++ b/src/cordova-plugin-iosrtc/PluginRTCPeerConnection.swift
@@ -19,6 +19,7 @@ class PluginRTCPeerConnection : NSObject, RTCPeerConnectionDelegate, RTCSessionD
 	var onSetDescriptionSuccessCallback: (() -> Void)!
 	var onSetDescriptionFailureCallback: ((_ error: Error) -> Void)!
 	var onGetStatsCallback: ((_ array: NSArray) -> Void)!
+	var mediaStreamIdMap: [String: String]
 
 	init(
 		rtcPeerConnectionFactory: RTCPeerConnectionFactory,
@@ -36,6 +37,7 @@ class PluginRTCPeerConnection : NSObject, RTCPeerConnectionDelegate, RTCSessionD
 		self.eventListener = eventListener
 		self.eventListenerForAddStream = eventListenerForAddStream
 		self.eventListenerForRemoveStream = eventListenerForRemoveStream
+		self.mediaStreamIdMap = [:]
 	}
 
 
@@ -544,10 +546,13 @@ class PluginRTCPeerConnection : NSObject, RTCPeerConnectionDelegate, RTCSessionD
 
 	func peerConnection(_ rtcPeerConnection: RTCPeerConnection!,
 		addedStream rtcMediaStream: RTCMediaStream!) {
-		NSLog("PluginRTCPeerConnection | onaddstream")
-
+		
 		let pluginMediaStream = PluginMediaStream(rtcMediaStream: rtcMediaStream)
 
+		NSLog("PluginRTCPeerConnection | onaddstream [" + pluginMediaStream.id + "]")
+        
+		self.mediaStreamIdMap[rtcMediaStream.label] = pluginMediaStream.id
+        
 		pluginMediaStream.run()
 
 		// Let the plugin store it in its dictionary.
@@ -563,14 +568,19 @@ class PluginRTCPeerConnection : NSObject, RTCPeerConnectionDelegate, RTCSessionD
 
 	func peerConnection(_ rtcPeerConnection: RTCPeerConnection!,
 		removedStream rtcMediaStream: RTCMediaStream!) {
-		NSLog("PluginRTCPeerConnection | onremovestream")
+        
+		let streamId = self.mediaStreamIdMap[rtcMediaStream.label]!
+        
+		self.mediaStreamIdMap.removeValue(forKey: rtcMediaStream.label)
+        
+		NSLog("PluginRTCPeerConnection | onremovestream [" + streamId + "]")
 
 		// Let the plugin remove it from its dictionary.
 		self.eventListenerForRemoveStream(rtcMediaStream.label)
 
 		self.eventListener([
 			"type": "removestream",
-			"streamId": rtcMediaStream.label  // NOTE: No "id" property yet.
+			"streamId": streamId
 		])
 	}

Regards,
Chris

@hthetiot
Copy link
Contributor

hthetiot commented Sep 4, 2019

Will make PR @l7s thx.

@hthetiot hthetiot added this to the 5.0.x milestone Sep 4, 2019
@hthetiot hthetiot added the bug label Sep 4, 2019
@hthetiot hthetiot self-assigned this Sep 10, 2019
@hthetiot
Copy link
Contributor

@hthetiot hthetiot added the webrtc-api webrtc-api related label Sep 10, 2019
@hthetiot hthetiot modified the milestones: 5.0.x, 5.0.2 Sep 13, 2019
@hthetiot
Copy link
Contributor

@l7s Thank you I made a PR and confirmed the fix, will be on 5.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants