From 2fda5e9751d14a520a4d1d327a5d8e10dd67b097 Mon Sep 17 00:00:00 2001
From: Jan-Ivar Bruaroey <jib@mozilla.com>
Date: Fri, 28 Feb 2020 21:56:52 +0000
Subject: [PATCH] Bug 1600824 - Fix operations queue to run 1st operation
 synchronously. r=bwc

Differential Revision: https://phabricator.services.mozilla.com/D55585

--HG--
extra : moz-landing-system : lando
---
 dom/media/PeerConnection.jsm                  |  29 ++-
 ...TCPeerConnection-operations.https.html.ini |  22 ++
 .../RTCPeerConnection-operations.https.html   | 211 ++++++++++++++++++
 3 files changed, 251 insertions(+), 11 deletions(-)
 create mode 100644 testing/web-platform/meta/webrtc/RTCPeerConnection-operations.https.html.ini
 create mode 100644 testing/web-platform/tests/webrtc/RTCPeerConnection-operations.https.html

diff --git a/dom/media/PeerConnection.jsm b/dom/media/PeerConnection.jsm
index d47cbfcdab56..8691a1dfa0ca 100644
--- a/dom/media/PeerConnection.jsm
+++ b/dom/media/PeerConnection.jsm
@@ -519,7 +519,7 @@ class RTCPeerConnection {
     this.makeGetterSetterEH("onidpvalidationerror");
 
     this._pc = new this._win.PeerConnectionImpl();
-    this._operationsChain = this._win.Promise.resolve();
+    this._operations = 0;
 
     this.__DOM_IMPL__._innerObject = this;
     const observer = new this._win.PeerConnectionObserver(this.__DOM_IMPL__);
@@ -586,20 +586,20 @@ class RTCPeerConnection {
   // Add a function to the internal operations chain.
 
   async _chain(func) {
-    let p = (async () => {
-      await this._operationsChain;
-      // Don't _checkClosed() inside the chain, because it throws, and spec
-      // behavior is to NOT reject outstanding promises on close. This is what
-      // happens most of the time anyways, as the c++ code stops calling us once
-      // closed, hanging the chain. However, c++ may already have queued tasks
-      // on us, so if we're one of those then sit back.
+    const p = (async () => {
+      if (this._operations++) {
+        await this._operationsChain;
+      }
       if (this._closed) {
         return null;
       }
       return func();
     })();
     // don't propagate errors in the operations chain (this is a fork of p).
-    this._operationsChain = p.catch(() => {});
+    this._operationsChain = p.then(
+      () => this._operations--,
+      () => this._operations--
+    );
     return p;
   }
 
@@ -1085,12 +1085,18 @@ class RTCPeerConnection {
     this._sanityCheckSdp(action, type, sdp);
 
     return this._chain(async () => {
-      await this._getPermission();
+      // Avoid Promise.all ahead of synchronous part of spec algorithm, since it
+      // defers. NOTE: The spec says to return an already-rejected promise in
+      // some cases, which is difficult to achieve in practice from JS (would
+      // require avoiding await and then() entirely), but we want to come as
+      // close as we reasonably can.
+      const p = this._getPermission();
       await new Promise((resolve, reject) => {
         this._onSetDescriptionSuccess = resolve;
         this._onSetDescriptionFailure = reject;
         this._impl.setLocalDescription(action, sdp);
       });
+      await p;
       this._negotiationNeeded = false;
       if (type == "answer") {
         if (this._localUfragsToReplace.size > 0) {
@@ -1178,7 +1184,6 @@ class RTCPeerConnection {
 
     return this._chain(async () => {
       const haveSetRemote = (async () => {
-        await this._getPermission();
         if (type == "offer" && this.signalingState == "have-local-offer") {
           await new Promise((resolve, reject) => {
             this._onSetDescriptionSuccess = resolve;
@@ -1193,11 +1198,13 @@ class RTCPeerConnection {
           this._transceivers = this._transceivers.filter(t => !t.shouldRemove);
           this._updateCanTrickle();
         }
+        const p = this._getPermission();
         await new Promise((resolve, reject) => {
           this._onSetDescriptionSuccess = resolve;
           this._onSetDescriptionFailure = reject;
           this._impl.setRemoteDescription(action, sdp);
         });
+        await p;
         this._processTrackAdditionsAndRemovals();
         this._fireLegacyAddStreamEvents();
         this._transceivers = this._transceivers.filter(t => !t.shouldRemove);
diff --git a/testing/web-platform/meta/webrtc/RTCPeerConnection-operations.https.html.ini b/testing/web-platform/meta/webrtc/RTCPeerConnection-operations.https.html.ini
new file mode 100644
index 000000000000..5ba5934415e5
--- /dev/null
+++ b/testing/web-platform/meta/webrtc/RTCPeerConnection-operations.https.html.ini
@@ -0,0 +1,22 @@
+[RTCPeerConnection-operations.https.html]
+  [SLD(offer) must validate against LastCreatedOffer early (prerequisite)]
+    expected: FAIL
+
+  [setLocalDescription uses operations chain]
+    expected: FAIL
+
+  [addIceCandidate uses operations chain]
+    expected: FAIL
+
+  [setRemoteDescription uses operations chain]
+    expected: FAIL
+
+  [SLD(offer) must validate input before signaling state (prerequisite)]
+    expected: FAIL
+
+  [createAnswer uses operations chain]
+    expected: FAIL
+
+  [createOffer uses operations chain]
+    expected: FAIL
+
diff --git a/testing/web-platform/tests/webrtc/RTCPeerConnection-operations.https.html b/testing/web-platform/tests/webrtc/RTCPeerConnection-operations.https.html
new file mode 100644
index 000000000000..e88c8b28172c
--- /dev/null
+++ b/testing/web-platform/tests/webrtc/RTCPeerConnection-operations.https.html
@@ -0,0 +1,211 @@
+<!doctype html>
+<meta charset=utf-8>
+<title></title>
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<script>
+'use strict';
+
+promise_test(async t => {
+  const pc1 = new RTCPeerConnection();
+  t.add_cleanup(() => pc1.close());
+  const pc2 = new RTCPeerConnection();
+  t.add_cleanup(() => pc2.close());
+
+  pc1.addTransceiver("video");
+  const offer = await pc1.createOffer();
+  await pc1.setLocalDescription(offer);
+  const {candidate} = await new Promise(r => pc1.onicecandidate = r);
+  try {
+    await pc2.addIceCandidate(candidate);
+    assert_unreached("Control. Must not succeed");
+  } catch (e) {
+    assert_equals(e.name, "InvalidStateError");
+  }
+  const p = pc2.setRemoteDescription(offer);
+  await pc2.addIceCandidate(candidate);
+  await p;
+}, "addIceCandidate chains onto SRD, fails before");
+
+promise_test(async t => {
+  const pc1 = new RTCPeerConnection();
+  t.add_cleanup(() => pc1.close());
+  const pc2 = new RTCPeerConnection();
+  t.add_cleanup(() => pc2.close());
+
+  pc1.addTransceiver("video");
+  await Promise.all([
+    pc1.createOffer(),
+    pc1.setLocalDescription({type: "offer"})
+  ]);
+  await Promise.all([
+    pc2.setRemoteDescription(pc1.localDescription),
+    pc2.createAnswer(),
+    pc2.setLocalDescription({type: "answer"})
+  ]);
+  await pc1.setRemoteDescription(pc2.localDescription);
+}, "Pack operations queue with implicit offer and answer");
+
+promise_test(async t => {
+  const pc1 = new RTCPeerConnection();
+  t.add_cleanup(() => pc1.close());
+  const pc2 = new RTCPeerConnection();
+  t.add_cleanup(() => pc2.close());
+
+  const state = (pc, s) => new Promise(r => pc.onsignalingstatechange =
+                                       () => pc.signalingState == s && r());
+  pc1.addTransceiver("video");
+  pc1.createOffer();
+  pc1.setLocalDescription({type: "offer"});
+  await state(pc1, "have-local-offer");
+  pc2.setRemoteDescription(pc1.localDescription);
+  pc2.createAnswer();
+  pc2.setLocalDescription({type: "answer"});
+  await state(pc2, "stable");
+  await pc1.setRemoteDescription(pc2.localDescription);
+}, "Negotiate solely by operations queue and signaling state");
+
+// Helpers to test APIs "return a promise rejected with a newly created" error.
+// Strictly speaking this means already-rejected upon return.
+function promiseState(p) {
+  const t = {};
+  return Promise.race([p, t])
+    .then(v => (v === t)? "pending" : "fulfilled", () => "rejected");
+}
+
+// However, to allow promises to be used in implementations, this helper adds
+// some slack: returning a pending promise will pass, provided it is rejected
+// before the end of the current run of the event loop (i.e. on microtask queue
+// before next task).
+async function promiseStateFinal(p) {
+  for (let i = 0; i < 20; i++) {
+    await promiseState(p);
+  }
+  return promiseState(p);
+}
+
+promise_test(async t => {
+  assert_equals(await promiseState(Promise.resolve()), "fulfilled");
+  assert_equals(await promiseState(Promise.reject()), "rejected");
+  assert_equals(await promiseState(new Promise(() => {})), "pending");
+}, "promiseState helper works");
+
+promise_test(async t => {
+  const pc = new RTCPeerConnection();
+  t.add_cleanup(() => pc.close());
+  const p = pc.setLocalDescription({type: "offer", sdp: "blah"});
+  const haveState = promiseStateFinal(p);
+  try {
+    await p;
+    assert_unreached("Control. Must not succeed");
+  } catch (e) {
+    assert_equals(e.name, "InvalidModificationError");
+  }
+  assert_equals(await haveState, "rejected", "promise rejected on same task");
+}, "Operations chain must run first operation's sync part synchronously");
+
+// Helper builds on above test to check if operations queue is empty or not.
+async function isOperationsChainEmpty(pc) {
+  const type = pc.signalingState == "have-remote-offer" ? "answer" : "offer";
+  const p = pc.setLocalDescription({type, sdp: "blah"});
+  const state = await promiseStateFinal(p);
+  try {
+    await p;
+    assert_unreached("Control. Must not succeed");
+  } catch (e) {
+    assert_equals(e.name, "InvalidModificationError", "isOperationsChainEmpty");
+  }
+  return state == "rejected";
+}
+
+promise_test(async t => {
+  const pc = new RTCPeerConnection();
+  t.add_cleanup(() => pc.close());
+  assert_true(await isOperationsChainEmpty(pc), "Empty to start");
+}, "isOperationsChainEmpty detects empty");
+
+promise_test(async t => {
+  const pc = new RTCPeerConnection();
+  t.add_cleanup(() => pc.close());
+  pc.createDataChannel("dummy");
+  const p = await pc.createOffer();
+  try {
+    await pc.setLocalDescription({type: "offer", sdp: "blah"});
+    assert_unreached("Control. Must not succeed");
+  } catch (e) {
+    assert_equals(e.name, "InvalidModificationError");
+  }
+}, "SLD(offer) must validate against LastCreatedOffer early (prerequisite)");
+
+promise_test(async t => {
+  const pc = new RTCPeerConnection();
+  t.add_cleanup(() => pc.close());
+  pc.createDataChannel("dummy");
+  await pc.setRemoteDescription(await pc.createOffer());
+  try {
+    await pc.setLocalDescription({type: "offer", sdp: "blah"});
+    assert_unreached("Control. Must not succeed");
+  } catch (e) {
+    assert_equals(e.name, "InvalidModificationError");
+  }
+}, "SLD(offer) must validate input before signaling state (prerequisite)");
+
+promise_test(async t => {
+  const pc = new RTCPeerConnection();
+  t.add_cleanup(() => pc.close());
+  pc.createDataChannel("dummy");
+  const p = pc.createOffer();
+  assert_true(!await isOperationsChainEmpty(pc), "Non-empty queue");
+  await p;
+}, "createOffer uses operations chain");
+
+promise_test(async t => {
+  const pc = new RTCPeerConnection();
+  t.add_cleanup(() => pc.close());
+  pc.createDataChannel("dummy");
+  await pc.setRemoteDescription(await pc.createOffer());
+  const p = pc.createAnswer();
+  assert_true(!await isOperationsChainEmpty(pc), "Non-empty queue");
+  await p;
+}, "createAnswer uses operations chain");
+
+promise_test(async t => {
+  const pc = new RTCPeerConnection();
+  t.add_cleanup(() => pc.close());
+  pc.createDataChannel("dummy");
+
+  const offer = await pc.createOffer();
+  const p = pc.setLocalDescription(offer);
+  assert_true(!await isOperationsChainEmpty(pc), "Non-empty queue");
+  await p;
+}, "setLocalDescription uses operations chain");
+
+promise_test(async t => {
+  const pc = new RTCPeerConnection();
+  t.add_cleanup(() => pc.close());
+  pc.createDataChannel("dummy");
+
+  const offer = await pc.createOffer();
+  assert_true(await isOperationsChainEmpty(pc), "Empty after settled");
+  const p = pc.setRemoteDescription(offer);
+  assert_true(!await isOperationsChainEmpty(pc), "Non-empty queue");
+  await p;
+}, "setRemoteDescription uses operations chain");
+
+promise_test(async t => {
+  const pc1 = new RTCPeerConnection();
+  t.add_cleanup(() => pc1.close());
+  const pc2 = new RTCPeerConnection();
+  t.add_cleanup(() => pc2.close());
+
+  pc1.addTransceiver("video");
+  const offer = await pc1.createOffer();
+  await pc1.setLocalDescription(offer);
+  const {candidate} = await new Promise(r => pc1.onicecandidate = r);
+  await pc2.setRemoteDescription(offer);
+  const p = pc2.addIceCandidate(candidate);
+  assert_true(!await isOperationsChainEmpty(pc2), "Non-empty queue");
+  await p;
+}, "addIceCandidate uses operations chain");
+
+</script>