New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 840737 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

Convert webrtc_add_stream_no_deadlock_browsertest to a layout test

Project Member Reported by kerl@google.com, May 8 2018

Issue description

The browser test webrtc_add_stream_no_deadlock_browsertest.cc added in https://crbug.com/818584 is more suited to be a layout test. That would remove much of the current boiler plate.
 

Comment 1 by hbos@chromium.org, May 8 2018

Thanks for showing an interest in this.

Consider adding it as a WPT to https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setRemoteDescription-tracks.https.html.

For WPT, don't rely on Chrome-only APIs.
- Use addTrack instead of addStream. addStream is not in the spec and is implemented on top of addTrack, so calling addTrack directly should trigger the same problem.
- Remove the parameter of createOffer ({offerToReceiveAudio: true, offerToReceiveVideo: true})

(The former, addStream, is not in the spec anymore. The latter is actually in the spec, https://w3c.github.io/webrtc-pc/#legacy-configuration-extensions, but under legacy section. But it is also irrelevant to the bug.)

Comment 2 by hbos@chromium.org, May 8 2018

That particular file I referenced uses async_test(). Prefer promise_test() so you don't have to explicitly call t.done().

Also prefer using async and await when possible, though in this case it is important that you do not await the SRD when calling addStream() and createOffer().

Comment 3 by hbos@chromium.org, May 8 2018

ninja -C out/Debug -j 2000 blink_tests
out/Debug/content_shell --run-layout-test external/wpt/webrtc/RTCPeerConnection-setRemoteDescription-tracks.https.html

See also external/wpt/webrtc/RTCPeerConnection-setRemoteDescription-tracks.https-expected.txt.

Comment 4 by kerl@google.com, May 14 2018

For reference:
The parent commit of the fix is 2485344d4e8792bc4f9716deadda41fbc0d89ad9. If running the layout tests against this commit, the layout test script is in another location:
blink/tools/run_layout_tests.sh
Project Member

Comment 5 by bugdroid1@chromium.org, May 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd3afeb02abdef655457ada0c1f87b58cb06f805

commit bd3afeb02abdef655457ada0c1f87b58cb06f805
Author: Kristoffer Erlandsson <kerl@google.com>
Date: Wed May 16 09:39:20 2018

Add test to verify a particular PeerConnection setup does not deadlock

Does the connection setup in a specific sequence that historically
triggered a deadlock ( https://crbug.com/736725 ). The test simply
verifies this specific sequence completes without errors or timeout.

Verified it fails on the deadlock by running the test on the parent commit
(2485344d4e8792bc4f9716deadda41fbc0d89ad9) of the original bug fix.

BUG= chromium:840737 

Change-Id: I93d9d8cb50cad9629268971520de489450d8649b
Reviewed-on: https://chromium-review.googlesource.com/1057249
Reviewed-by: Henrik Boström <hbos@chromium.org>
Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Commit-Queue: Kristoffer Erlandsson <kerl@google.com>
Cr-Commit-Position: refs/heads/master@{#559022}
[add] https://crrev.com/bd3afeb02abdef655457ada0c1f87b58cb06f805/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-add-track-no-deadlock.https.html

Comment 6 by hbos@chromium.org, May 16 2018

Shall we delete the fast/peerconnection/ version while we're at it?

Comment 7 by kerl@google.com, May 16 2018

WDYM fast/peerconnection/?

I just uploaded https://crrev.com/c/1061174 removing the browser test variant that is not needed now, if that is what you mean?

Comment 8 by hbos@chromium.org, May 16 2018

Yes that's it, I meant browser test not fast/peerconnection/.

Comment 9 by dtosic@google.com, May 16 2018

Cc: liwwei@google.com saeedj@chromium.org
Project Member

Comment 10 by bugdroid1@chromium.org, May 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b93b64e20199275c0a6a73c927374ad74c41d20e

commit b93b64e20199275c0a6a73c927374ad74c41d20e
Author: Kristoffer Erlandsson <kerl@google.com>
Date: Wed May 16 11:27:09 2018

Remove browser test that was converted to layout test.

This test was converted to a layout test in https://crrev.com/c/1057249.

BUG= chromium:840737 

Change-Id: Iebed1ea6f8707928e3b5cfcb2fff5e21bb8c159b
Reviewed-on: https://chromium-review.googlesource.com/1061174
Reviewed-by: Henrik Boström <hbos@chromium.org>
Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Commit-Queue: Kristoffer Erlandsson <kerl@google.com>
Cr-Commit-Position: refs/heads/master@{#559039}
[delete] https://crrev.com/f83a17e827e8dc5b18c2f191df8067a18c2f6949/content/browser/webrtc/webrtc_add_stream_no_deadlock_browsertest.cc
[modify] https://crrev.com/b93b64e20199275c0a6a73c927374ad74c41d20e/content/test/BUILD.gn
[delete] https://crrev.com/f83a17e827e8dc5b18c2f191df8067a18c2f6949/content/test/data/media/peerconnection-add-stream.html

Comment 11 by kerl@google.com, May 16 2018

Status: Fixed (was: Assigned)

Comment 12 by hbos@chromium.org, May 16 2018

Status: Verified (was: Fixed)

Sign in to add a comment