Skip to content
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

Merged
merged 1 commit into from Nov 8, 2019

Conversation

jianjunz
Copy link
Member

Add a test case for w3c/webrtc-pc#2335 and w3c/webrtc-pc#2336.

Copy link
Contributor

@henbos henbos left a 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();
Copy link
Contributor

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

Copy link
Member

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.

Copy link
Member Author

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.

@henbos henbos merged commit 2d737f5 into web-platform-tests:master Nov 8, 2019
pc2.oniceconnectionstatechange = t.unreached_func();
pc2.close();
await Promise.resolve();
assert_true(pc2.iceConnectionState === 'closed');
Copy link
Member

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));

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants