Repository navigation
panic on checkNegotiationNeeded #1494
Description
Seems there is a case where Track is nil while executing checkNegotiationNeeded. I don't think this should be possible, since the Senders direction is Send* but it doesn't have a track set. I think all senders in Send* state should have a track defined.
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x15caf3c]
goroutine 4980 [running]:
github.com/pion/webrtc/v3.(*RTPSender).Track(0x0, 0x0)
/Users/tvanas/go/pkg/mod/github.com/pion/webrtc/v3@v3.0.0-beta.10/rtpsender.go:80 +0x3c
github.com/pion/webrtc/v3.(*PeerConnection).checkNegotiationNeeded(0xc00011eb60, 0x15dc500)
/Users/tvanas/go/pkg/mod/github.com/pion/webrtc/v3@v3.0.0-beta.10/peerconnection.go:359 +0x91a
github.com/pion/webrtc/v3.(*PeerConnection).negotiationNeededOp(0xc00011eb60)
/Users/tvanas/go/pkg/mod/github.com/pion/webrtc/v3@v3.0.0-beta.10/peerconnection.go:301 +0x90
github.com/pion/webrtc/v3.(*operations).start(0xc0009aa240)
/Users/tvanas/go/pkg/mod/github.com/pion/webrtc/v3@v3.0.0-beta.10/operations.go:84 +0x61
created by github.com/pion/webrtc/v3.(*operations).Enqueue
/Users/tvanas/go/pkg/mod/github.com/pion/webrtc/v3@v3.0.0-beta.10/operations.go:35 +0xcb
exit status 2
Activity
tomsun commentedon Nov 5, 2020
Have had the same thing happen, possibly a few times
Sean-Der commentedon Nov 6, 2020
Really sorry about that @tomsun!
We are planning on making this feature only fire if you set
OnNegotiationNeededand that should help. There is a lot of complexity here we are still working on nailing down :)@OrlandoCo Did you get a chance to investigate this at all? Any idea how hard/easy it is? I can probably grab it tomorrow if you haven't start yet!
OrlandoCo commentedon Nov 6, 2020
@Sean-Der Sorry I have been fighting with a nasty chrome bug for a week now, andI have not had time, hope we can solve it soon!
Fix RemoveTrack race
Fix RemoveTrack race
Fix RemoveTrack race
tomsun commentedon Nov 23, 2020
Same in beta 12:
Also a similar but different one:
Don't run OnNegotiationNeeded if handler not set
Sean-Der commentedon Nov 23, 2020
@tomsun sorry about that! I opened #1548
We will get that merged today and tagged so you can stop seeing that :(
Don't run OnNegotiationNeeded if handler not set
Sean-Der commentedon Nov 23, 2020
@tomsun I just tagged
v3.0.0-beta.14, so you shouldn't hit this anymore!We are two PRs away from
v3.0.0as well. Will be working on lots of stabilization tasks as we get closer :)thanks for using Pion, really appreciate the patience with the bugs.
tomsun commentedon Nov 23, 2020
🙌 thanks @Sean-Der - awesome!
(Mainly trying out https://github.com/pion/webrtc through https://github.com/pion/ion-sfu - maybe you're right that the sfu doesn't implement renegotiate itself..? Trying out your patch 00ec48a now 🙏 , on top of where we otherwise were, using some replace statements in relevant go.mod:s )
(Also using pion/webrtc client-side, with
peerConnection.OnNegotiationNeededimplemented. A crash in that client has a slightly better recovery path though, as it only has to recover itself. Haven't actually seen the above crashes in the client though (although the logs in that app are quite noisy))Fix panic on checkNegotiationNeeded
Fix panic on checkNegotiationNeeded
tekig commentedon Apr 3, 2021
The problem came up when removing a track that was not thread safe that the PR closed. Now sender cannot become unexpectedly nil. I think the problem can be closed.
Sean-Der commentedon Apr 3, 2021
That's great news! Sorry that stuff got lost in the shuffle @tekig :(