Backed out 4 changesets (bug 1654430) for mda failure on test_ondevicechange.html . CLOSED TREE

Backed out changeset 10a70b4fad32 (bug 1654430) Backed out changeset 495576ebc999 (bug 1654430) Backed out changeset 0f1db7f155cb (bug 1654430) Backed out changeset 788a6c7c52f1 (bug 1654430)
2020-09-23 16:52:16 +03:00 · 2020-09-23 16:52:16 +03:00 · a2936f5009
--- a/dom/media/MediaManager.cpp
+++ b/dom/media/MediaManager.cpp
@ -2217,60 +2217,32 @@ void MediaManager::DeviceListChanged() {
  }
  mDeviceListChangeEvent.Notify();

-  // Wait 200 ms, because
-  // A) on some Windows machines, if we call EnumerateRawDevices immediately
-  //    after receiving devicechange event, we'd get an outdated devices list.
-  // B) Waiting helps coalesce multiple calls on us into one, which can happen
-  //    if a device with both audio input and output is attached or removed.
-  //    We want to react & fire a devicechange event only once in that case.
-
-  if (mDeviceChangeTimer) {
-    mDeviceChangeTimer->Cancel();
-  } else {
-    mDeviceChangeTimer = MakeRefPtr<MediaTimer>();
-  }
-  RefPtr<MediaManager> self = this;
+  // On some Windows machines, if we call EnumerateRawDevices immediately after
+  // receiving devicechange event, we would get an outdated devices list.
+  PR_Sleep(PR_MillisecondsToInterval(200));
  auto devices = MakeRefPtr<MediaDeviceSetRefCnt>();
