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

Issue 742859 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

WebRTC tests relying on timers running fast in background tabs

Project Member Reported by magjed@chromium.org, Jul 14 2017

Issue description

After enabling timer throttling for background renderers (https://chromium-review.googlesource.com/c/567932/), gathering of ICE candidates started failing flakingly with this error log:

[7466:7466:0713/070330.078566:INFO:CONSOLE(13)] "Returning Test failed: Error: Received invalid ICE candidate list from peer: []
    at failTest (http://127.0.0.1:52364/webrtc/test_functions.js:46:15)
    at receiveIceCandidates (http://127.0.0.1:52364/webrtc/peerconnection.js:371:11)
    at <anonymous>:1:51 to test.", source: http://127.0.0.1:52364/webrtc/test_functions.js (13)
../../chrome/browser/media/webrtc/webrtc_browsertest_base.cc:412: Failure
      Expected: "ok-received-candidates"
To be equal to: ExecuteJavascript( base::StringPrintf("receiveIceCandidates('%s')", ice_candidates.c_str()), to_tab)
      Which is: "Test failed: Error: Received invalid ICE candidate list from peer: []\n    at failTest (http://127.0.0.1:52364/webrtc/test_functions.js:46:15)\n    at receiveIceCandidates (http://127.0.0.1:52364/webrtc/peerconnection.js:371:11)\n    at <anonymous>:1:51"
With diff:
@@ -1,1 +1,4 @@
-ok-received-candidates
+Test failed: Error: Received invalid ICE candidate list from peer: []
+    at failTest (http://127.0.0.1:52364/webrtc/test_functions.js:46:15)
+    at receiveIceCandidates (http://127.0.0.1:52364/webrtc/peerconnection.js:371:11)
+    at <anonymous>:1:51

Link to bots:
https://build.chromium.org/p/chromium.webrtc/builders/Linux%20Tester/builds/30651
https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/69890
https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester/builds/36437

The ICE candidate gathering probably fails from a timeout.

The WebRTC tests are currently blocking https://chromium-review.googlesource.com/c/567932/. We should update the WebRTC tests so we can land this desired behavior.
 
Components: Blink>Scheduling
Status: Started (was: Assigned)
Components: -Blink>WebRTC Blink>WebRTC>PeerConnection
Owner: deadbeef@chromium.org
I believe I understand the issue. The root cause is that "RTCPeerConnection.iceGatheringState" isn't changed atomically in the same task in which "icegatheringstatechange" is fired. So the application may see "the gathering state is complete" before it's seen the "icegatheringstatechange" or even "icecandidate" events.

The "background timer throttling" revealed this issue because RTCPeerConnection uses an AsyncMethodRunner to schedule the "icecandidate" events, which internally uses Timer, which uses TimerTaskRunner. So, normally the events will be dispatched effectively immediately, and this happens:

1. "gathering" state change seen from webrtc; cached state set to "gathering", event queued.
2. Event fired. Application reads "gathering".
3. "complete" state change seen from webrtc; cached state set to "complete", event queued.
4. Event fired. Application reads "complete".

But with the throttling, this happens:

1. "gathering" state change seen from webrtc; cached state set to "gathering", event queued.
2. "complete" state change seen from webrtc; cached state set to "complete", event queued.
3. Event fired. Application reads "complete".
4. Event fired. Application reads "complete" again.

Should be simple to fix. It looks like this issue already was noticed and fixed for iceConnectionState, just not for iceGatheringState...
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 19 2017

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

commit 129ff07e68b7505ce6ccdd5511cead4ae83d5d3c
Author: Taylor Brandstetter <deadbeef@chromium.org>
Date: Wed Jul 19 23:30:01 2017

Update gathering state at the same time as the event is fired.

This CL ensures that it won't be possible for
"RTCPeerConnection.iceGatheringState" to return a new value until the
"icegatheringstatechange" event has been dispatched. This is something
the WebRTC specification guarantees.

This was previously fixed for iceConnectionState:
https://codereview.chromium.org/1523213002

This CL extends that logic to iceGatheringState.

This CL also ensures that the special "null" candidate (used
as another indication that gathering is complete) is emitted
at the proper time. To do this, the responsibility for creating this
special candidate is moved from the content layer to the Blink layer.

Lastly, this CL updates the "setConfiguration" test, resolving a TODO.

Bug:  742859 
Change-Id: I55fb605913314ff5e780fa3ca30112180bdc7f37
Reviewed-on: https://chromium-review.googlesource.com/575849
Commit-Queue: Taylor Brandstetter <deadbeef@chromium.org>
Reviewed-by: Tommi <tommi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488038}
[modify] https://crrev.com/129ff07e68b7505ce6ccdd5511cead4ae83d5d3c/content/renderer/media/mock_web_rtc_peer_connection_handler_client.cc
[modify] https://crrev.com/129ff07e68b7505ce6ccdd5511cead4ae83d5d3c/content/renderer/media/rtc_peer_connection_handler.cc
[modify] https://crrev.com/129ff07e68b7505ce6ccdd5511cead4ae83d5d3c/content/test/data/media/peerconnection-setConfiguration.html
[modify] https://crrev.com/129ff07e68b7505ce6ccdd5511cead4ae83d5d3c/content/test/data/media/webrtc_test_common.js
[modify] https://crrev.com/129ff07e68b7505ce6ccdd5511cead4ae83d5d3c/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
[modify] https://crrev.com/129ff07e68b7505ce6ccdd5511cead4ae83d5d3c/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h

Cc: altimin@chromium.org
Status: Fixed (was: Started)
Alright altimin@, I believe that should fix the WebRTC issue.
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 31 2017

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

commit 1ff0bb6199125c14dcf548f7f4b4ef597e73de50
Author: Alexander Timin <altimin@chromium.org>
Date: Thu Aug 31 13:24:47 2017

[blink] Use TaskRunnerTimer in media element event queues.

MediaElements used to be the only user of GenericEventQueue which
posted tasks to renderer-global default timer queue.

Rename GenericEventQueue to MediaElementEventQueue and make this 
event queue post tasks with kMediaElementEvent type to a relevant
per-frame queue.

R=haraken@chromium.org

BUG= 742859 

Change-Id: I0de9721a92b4848b88791d09c4ac04af2aeb95ab
Reviewed-on: https://chromium-review.googlesource.com/574713
Commit-Queue: Alexander Timin <altimin@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#498843}
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/core/dom/BUILD.gn
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp
[rename] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/core/dom/events/MediaElementEventQueue.cpp
[rename] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/core/dom/events/MediaElementEventQueue.h
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/core/html/HTMLMediaElement.h
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/core/html/track/TextTrackList.cpp
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/core/html/track/TextTrackList.h
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.cpp
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/modules/encryptedmedia/MediaKeySession.h
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/modules/mediasource/MediaSource.cpp
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/modules/mediasource/MediaSource.h
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/modules/mediasource/SourceBuffer.h
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/modules/mediasource/SourceBufferList.cpp
[modify] https://crrev.com/1ff0bb6199125c14dcf548f7f4b4ef597e73de50/third_party/WebKit/Source/modules/mediasource/SourceBufferList.h

Sign in to add a comment