Closed Bug 1568169 Opened 5 years ago Closed 5 years ago

Unrelease MediaStream after failed getUserMedia request

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

Desktop
All
defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- verified
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- verified
firefox71 --- verified

People

(Reporter: thomasbelin4, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

[tested with a linux machine]

  • disable webcam driver from Linux machine (https://askubuntu.com/a/166819)
  • open a new tab in Firefox
  • open the console
  • run the following code in the console
navigator.mediaDevices
  .getUserMedia({audio: true, video: true})
  .catch(() => {
    navigator.mediaDevices
      .getUserMedia({audio: true})
      .then(s => setTimeout(() => s.getTracks().forEach(t => t.stop()), 1000));
  });

Actual results:

After 1s the microphone access indicator still blinks in the Firefox tab.

Expected results:

After 1s the stream should be released and Firefox mic indicator should not be shown

Hi @thomasbelin4, I've checked the issue on several machines: Ubuntu 18.04, Windows 10 & Mac OS X using nightly 70.0a1 and beta 69.0b8.

  • on none of this version or machine I've observed unormal behavior - see the small video attached- https://streamable.com/amjmc(the micro icon blinks and remains in that state as long as the call or video takes).
    If you have different scenario or missed some steps please provide it.
    Further, I will set a component. if isn't the right one please fell free to change it.
    Thanks.
Component: Untriaged → WebRTC: Audio/Video
Product: Firefox → Core
Flags: needinfo?(thomasbelin4)

Hi Luviu.

Thanks for your reply. From the video you sent me, it seems you reproduced the bug :)
The mic icon should stop blinking after 1s (which doesn't seem to happen in the video).

Can you confirm that the mic icon doesn't stop blinking after 1s?

Flags: needinfo?(thomasbelin4)

yes, I can confirm that the microphone icon blinks and remains in that state even after 1 s. I will mark this as NEW and if the intended behavior is that this icon should blink than will mark this issue accordingly.
Thanks.

Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → Desktop
Version: 69 Branch → Trunk

If you want to convince yourself that it's a bug, you can try the code that is inside the catch.
It works as expected and release the mic after 1s.

 navigator.mediaDevices
      .getUserMedia({audio: true})
      .then(s => setTimeout(() => s.getTracks().forEach(t => t.stop()), 1000));

:jib could you help me look at this and let me know if you have any further insight?

Rewording the STR so they're more generally applicable:

  • Put the test machine into a state where it has a microphone attached but no webcam.
  • Open Firefox.
  • Open the console.
  • Execute:
navigator.mediaDevices
  .getUserMedia({audio: true, video: true})
  .catch(() => {
    navigator.mediaDevices
      .getUserMedia({audio: true})
      .then(s => setTimeout(() => s.getTracks().forEach(t => t.stop()), 1000));
  });
  • Accept the prompt (thomasbelin4, can you confirm that you're accepting the prompt, of have your Firefox configured to automatically do so?).

What appears to be happening:

  • The first gUM then fails because it can't get a video (no webcam).
  • We hit the gUM in the catch and accept it.
  • We open a stream, but quickly stop all the tracks in it.
  • We clear most of the associated GUI: The menu bar item showing capturing tabs is cleared, as is the Firefox is capturing icon which is always ontop. However, we fail to clear the microphone access blinker icon in the address bar.

As noted in comment 4, this only happens if we perform the gUM call to the mic in the catch. If the code in the catch is run on its own then we clear all indicators correctly.

Flags: needinfo?(jib)
Priority: -- → P3

Thanks for summarizing and clarifying Bryce! This is an accurate description of my problem

I can reproduce it on Mac using https://jsfiddle.net/jib1/0htpdbjv/ - the microphone indicator in the URL bar stays lit forever.

If I remove the video part then the microphone indicator disappears after 3 second the way it's supposed to.

Some kind of timing bug. Unsure if it's in the c++ part or in the JS permission code. Johann, WDYT?

Flags: needinfo?(jib) → needinfo?(jhofmann)

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #7)

If I remove the video part then the microphone indicator disappears after 3 second the way it's supposed to.

Hmm, it's more like 5 seconds then. Almost like the stop() doesn't take. Andreas, is that expected?

Flags: needinfo?(apehrson)

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #8)

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #7)

If I remove the video part then the microphone indicator disappears after 3 second the way it's supposed to.

Hmm, it's more like 5 seconds then. Almost like the stop() doesn't take. Andreas, is that expected?

That's expected because stop() is not called in this case (the original gUM succeeds), so GC eventually stops the track (be it after 3 or 5 or less or more seconds).

With video however, per the original report, we're seeing a bug. I see two ways forward;

  • either we dump the chrome events from MediaManager and judge whether they're sent as expected (I suspect a race here), or
  • we get an rr recording, preferably in pernosco, and inspect the events that way
Flags: needinfo?(apehrson)

Going down the UI rabbit hole a bit I end up here: https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/dom/media/MediaManager.cpp#539

Where removeWindowID isn't called (but the prior recording-device-stopped event seems to be sent).

I haven't actually gone and debugged this further with lldb. This feels quite low-priority to me, so I'm mostly dumping this here in case it helps anyone :)

Flags: needinfo?(jhofmann)

I have a pernosco recording of the steps in bug 1192170 comment 16.

It looks like we're not removing the window ID because there are still 3 inactive SourceListeners here. Looking into why.

It seems like we have asynchronous failure paths throughout getUserMedia that do not clean up the SourceListener that getUserMedia synchronously added, and so the GetUserMediaWindowListener does not get cleaned up either. Normally it gets cleaned up when the last SourceListener goes away.

It reproduces for me all the way back to 60, so I wouldn't call it a regression.

Assignee: nobody → apehrson
Status: NEW → ASSIGNED
Priority: P3 → P2

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
Blocks: 1586097
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f4dd9d524447
Remove SourceListener from window listener in all gUM failure paths. r=jib
Keywords: regression
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Comment on attachment 9098233 [details]
Bug 1568169 - Remove SourceListener from window listener in all gUM failure paths. r?jib

Beta/Release Uplift Approval Request

  • User impact if declined: The media device capture indicator in the address bar, and on background tabs, can stay and indicate a live capture, although all tracks have been stopped and no capturing is taking place.
    Note that it doesn't mean that another request for the same device automatically succeeds, per my testing.
    The only ways to get rid of the live capture indicator are: 1) navigating the tab somewhere, 2) closing the tab
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See bug 1192170 comment 16 for the simplest steps.
    One could also use a variant on the steps in the description of this bug and run the following code in the Web Console on any https site. Accept the microphone permission prompt when it appears. The microphone indicator should go away 1 second after being displayed. With the bug it doesn't.