-  mDeviceChangeTimer->WaitFor(TimeDuration::FromMilliseconds(200), __func__)
+  EnumerateRawDevices(0, MediaSourceEnum::Camera, MediaSourceEnum::Microphone,
+                      MediaSinkEnum::Speaker, DeviceEnumerationType::Normal,
+                      DeviceEnumerationType::Normal, false, devices)
      ->Then(
          GetCurrentSerialEventTarget(), __func__,
-          [self, this, devices] {
-            if (!MediaManager::GetIfExists()) {
-              return MgrPromise::CreateAndReject(
-                  MakeRefPtr<MediaMgrError>(MediaMgrError::Name::AbortError,
-                                            u"In shutdown"_ns),
-                  __func__);
-            }
-            return EnumerateRawDevices(
-                0, MediaSourceEnum::Camera, MediaSourceEnum::Microphone,
-                MediaSinkEnum::Speaker, DeviceEnumerationType::Normal,
-                DeviceEnumerationType::Normal, false, devices);
-          },
-          []() {
-            // Timer was canceled by us, or we're in shutdown.
-            return MgrPromise::CreateAndReject(
-                MakeRefPtr<MediaMgrError>(MediaMgrError::Name::AbortError),
-                __func__);
-          })
-      ->Then(
-          GetCurrentSerialEventTarget(), __func__,
-          [self, this, devices](bool) {
+          [self = RefPtr<MediaManager>(this), this, devices](bool) {
            if (!MediaManager::GetIfExists()) {
              return;
            }

-            MediaManager::DeviceIdSet deviceIDs;
+            nsTArray<nsString> deviceIDs;
            for (auto& device : *devices) {
              nsString id;
              device->GetId(id);
-              MOZ_ALWAYS_TRUE(deviceIDs.put(std::move(id)));
+              if (!deviceIDs.Contains(id)) {
+                deviceIDs.AppendElement(id);
+              }
            }
-            // For any real removed cameras, microphones or speakers, notify
-            // their listeners cleanly that the source has stopped, so JS knows
-            // and usage indicators update.
-            for (auto iter = mDeviceIDs.iter(); !iter.done(); iter.next()) {
-              const auto& id = iter.get();
-              if (deviceIDs.has(id)) {
+            // For any removed devices, notify their listeners cleanly that the
+            // source has stopped, so JS knows and usage indicators update.
+            for (auto& id : mDeviceIDs) {
+              if (deviceIDs.Contains(id)) {
                // Device has not been removed
                continue;
              }
@ -3131,8 +3103,7 @@ RefPtr<MediaManager::MgrPromise> MediaManager::EnumerateDevicesImpl(
          })
      ->Then(
          GetMainThreadSerialEventTarget(), __func__,
-          [aWindowId, originKey, aVideoInputEnumType, aAudioInputEnumType,
-           aOutDevices](bool) {
+          [aWindowId, originKey, aOutDevices](bool) {
            // Only run if window is still on our active list.
            MediaManager* mgr = MediaManager::GetIfExists();
            if (!mgr || !mgr->IsWindowStillActive(aWindowId)) {
@ -3141,17 +3112,12 @@ RefPtr<MediaManager::MgrPromise> MediaManager::EnumerateDevicesImpl(
                  __func__);
            }

+            mgr->mDeviceIDs.Clear();
            for (auto& device : *aOutDevices) {
-              if (device->mKind == MediaDeviceKind::Audiooutput ||
-                  (device->mKind == MediaDeviceKind::Audioinput &&
-                   aAudioInputEnumType != DeviceEnumerationType::Fake &&
-                   device->GetMediaSource() == MediaSourceEnum::Microphone) ||
-                  (device->mKind == MediaDeviceKind::Videoinput &&
-                   aVideoInputEnumType != DeviceEnumerationType::Fake &&
-                   device->GetMediaSource() == MediaSourceEnum::Camera)) {
-                nsString id;
-                device->GetId(id);
-                MOZ_ALWAYS_TRUE(mgr->mDeviceIDs.put(std::move(id)));
+              nsString id;
+              device->GetId(id);
+              if (!mgr->mDeviceIDs.Contains(id)) {
+                mgr->mDeviceIDs.AppendElement(id);
              }
            }

@ -3652,12 +3618,6 @@ void MediaManager::Shutdown() {
 #endif
  }

-  if (mDeviceChangeTimer) {
-    mDeviceChangeTimer->Cancel();
-    // Drop ref to MediaTimer early to avoid blocking SharedThreadPool shutdown
-    mDeviceChangeTimer = nullptr;
-  }
-
  {
    // Close off any remaining active windows.

@ -3680,7 +3640,7 @@ void MediaManager::Shutdown() {
  mActiveCallbacks.Clear();
  mCallIds.Clear();
  mPendingGUMRequest.Clear();
-  mDeviceIDs.clear();
+  mDeviceIDs.Clear();
 #ifdef MOZ_WEBRTC
  StopWebRtcLog();
 #endif
--- a/dom/media/MediaManager.h
+++ b/dom/media/MediaManager.h
@ -41,7 +41,6 @@ class nsIPrefBranch;

 namespace mozilla {
 class TaskQueue;
-class MediaTimer;
 namespace dom {
 struct MediaStreamConstraints;
 struct MediaTrackConstraints;
@ -344,7 +343,6 @@ class MediaManager final : public nsIMediaManagerService, public nsIObserver {
  nsRefPtrHashtable<nsStringHashKey, GetUserMediaTask> mActiveCallbacks;
  nsClassHashtable<nsUint64HashKey, nsTArray<nsString>> mCallIds;
  nsTArray<RefPtr<dom::GetUserMediaRequest>> mPendingGUMRequest;
-  RefPtr<MediaTimer> mDeviceChangeTimer;
  bool mCamerasMuted = false;
  bool mMicrophonesMuted = false;

@ -358,21 +356,7 @@ class MediaManager final : public nsIMediaManagerService, public nsIObserver {
  static StaticRefPtr<MediaManager> sSingleton;
  static StaticMutex sSingletonMutex;

-  struct nsStringHasher {
-    using Key = nsString;
-    using Lookup = nsString;
-
-    static HashNumber hash(const Lookup& aLookup) {
-      return HashString(aLookup.get());
-    }
-
-    static bool match(const Key& aKey, const Lookup& aLookup) {
-      return aKey == aLookup;
-    }
-  };
-
-  using DeviceIdSet = HashSet<nsString, nsStringHasher, InfallibleAllocPolicy>;
-  DeviceIdSet mDeviceIDs;
+  nsTArray<nsString> mDeviceIDs;

  // Connect/Disconnect on media thread only
  MediaEventListener mDeviceListChangeListener;
--- a/dom/media/tests/mochitest/head.js
+++ b/dom/media/tests/mochitest/head.js
@ -418,10 +418,6 @@ function pushPrefs(...p) {
  return SpecialPowers.pushPrefEnv({ set: p });
 }

-function popPrefs() {
-  return SpecialPowers.popPrefEnv();
-}
-
 function setupEnvironment() {
  var defaultMochitestPrefs = {
    set: [
--- a/dom/media/tests/mochitest/mochitest.ini
+++ b/dom/media/tests/mochitest/mochitest.ini
@ -59,7 +59,6 @@ skip-if = os != 'linux' # the only platform with real devices
 skip-if = os != 'linux' # the only platform with real devices
 [test_setSinkId_preMutedElement.html]
 [test_ondevicechange.html]
-run-sequentially = sets prefs that may disrupt other tests
 [test_getUserMedia_active_autoplay.html]
 [test_getUserMedia_audioCapture.html]
 skip-if = toolkit == 'android' || (os == "win" && processor == "aarch64") # android(Bug 1189784, timeouts on 4.3 emulator), android(Bug 1264333), aarch64 due to 1538359
--- a/dom/media/tests/mochitest/pc.js
+++ b/dom/media/tests/mochitest/pc.js
@ -1787,88 +1787,91 @@ PeerConnectionWrapper.prototype = {
   * @returns {Promise}
   *        Returns a promise which yields a StatsReport object with RTP stats.
   */
-  async _waitForRtpFlow(target, rtpType) {
-    const { track } = target;
-    info(`_waitForRtpFlow(${track.id}, ${rtpType})`);
-    const packets = `packets${rtpType == "outbound-rtp" ? "Sent" : "Received"}`;
+  async waitForRtpFlow(track) {
+    info("waitForRtpFlow(" + track.id + ")");
+    let hasFlow = (stats, retries) => {
+      const dict = JSON.stringify([...stats.entries()]);
+      info(
+        `Checking for stats in  ${dict} for ${track.kind} track ${track.id}` +
+          `retry number ${retries}`
+      );
+      const rtp = [...stats.values()].find(({ type }) =>
+        ["inbound-rtp", "outbound-rtp"].includes(type)
+      );
+      if (!rtp) {
+        return false;
+      }
+      info("Should have RTP stats for track " + track.id);
+      info("RTP stats: " + JSON.stringify(rtp));
+      let nrPackets =
+        rtp[rtp.type == "outbound-rtp" ? "packetsSent" : "packetsReceived"];
+      info(
+        "Track " +
+          track.id +
+          " has " +
+          nrPackets +
+          " " +
+          rtp.type +
+          " RTP packets."
+      );
+      return nrPackets > 0;
+    };

-    const retryInterval = 500; // Time between stats checks
-    const timeout = 30000; // Timeout in ms
-    const retries = timeout / retryInterval;
-
-    for (let i = 0; i < retries; i++) {
-      info(`Checking ${rtpType} for ${track.kind} track ${track.id} try ${i}`);
-      for (const rtp of (await target.getStats()).values()) {
-        if (rtp.type != rtpType) {
-          continue;
-        }
-        if (rtp.kind != track.kind) {
-          continue;
-        }
-
-        const numPackets = rtp[packets];
-        info(`Track ${track.id} has ${numPackets} ${packets}.`);
-        if (!numPackets) {
-          continue;
-        }
-
-        ok(true, `RTP flowing for ${track.kind} track ${track.id}`);
-        return;
+    // Time between stats checks
+    const retryInterval = 500;
+    // Timeout in ms
+    const timeout = 30000;
+    let retry = 0;
+    // Check hasFlow at a reasonable interval
+    for (let remaining = timeout; remaining >= 0; remaining -= retryInterval) {
+      let stats = await this._pc.getStats(track);
+      if (hasFlow(stats, retry++)) {
+        ok(true, "RTP flowing for " + track.kind + " track " + track.id);
+        return stats;
      }
      await wait(retryInterval);
    }
    throw new Error(
-      `Checking stats for track ${track.id} timed out after ${timeout} ms`
+      "Timeout checking for stats for track " +
+        track.id +
+        " after at least" +
+        timeout +
+        "ms"
    );
  },

-  /**
-   * Wait for inbound RTP packet flow for the given MediaStreamTrack.
-   *
-   * @param {object} receiver
-   *        An RTCRtpReceiver to wait for data flow on.
-   * @returns {Promise}
-   *        Returns a promise that resolves once data is flowing.
-   */
-  async waitForInboundRtpFlow(receiver) {
-    return this._waitForRtpFlow(receiver, "inbound-rtp");
-  },
-
-  /**
-   * Wait for outbound RTP packet flow for the given MediaStreamTrack.
-   *
-   * @param {object} sender
-   *        An RTCRtpSender to wait for data flow on.
-   * @returns {Promise}
-   *        Returns a promise that resolves once data is flowing.
-   */
-  async waitForOutboundRtpFlow(sender) {
-    return this._waitForRtpFlow(sender, "outbound-rtp");
-  },
-
-  getExpectedActiveReceivers() {
+  getExpectedActiveReceiveTracks() {
    return this._pc
      .getTransceivers()
-      .filter(
-        t =>
+      .filter(t => {
+        return (
          !t.stopped &&
          t.currentDirection &&
          t.currentDirection != "inactive" &&
          t.currentDirection != "sendonly"
-      )
-      .filter(({ receiver }) => receiver.track)
-      .map(({ mid, currentDirection, receiver }) => {
-        info(
-          `Found transceiver that should be receiving RTP: mid=${mid}` +
-            ` currentDirection=${currentDirection}` +
-            ` kind=${receiver.track.kind} track-id=${receiver.track.id}`
        );
-        return receiver;
-      });
+      })
+      .map(t => {
+        info(
+          "Found transceiver that should be receiving RTP: mid=" +
+            t.mid +
+            " currentDirection=" +
+            t.currentDirection +
+            " kind=" +
+            t.receiver.track.kind +
+            " track-id=" +
+            t.receiver.track.id
+        );
+        return t.receiver.track;
+      })
+      .filter(t => t);
  },

-  getExpectedSenders() {
-    return this._pc.getSenders().filter(({ track }) => track);
+  getExpectedSendTracks() {
+    return this._pc
+      .getSenders()
+      .map(s => s.track)
+      .filter(t => t);
  },

  /**
@ -1879,21 +1882,24 @@ PeerConnectionWrapper.prototype = {
   *        A promise that resolves when media flows for all elements and tracks
   */
  waitForMediaFlow() {
-    const receivers = this.getExpectedActiveReceivers();
-    return Promise.all([
-      ...this.localMediaElements.map(el => this.waitForMediaElementFlow(el)),
-      ...this.remoteMediaElements
-        .filter(({ srcObject }) =>
-          receivers.some(({ track }) =>
-            srcObject.getTracks().some(t => t == track)
+    return Promise.all(
+      [].concat(
+        this.localMediaElements.map(element =>
+          this.waitForMediaElementFlow(element)
+        ),
+        this.remoteMediaElements
+          .filter(elem =>
+            this.getExpectedActiveReceiveTracks().some(track =>
+              elem.srcObject.getTracks().some(t => t == track)
+            )
          )
-        )
-        .map(el => this.waitForMediaElementFlow(el)),
-      ...receivers.map(receiver => this.waitForInboundRtpFlow(receiver)),
-      ...this.getExpectedSenders().map(sender =>
-        this.waitForOutboundRtpFlow(sender)
-      ),
-    ]);
+          .map(elem => this.waitForMediaElementFlow(elem)),
+        this.getExpectedActiveReceiveTracks().map(track =>
+          this.waitForRtpFlow(track)
+        ),
+        this.getExpectedSendTracks().map(track => this.waitForRtpFlow(track))
+      )
+    );
  },

  async waitForSyncedRtcp() {
--- a/dom/media/tests/mochitest/templates.js
+++ b/dom/media/tests/mochitest/templates.js
@ -153,43 +153,48 @@ function waitForAnIceCandidate(pc) {
  });
 }

-async function checkTrackStats(pc, track, outbound) {
-  const audio = track.kind == "audio";
-  const msg =
-    `${pc} stats ${outbound ? "outbound " : "inbound "}` +
-    `${audio ? "audio" : "video"} rtp track id ${track.id}`;
-  const stats = await pc.getStats(track);
-  ok(
-    pc.hasStat(stats, {
-      type: outbound ? "outbound-rtp" : "inbound-rtp",
-      kind: audio ? "audio" : "video",
-    }),
-    `${msg} - found expected stats`
-  );
-  ok(
-    !pc.hasStat(stats, {
-      type: outbound ? "inbound-rtp" : "outbound-rtp",
-    }),
-    `${msg} - did not find extra stats with wrong direction`
-  );
-  ok(
-    !pc.hasStat(stats, {
-      kind: audio ? "video" : "audio",
-    }),
-    `${msg} - did not find extra stats with wrong media type`
-  );
+function checkTrackStats(pc, track, outbound) {
+  var audio = track.kind == "audio";
+  var msg =
+    pc +
+    " stats " +
+    (outbound ? "outbound " : "inbound ") +
+    (audio ? "audio" : "video") +
+    " rtp track id " +
+    track.id;
+  return pc.getStats(track).then(stats => {
+    ok(
+      pc.hasStat(stats, {
+        type: outbound ? "outbound-rtp" : "inbound-rtp",
+        kind: audio ? "audio" : "video",
+      }),
+      msg + " - found expected stats"
+    );
+    ok(
+      !pc.hasStat(stats, {
+        type: outbound ? "inbound-rtp" : "outbound-rtp",
+      }),
+      msg + " - did not find extra stats with wrong direction"
+    );
+    ok(
+      !pc.hasStat(stats, {
+        kind: audio ? "video" : "audio",
+      }),
+      msg + " - did not find extra stats with wrong media type"
+    );
+  });
 }

-function checkAllTrackStats(pc) {
-  return Promise.all([
-    ...pc
-      .getExpectedActiveReceivers()
-      .map(({ track }) => checkTrackStats(pc, track, false)),
-    ...pc
-      .getExpectedSenders()
-      .map(({ track }) => checkTrackStats(pc, track, true)),
-  ]);
-}
+var checkAllTrackStats = pc => {
+  return Promise.all(
+    [].concat(
+      pc
+        .getExpectedActiveReceiveTracks()
+        .map(track => checkTrackStats(pc, track, false)),
+      pc.getExpectedSendTracks().map(track => checkTrackStats(pc, track, true))
+    )
+  );
+};

 // Commands run once at the beginning of each test, even when performing a
 // renegotiation test.
--- a/dom/media/tests/mochitest/test_ondevicechange.html
+++ b/dom/media/tests/mochitest/test_ondevicechange.html
@ -15,10 +15,23 @@ createHTML({

 const RESPONSE_WAIT_TIME_MS = 3000;

-async function maybeReceiveDevicechangeEvent() {
+function OnDeviceChangeEvent() {
+  return new Promise(resolve => navigator.mediaDevices.ondevicechange = resolve);
+}
+
+function OnDeviceChangeEventReceived() {
  return Promise.race([
-    new Promise(r => navigator.mediaDevices.ondevicechange = () => r(true)),
-    wait(RESPONSE_WAIT_TIME_MS).then(() => false)
+    OnDeviceChangeEvent(),
+    wait(RESPONSE_WAIT_TIME_MS).then(() =>
+      Promise.reject(new Error("Timed out waiting for devicechange"))),
+  ]);
+}
+
+function OnDeviceChangeEventNotReceived() {
+  return Promise.race([
+    OnDeviceChangeEvent().then(() =>
+      Promise.reject(new Error("devicechange fired unexpectedly"))),
+    wait(RESPONSE_WAIT_TIME_MS),
  ]);
 }

@ -28,51 +41,30 @@ runTest(async () => {
  await pushPrefs(
    // Ensure there are continuous fake devicechange events throughout this test
    ["media.ondevicechange.fakeDeviceChangeEvent.enabled", true],
+    // Enforce gUM permission prompts initially
+    ["media.navigator.permission.disabled", false],
    // Make fake devices count as real, permission-wise, or devicechange events
    // won't be exposed
    ["media.navigator.permission.fake", true],
-    // Ensure this precondition to the below tests
-    ["media.navigator.permission.disabled", true]
  );

-  try {
-    {
-      const stream = await getUserMedia({video: true, fake: true});
-      const [track] = stream.getVideoTracks();
-      await pushPrefs(["media.navigator.permission.disabled", false]);
-      try {
-        ok(await maybeReceiveDevicechangeEvent(),
-           "devicechange event is fired when gUM is in use without permanent " +
-           "permission granted");
-      } finally {
-        track.stop();
-        await popPrefs();
-      }
-    }
+  info("assure devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted");
+  await OnDeviceChangeEventNotReceived();
+  ok(true, "devicechange event is NOT fired when gUM is NOT in use and permanent permission is NOT granted");

-    {
-      await pushPrefs(["media.navigator.permission.disabled", false]);
-      try {
-        ok(!await maybeReceiveDevicechangeEvent(),
-           "devicechange event is NOT fired when gUM is NOT in use and " +
-           "permanent permission is NOT granted");
-      } finally {
-        await popPrefs();
-      }
-    }
+  await pushPrefs(['media.navigator.permission.disabled', true]);

-    {
-      ok(await maybeReceiveDevicechangeEvent(),
-         "devicechange event is fired when gUM is NOT in use and permanent "+
-         "permission is granted");
-    }
-  } finally {
-    await popPrefs();
-  }
+  info("assure devicechange event is fired when gUM is NOT in use and permanent permission is granted");
+  await OnDeviceChangeEventReceived();
+  ok(true, "devicechange event is fired when gUM is NOT in use and permanent permission is granted");

-  // Let some time pass to allow fake ended events to finish firing on other
-  // documents before proceeding with subsequent tests to not distrupt them.
-  await maybeReceiveDevicechangeEvent();
+  info("assure devicechange event is fired when gUM is in use");
+  const st = await getUserMedia({video: true});
+  const videoTracks = st.getVideoTracks();
+  await pushPrefs(['media.navigator.permission.disabled', false]);
+  await OnDeviceChangeEventReceived();
+  ok(true, "devicechange event is fired when gUM is in use");
+  videoTracks.forEach(track => track.stop());
 });

 </script>
--- a/dom/media/tests/mochitest/test_peerConnection_addAudioTrackToExistingVideoStream.html
+++ b/dom/media/tests/mochitest/test_peerConnection_addAudioTrackToExistingVideoStream.html
@ -1,4 +1,4 @@
-<!DOCTYPE HTML>
+<!DOCTYPE HTML>
 <html>
 <head>
  <script type="application/javascript" src="pc.js"></script>
@ -12,7 +12,6 @@
  });

  runNetworkTest(function (options) {
-    SimpleTest.requestCompleteLog();
    var test = new PeerConnectionTest(options);
    test.chain.replace("PC_LOCAL_GUM",
      [
--- a/dom/media/webrtc/MediaEngine.h
+++ b/dom/media/webrtc/MediaEngine.h
@ -47,9 +47,7 @@ class MediaEngine {

  virtual void Shutdown() = 0;

-  virtual void SetFakeDeviceChangeEventsEnabled(bool aEnable) {
-    MOZ_DIAGNOSTIC_ASSERT(false, "Fake events may not have started/stopped");
-  }
+  virtual void SetFakeDeviceChangeEventsEnabled(bool aEnable) {}

  virtual MediaEventSource<void>& DeviceListChangeEvent() = 0;