Wpt: WebRTC tests should close RTCPeerConnection's at end of test

[nils-ohlmeier]

It looks like the current WPT tests for WebRTC do not call close() at the end of each test.

[nils-ohlmeier]

cc @docfaraday

@soareschen what do you think? Could look into calling close() at end of each test?

[jan-ivar]

I think we should close peer connections after every test. Peer connections allocate resources, and without pc.close() we introduce nondeterminism (dependence on GC) in the tests. Implementations are going to have practical limits, but if we wish to stress-test them, we should do so in a controlled test, not implicitly by running countless of independent tests in a row.

For context, we ran into this trying to land transceivers in Firefox. While we hope to improve our implementation to withstand this problem in this particular case, we think this highlights an issue with the WPT tests of RTCPeerConnection.

[soareschen]

@nils-ohlmeier I have submitted #8303 to add clean up handler to each addTransceiver test. Can you review and verify that the clean up helps fix the problem? If it works we can use the same approach and fix the other tests.

Agree that it make sense to have limits on resource allocation, though 16~64 seems a bit low. addTransceiver is synchronous and doesn't give users the impression that it requires heavy resource usage. Follow up we can probably add a separate stress test on addTransceiver in WPT to make sure that "reasonable number" of idle transceivers can be created in practice.

Note to myself: There are similar issues of heavy resource usage when getUserMedia() is called in each test to generate track. We can do proper clean up in each test as well when doing subsequent PRs.

[snuggs]

Follow up we can probably add a separate stress test on addTransceiver in WPT to make sure that "reasonable number" of idle transceivers can be created in practice.

There are similar issues of heavy resource usage when getUserMedia() is called in each test to generate track. We can do proper clean up in each test as well when doing subsequent PRs.

@soareschen great conventions and a nice cowpath to follow.

[soareschen]

8303 is merged. I will modify other test files to close pc as well.

[snuggs]

Great @soareschen. Quick question (2 parts). What's the wg-webrtc flag used for (more specifically the wg). and what is wg-dap. Noticed you added tag and want to know our conventions without assuming too much.

Thanks in advance.

[snuggs]

@soareschen reviewed #8542. Also please see comment above.

[alvestrand]

seems we are having the same discussion in https://bugs.chromium.org/p/chromium/issues/detail?id=836871

[fippo]

this is largely done by now (via chrome export). There are some helpers which create a throw-away peerconnection but I assume those will be eliminated over time.

[snuggs]

@fippo would you mind leaving a breadcrumb to some examples/tests you speak of? Would be helpful. Especially for myself. :-) The last I recall was there was a healthy sized effort late last year ensuring all connections got closed. I assumed then that would be abstracted and refactored. And sounds like it has been.

Thanks in advance!

[fippo]

@snuggs this landed in https://bugs.chromium.org/p/chromium/issues/detail?id=836871#c44 via Chrome so we could use the bots to ensure this did not change any test expecations. Landed here in #11455

The tooling used is in #11524 as it might be necessary to run this again from time to time in case code review misses this.

[nils-ohlmeier]

I think with all the improvements landed this has reached now decent stability level that we can close this.