Reject candidates from old generation

[JoeTurki]

Description

Return an error if a candidate with a username fragment that does not match the username fragment in the remote description is added. This usually indicates that the candidate was generated before the renegotiation.

Last time I tested Chrome silently drops such candidates, firefox returns an error.

Maybe we should also handle "generation" extension.

Reference issue

Fixes #2993
Fixes #2511

[Sean-Der]

I bet some users have code that tears down the PeerConnection entirely on error. This change could trigger that :/

I agree with rejecting candidates and doing a logger.Error I just don't want to negatively impact anyone who is just auto upgrading.

Or is it doing wrong by users to not return an error?

[JoeTurki]

I agree with rejecting candidates and doing a logger.Error I just don't want to negatively impact anyone who is just auto upgrading.
Or is it doing wrong by users to not return an error?

This makes sense, firefox implementation around generations seems extreme, and returning errors already broke few of the tests :)

[codecov]

Codecov Report

Attention: Patch coverage is 78.12500% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.74%. Comparing base (d6154f6) to head (465d8bd).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
peerconnection.go	78.12%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3113      +/-   ##
==========================================
- Coverage   78.77%   78.74%   -0.03%     
==========================================
  Files          91       91              
  Lines       11383    11401      +18     
==========================================
+ Hits         8967     8978      +11     
- Misses       1932     1936       +4     
- Partials      484      487       +3     

Flag	Coverage Δ
go	80.64% <78.12%> (-0.04%)	⬇️
wasm	63.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JoeTurki]

Okay the error is gone, we still test it, I had to do a refactor to make the linter happy. However I don't know how I feel about this change overall, It aligns with other clients, and it shouldn't break any other client but I'm still not confident.

[Sean-Der]

Does accepting candidates before the first setRemoteDescription still have the same behavior. I have seen people on that behavior.

I think you should merge :) if it breaks things we will hear and fix. I think you have done more than your due diligence

[JoeTurki]

Does accepting candidates before the first setRemoteDescription still have the same behavior. I have seen people on that behavior.

• First setRemoteDescription call: The behavior remains the same: we return &rtcerr.InvalidStateError{Err: ErrNoRemoteDescription}.

• Second setRemoteDescription call: Here the behavior changes, but in a way that should prevent more problems than it might cause. we avoid accidentally applying late ICE candidates, like with @jech issue