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

jianjunz commented Oct 24, 2019

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

@jianjunz jianjunz force-pushed the jianjunz:event-on-close branch from 8316bd0 to a97b0e0 Oct 25, 2019
@henbos
henbos approved these changes Nov 7, 2019
Copy link
Contributor

henbos left a comment

LGTM with optional suggestion

await listenToIceConnected(pc2);

pc2.oniceconnectionstatechange = t.unreached_func();
pc2.close();

This comment has been minimized.

Copy link
@henbos

henbos Nov 7, 2019

Contributor

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

This comment has been minimized.

Copy link
@jan-ivar

jan-ivar Nov 7, 2019

Contributor

Good point, or the test may end prematurely. Probably even a t.set_timeout or whatever it is called.

This comment has been minimized.

Copy link
@jianjunz

jianjunz Nov 8, 2019

Author Member

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.

@jianjunz jianjunz force-pushed the jianjunz:event-on-close branch from a97b0e0 to 04062b2 Nov 8, 2019
@community-tc-integration

This comment has been minimized.

Copy link

community-tc-integration bot commented on 04062b2 Nov 8, 2019

Submitting the task to Taskcluster failed. Details

Taskcluster-GitHub attempted to create a task for this event with the following scopes:

[
  "assume:repo:github.com/web-platform-tests/wpt:pull-request",
  "queue:route:statuses",
  "queue:scheduler-id:taskcluster-github"
]

The expansion of these scopes is not sufficient to create the task, leading to the following:

Client ID static/taskcluster/github does not have sufficient scopes and is missing the following scopes:

{
  "AnyOf": [
    {
      "AnyOf": [
        "queue:create-task:highest:aws-provisioner-v1/wpt-docker-worker",
        "queue:create-task:very-high:aws-provisioner-v1/wpt-docker-worker",
        "queue:create-task:high:aws-provisioner-v1/wpt-docker-worker",
        "queue:create-task:medium:aws-provisioner-v1/wpt-docker-worker",
        "queue:create-task:low:aws-provisioner-v1/wpt-docker-worker",
        "queue:create-task:very-low:aws-provisioner-v1/wpt-docker-worker",
        "queue:create-task:lowest:aws-provisioner-v1/wpt-docker-worker"
      ]
    },
    {
      "AnyOf": [
        "queue:create-task:aws-provisioner-v1/wpt-docker-worker",
        {
          "AllOf": [
            "queue:define-task:aws-provisioner-v1/wpt-docker-worker",
            "queue:task-group-id:taskcluster-github/Qcyyj0yJQlmFS83dmej-EQ",
            "queue:schedule-task:taskcluster-github/Qcyyj0yJQlmFS83dmej-EQ/eKvCkF_iR26MiRyfJAWU7w"
          ]
        }
      ]
    }
  ]
}

This request requires the client to satisfy the following scope expression:

{
  "AllOf": [
    "queue:route:statuses",
    {
      "AnyOf": [
        {
          "AllOf": [
            "queue:scheduler-id:taskcluster-github",
            {
              "AnyOf": [
                "queue:create-task:highest:aws-provisioner-v1/wpt-docker-worker",
                "queue:create-task:very-high:aws-provisioner-v1/wpt-docker-worker",
                "queue:create-task:high:aws-provisioner-v1/wpt-docker-worker",
                "queue:create-task:medium:aws-provisioner-v1/wpt-docker-worker",
                "queue:create-task:low:aws-provisioner-v1/wpt-docker-worker",
                "queue:create-task:very-low:aws-provisioner-v1/wpt-docker-worker",
                "queue:create-task:lowest:aws-provisioner-v1/wpt-docker-worker"
              ]
            }
          ]
        },
        {
          "AnyOf": [
            "queue:create-task:aws-provisioner-v1/wpt-docker-worker",
            {
              "AllOf": [
                "queue:define-task:aws-provisioner-v1/wpt-docker-worker",
                "queue:task-group-id:taskcluster-github/Qcyyj0yJQlmFS83dmej-EQ",
                "queue:schedule-task:taskcluster-github/Qcyyj0yJQlmFS83dmej-EQ/eKvCkF_iR26MiRyfJAWU7w"
              ]
            }
          ]
        }
      ]
    }
  ]
}

  • method: createTask
  • errorCode: InsufficientScopes
  • statusCode: 403
  • time: 2019-11-08T01:49:29.214Z
@henbos
henbos approved these changes Nov 8, 2019
@henbos henbos merged commit 2d737f5 into web-platform-tests:master Nov 8, 2019
10 checks passed
10 checks passed
Azure Pipelines Build #20191108.7 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
pc2.oniceconnectionstatechange = t.unreached_func();
pc2.close();
await Promise.resolve();
assert_true(pc2.iceConnectionState === 'closed');

This comment has been minimized.

Copy link
@jan-ivar

jan-ivar Nov 8, 2019

Contributor

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

This comment has been minimized.

Copy link
@jianjunz

jianjunz Nov 11, 2019

Author Member

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.

@jianjunz jianjunz deleted the jianjunz:event-on-close branch Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.