navigator.mediaDevices
  .getUserMedia({audio: true, video: {width: {min: 1e9}}})
  .catch(() => {
    navigator.mediaDevices
      .getUserMedia({audio: true})
      .then(s => setTimeout(() => s.getTracks().forEach(t => t.stop()), 1000));
  });
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Trivial change
  • String changes made/needed:

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This bug is easy to trigger and leads to very confusing UX.
  • User impact if declined: The media device capture indicator in the address bar, and on background tabs, can stay and indicate a live capture, although all tracks have been stopped and no capturing is taking place.
    Note that it doesn't mean that another request for the same device automatically succeeds, per my testing.
    The only ways to get rid of the live capture indicator are: 1) navigating the tab somewhere, 2) closing the tab
  • Fix Landed on Version: 71
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Trivial change
  • String or UUID changes made by this patch:
Attachment #9098233 - Flags: approval-mozilla-esr68?
Attachment #9098233 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9098233 [details]
Bug 1568169 - Remove SourceListener from window listener in all gUM failure paths. r?jib

Fix for incorrect mic indicator. OK for beta 13 uplift.

Attachment #9098233 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

I've checked the issue in Windows 10 & Ubuntu 18.04 machines (using nightly 71.0a1 from 7th of October)- confirm that is fixed on this version.

Comment on attachment 9098233 [details]
Bug 1568169 - Remove SourceListener from window listener in all gUM failure paths. r?jib

Fixes confusing UX with gUM indicator. Approved for 68.2esr.

Attachment #9098233 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

I've checked again the issue on latest beta 70.0b13 & nightly 71.0a1 from 10/10/2019 using Windows 10 and Ubuntu 18.04 - by executing the steps provided on description. The issue won't occur on both versions.

I've checked again the issue on latest ESR build 68.2.0 - using Windows 10 and Ubuntu 18.04.

QA Whiteboard: [qa-triaged]
QA Whiteboard: [qa-triaged]
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.