-
Notifications
You must be signed in to change notification settings - Fork 117
Fix "does not allow remotely initiated RID renegotiation" step #2757
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
Conversation
Editor's agree the original language was probably overly strict compare to initial intent of it (as judged by the summary in the next sentence after the modified language). But the restrictive language may still match current implementations. So we should write some tests to confirm whether this is true or not before merging this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This effectively says "allow answers to change RIDs".
This may be too open-ended, but I think that will be fixed by #2758 once it's finished.
Approving.
I've written some tentative WPT tests in https://jsfiddle.net/jib1/opsv1ugf/ with console.log statements instead of asserts in places, and the results are in. Chrome, Safari and Edge pretty much yield the same results. Firefox only passes the first (existing) test since we're about to update set/getParameters to spec. In Chrome: (Note the last 3 PASSes illustrate implemented behaviors, not spec compliance)
To summarize:
1, 2, 6 and 7 violate the spec I think. We should perhaps assess how web compatible we think it would be to enforce the spec at this point. |
4 also violates the spec since it doesn't fail, I think. |
These results seem reasonable, in general. So as a goal, it would seem like a good idea to align the spec to the implementations. |
If we agree on this, then I think this means the paragraph this PR is modifying instead goes away entirely, because of 1. |
@Orphis does this way forward we suggest make sense to you? |
I am worried about 7. In particular, if the offer is 1,2,3 and the answer is 1,foo,2, exactly what is being removed? If we want to be lenient, I would treat any answer that has changes as erroneous layers and ignore them. Thus: 1,2,3 + 1,2 -> 1,2 (narrowing allowed) And CreateOffer() after 1,2,3 + 1,2 should offer 1,2 (no widening allowed from either side). |
The conversation moved to #2762 (comment) where the current suggestion seems to be to fail over 7. |
Fixes #2743.
Preview | Diff