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
Closing a PeerConnection should not fire iceconnectionstatechange event. #19861
Conversation
8316bd0
to
a97b0e0
Compare
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.
LGTM with optional suggestion
await listenToIceConnected(pc2); | ||
|
||
pc2.oniceconnectionstatechange = t.unreached_func(); | ||
pc2.close(); |
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.
Optional suggestion: Add this line before asserting:
await Promise.resolve();
This might catch an implementation that surfaces the invalid iceConnectionState change in a posted task
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.
Good point, or the test may end prematurely. Probably even a t.set_timeout or whatever it is called.
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.
Thanks for your suggestion. A new patch was submitted. Please help merge this PR if it looks good to you. I don't have write access to merge it. Thanks.
This is a test case for w3c/webrtc-pc#2335 and w3c/webrtc-pc#2336.
a97b0e0
to
04062b2
Compare
pc2.oniceconnectionstatechange = t.unreached_func(); | ||
pc2.close(); | ||
await Promise.resolve(); | ||
assert_true(pc2.iceConnectionState === 'closed'); |
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.
Actually, we want to assert first, to test the state is closed synchronously, followed by allowing sufficient time to ensure an event does not come in.
Unfortunately, await Promise.resolve()
only queues a microtask, which merely defers execution to the end of this run of the main event loop. This is not enough time for an event to have fired, since the firing of this event would be queued in a new task. We need:
await new Promise(r => t.step_timeout(r));
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.
Yes, ICE connection state should be set to "closed" synchronously. I'm going to submit a new PR to fix it and wait 100ms for oniceconnectionstatechange
event.
Add a test case for w3c/webrtc-pc#2335 and w3c/webrtc-pc#2336.