This is blocking WPT exports, making it Pri-1. Anyone cc'd volunteering to take a closer look?

The signaling state is surfaced to Chrome in a PostTask[1][2] that is not coordinated with the PostTask that is used to surface "setLocalDescription completed" so that randomly there may or may not be a JavaScript task execution cycle between the tasks. Also, when the signalingstatechange reaches JavaScript land, it is scheduled to fire in the next cycle[3].

I think the right thing to do is to always coordinate any updated state information in a single callback, so that Chrome can do a single PostTask with all updated information. Otherwise we're at risk of things happening in-between state changes.

This was a problem before for setRemoteDescription in that each stream was surfaced in a separate observer callback - a separate PostTask - so the result of SRD was not an "all or nothing" operation. The fix for that was to process all relevant state changes in the setRemoteDescription callback, and then add remote streams at the same time when we reach the JS thread.

Can the signaling state only change due to user invoked APIs such as setLocalDescription and setRemoteDescription? If so we could just copy the third_party/webrtc signaling state information in the SLD/SRD callback and handle that in the same PostTask as the rest of the operation.

[1] https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/rtc_peer_connection_handler.cc?sq=package:chromium&dr&g=0&l=1184

[2] https://cs.chromium.org/chromium/src/third_party/webrtc/pc/peerconnection.cc?sq=package:chromium&dr&g=0&l=3537

[3] https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc?q=rtc_peer_connection.cc&sq=package:chromium&dr&l=1878

Background information: All third_party/webrtc code executes on a thread separate from JavaScript (renderer main thread). setLocalDescription/setRemoteDescription is async in the spec, but implemented as (more or less) synchronously in third_party/webrtc. Async behavior is achieved in Chrome by not blocking the main thread when invoking SLD/SRD, and sync behavior is achieved in Chrome by blocking the main thread when invoking other operations - but usually they're synchronous in third_party/webrtc.

(This necessarily leads to spec non-conformance in some cases where async and sync operations are mixed, but these are typically non-representative edge cases. I have a bug filed for that somewhere, but that is not blocking this.)

In the SLD/SRD case, the synchronous implementation means that in third_party/webrtc when you call SLD it either happens or not. But surfacing what happened in multiple PostTasks you can get into a state in JavaScript where some of the things have happened and other things are queued to happen but have not been processed yet. If that makes sense.

Ah, I'll take a stab at it!

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

commit c220be7c001dbe68a36afc3e02838c510b0b3b0f
Author: Henrik Boström <hbos@chromium.org>
Date: Thu Jun 07 16:30:41 2018

Fire RTCPeerConnection.onsignalingstatechange in the correct order.

With this change, onsignalingstatechange always fires before the
setLocalDescription's promise resolves.

Before this change, signaling state changes were surfaced with callbacks
firing separately from the callback that setLocalDescription or
setRemoteDescription had completed. As a result, the result of a SLD/SRD
operation was surfaced in two PostTasks instead of one. This could lead
to flaky behavior where JavaScript task execeution cycles may or may not
execute between PostTasks which lead to undefined order. Also when the
signalingstatechange was surfaced to the JS thread the firing of the
event was delayed by a task execution cycle due to
ScheduleDispatchEvent.
- If both PostTasks were handled in the same cycle, the event firing
  would be scheduled and the promise resolved, such that the promise's
  then() is invoked just before the event fires (scheduled after
  promise). This is NOT in accordance with the spec.
- If onsignalingstatechange's PostTask was handled in its own cycle,
  the event would be scheduled such that by the time the
  setLocalDescription's callback's PostTask occurs resolving the
  promise, the event would fire before the promise's then(). This IS
  in accordance with the spec.

With this change, the onsignalingstatechange event is fired without
delay, and it is processed in by the same observer that handles the
SLD/SRD callback, so the order is always guaranteed to be the same.

In addition, this CL removes the usage of
WebRTCPeerConnectionHandlerClient::SignalingState and relies on the
webrtc::PeerConnectionInterface::SignalingState enum directly. This
is part of the Onion Souping work (crbug.com/787254).

Bug:  848768 , 787254
Change-Id: I7bc10439c44c6ff6beab0289975295ff2e051b9c
Reviewed-on: https://chromium-review.googlesource.com/1085302
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565291}
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/content/renderer/media/webrtc/mock_web_rtc_peer_connection_handler_client.h
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/content/renderer/media/webrtc/peer_connection_tracker.cc
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/content/renderer/media/webrtc/peer_connection_tracker.h
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/content/renderer/media/webrtc/rtc_peer_connection_handler.cc
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/content/renderer/media/webrtc/rtc_peer_connection_handler.h
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/content/renderer/media/webrtc/webrtc_set_remote_description_observer.cc
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/content/renderer/media/webrtc/webrtc_set_remote_description_observer.h
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-expected.txt
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription.html
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/third_party/blink/public/platform/web_rtc_peer_connection_handler_client.h
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc
[modify] https://crrev.com/c220be7c001dbe68a36afc3e02838c510b0b3b0f/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.h