Crash in RTCPeerConnection.close()

[ibc]

After merging #159 (so cordova-ios 4 is used) and #160, calling peerConnection.close() crashes with:

It does not crash as follows:

func RTCPeerConnection_close(command: CDVInvokedUrlCommand) {
        NSLog("iosrtcPlugin#RTCPeerConnection_close()")

        let pcId = command.argumentAtIndex(0) as! Int
        let pluginRTCPeerConnection = self.pluginRTCPeerConnections[pcId]

        if pluginRTCPeerConnection == nil {
            NSLog("iosrtcPlugin#RTCPeerConnection_close() | ERROR: pluginRTCPeerConnection with pcId=%@ does not exist", String(pcId))
            return;
        }

        dispatch_async(self.queue) {
            pluginRTCPeerConnection!.close()
        }

        // Remove the pluginRTCPeerConnection from the dictionary.
        self.pluginRTCPeerConnections[pcId] = nil
    }

I've also tried passing [weak pluginRTCPeerConnection] in to dispatch_async() but it crashes as well, so I'm worried whether the native object is not properly deallocated.

If so, would this work?

        dispatch_async(self.queue) {
            pluginRTCPeerConnection!.close()
            pluginRTCPeerConnection = nil
        }

/cc @oNaiPs

[ibc]

Well, I mean this:

func RTCPeerConnection_close(command: CDVInvokedUrlCommand) {
    NSLog("iosrtcPlugin#RTCPeerConnection_close()")

    let pcId = command.argumentAtIndex(0) as! Int
    var pluginRTCPeerConnection = self.pluginRTCPeerConnections[pcId]

    if pluginRTCPeerConnection == nil {
        NSLog("iosrtcPlugin#RTCPeerConnection_close() | ERROR: pluginRTCPeerConnection with pcId=%@ does not exist", String(pcId))
        return;
    }

    dispatch_async(self.queue) {
        pluginRTCPeerConnection!.close()
        pluginRTCPeerConnection = nil
    }

    // Remove the pluginRTCPeerConnection from the dictionary.
    self.pluginRTCPeerConnections[pcId] = nil
}

[ibc]

I've committed the above code to avoid the crash, but I understand this must be re-checked.

[mark-veenstra]

@saghul even with the fix you got a crash? I am going to test this now also here

[ibc]

With the already applied commit I don't get the crash.

[oNaiPs]

@ibc i'm sorry I think I forgot to modify that function in my previous PR. Would this work?

Perhaps the problem is because when you do self.pluginRTCPeerConnections[pcId] = nil there are no more references to the RTCPeerConnection, so when dispatch_async is executed, the variable was already dealllocated. Could you try to put this line inside the dispatch_async after calling close()?

[oNaiPs]

Oh, and make sure you do [weak pluginRTCPeerConnection, unowned self]

[saghul]

I tested master a few hours ago, will test it again shortly.
On Apr 14, 2016 14:59, "José Pereira" [email protected] wrote:

Oh, and make sure you do [weak pluginRTCPeerConnection, unowned self]

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#161 (comment)

[saghul]

Tested again, with latest master and everything works as expected, great work everyone! 😍

[mark-veenstra]

Same here, no issues! Thanks for the work!

[ibc]

@saghul @mark-veenstra, current master was "fixed" by me yesterday so it does not crash, but as @oNaiPs said above some changes most be done for the PeerConnection to be released.

[ibc]

@oNaiPs wouldn't also need [..., unowned self] here?:

dispatch_async(self.queue) { [weak pluginRTCPeerConnection] in
            pluginRTCPeerConnection?.createOffer(options,
                callback: { (data: NSDictionary) -> Void in
                    self.emit(command.callbackId,
                        result: CDVPluginResult(status: CDVCommandStatus_OK, messageAsDictionary: data as [NSObject : AnyObject])
                    )
                },
                errback: { (error: NSError) -> Void in
                    self.emit(command.callbackId,
                        result: CDVPluginResult(status: CDVCommandStatus_ERROR, messageAsString: error.localizedDescription)
                    )
                }
            )
        }

I expect that given that self if used within the given callbacks. Am I right?

[ibc]

@oNaiPs I've committed right now a fix following your suggestions, may you please check it? I've tested it and it does not crash. However I do not understand why I must pass [unowned self] here:

dispatch_async(self.queue) { [weak pluginRTCPeerConnection, unowned self] in
            pluginRTCPeerConnection!.close()

            // Remove the pluginRTCPeerConnection from the dictionary.
            self.pluginRTCPeerConnections[pcId] = nil
        }

At the end self points to the cordova CDVPlugin instance, do I miss something?

[oNaiPs]

You;re right, you don't have to. CDVPlugin is not going to be deallocated anyways.

[mark-veenstra]

We still encounter (sometimes) a crash in this close.

[jgsacris]

I have been testing this fix for a while on an iPhone 5 with 9.3.1 and it no longer crashed.

[ibc]

Thanks @jgsacris

[ibsus]

Unfortunately we are continuing to get a crash, even with the new code implemented. Same place as OP.

func RTCPeerConnection_close(command: CDVInvokedUrlCommand) {
    NSLog("iosrtcPlugin#RTCPeerConnection_close()")

    let pcId = command.argumentAtIndex(0) as! Int
    let pluginRTCPeerConnection = self.pluginRTCPeerConnections[pcId]

    if pluginRTCPeerConnection == nil {
        NSLog("iosrtcPlugin#RTCPeerConnection_close() | ERROR: pluginRTCPeerConnection with pcId=%@ does not exist", String(pcId))
        return;
    }

    dispatch_async(self.queue) { [weak pluginRTCPeerConnection] in
        pluginRTCPeerConnection!.close() <!------ EXC_BREAKPOINT(code=1)

        // Remove the pluginRTCPeerConnection from the dictionary.
        self.pluginRTCPeerConnections[pcId] = nil
    }
}

Our configuration is as follows
iOS 9.3.5
Cordova ios 4.2.1

[mark-veenstra]

@jpkolind We are also encountering same issue, but when we change the code as follows it works like a charm:

func RTCPeerConnection_close(command: CDVInvokedUrlCommand) {
    NSLog("iosrtcPlugin#RTCPeerConnection_close()")

    let pcId = command.argumentAtIndex(0) as! Int
    let pluginRTCPeerConnection = self.pluginRTCPeerConnections[pcId]

    if pluginRTCPeerConnection == nil {
        NSLog("iosrtcPlugin#RTCPeerConnection_close() | ERROR: pluginRTCPeerConnection with pcId=%@ does not exist", String(pcId))
        return;
    }

    dispatch_async(self.queue) { [weak pluginRTCPeerConnection] in
        // Needs extra check to avoid crash on the close
        if pluginRTCPeerConnection != nil {
            pluginRTCPeerConnection!.close()
        }

        // Remove the pluginRTCPeerConnection from the dictionary.
        self.pluginRTCPeerConnections[pcId] = nil
    }
}

[saghul]

@mark-veenstra I think the crash can only happen if close it called twice. Since the actual close happens asynchronously, it's possible for close to be called twice and for the close to happen also on the Swift side. Now, if that happens, it's also possible that the first nil check is passed but the object becomes nil in the async block, so the fix LGTM. Can you please send a PR?

[ibsus]

Thank you! That worked perfectly.