Ada [:adalucinet] Reporter
	Description • 10 years ago

Reproducible on:
latest Aurora (BuildID: 20141123004001):
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
latest Nightly (BuildID: 20141124030207):
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0

Not reproducible on:
Firefox 34 beta 11 (BuildID: 20141120192249):
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0

Steps to reproduce:
1. Load: <iframe src="https://people.mozilla.org/~fqueze2/webrtc/"></iframe><iframe src="https://people.mozilla.org/~fqueze2/webrtc/"></iframe> or http://jsfiddle.net/nafbr2d7/
2. In the first iframe, click the "Audio" button and share your microphone.
3. Disconnect your camera and, in the second iframe, click the "Video" button.

Expected results: The microphone icon from URL bar is visible.

Actual results: The microphone icon from URL bar is dismissed.

Note:
1. Also reproducible on Ubuntu 12.04 32-bit.
2. An error message that the device is not found is displayed in the iframe on both Nightly and Aurora. The microphone is still working.
3. Screencast: http://goo.gl/qkh6nH

	:Gijs (he/him)
	Comment 1 • 10 years ago

[Tracking Requested - why for this release]:
Misleading indicator information about what you're sharing with the page

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #0)
> Not reproducible on:
> Firefox 34 beta 11 (BuildID: 20141120192249):
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0

Does the same message about the device not working appear on beta?

	Ada [:adalucinet] Reporter
	Comment 2 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #1)
> [Tracking Requested - why for this release]:
> Misleading indicator information about what you're sharing with the page
> 
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #0)
> > Not reproducible on:
> > Firefox 34 beta 11 (BuildID: 20141120192249):
> > Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
> 
> Does the same message about the device not working appear on beta?

Yes. Beta error message displayed in iframe is the same as in Aurora: 'NO_DEVICES_FOUND'
On Nightly, the error message is 'NotFoundError: The object can not be found here.'
In console I get 'video.mozSrcObject is null' message for all the builds.

Regression range (m-c):
Last good revision: 5bd6e09f074e (2014-09-22)
First bad revision: afc933adf723 (2014-09-23)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5bd6e09f074e&tochange=afc933adf723

Regression range (m-i):
Last good revision: 113ecae356de
First bad revision: afd45d7642b8
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=113ecae356de&tochange=afd45d7642b8 

Maybe caused by Bug 973001 ?

	Sylvestre Ledru [:Sylvestre]
	Comment 3 • 10 years ago

Regression, tracking.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 4 • 10 years ago

Brad/Maire what do you want to do here?  I'm assuming that it's not worthwhile to backout 973001 if that is indeed the cause.  We're down to our last desktop beta today and I'm inclined to wontfix this for 35 though it's unfortunate to ship a new user-facing regression.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 5 • 10 years ago

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #4)
> Brad/Maire what do you want to do here?  I'm assuming that it's not
> worthwhile to backout 973001 if that is indeed the cause.  We're down to our
> last desktop beta today and I'm inclined to wontfix this for 35 though it's
> unfortunate to ship a new user-facing regression.

I suggest we back out bug 973001 from 35 and fix this properly in 36.

	Maire Reavy [:mreavy]
	Comment 6 • 10 years ago

tl;dr -- We can back out bug 973001 from Fx35, but given how unlikely it seems it is for a user to hit this bug and where we are in the release cycle (and how many folks are on PTO), I would be ok with "wontfixing" this for Fx35, especially if the backout and verification got messy (which they might given the size of the patches and what the patches appear to touch). 

I'll work to find someone to fix this bug properly for Fx36.  Florian would be ideal if he's available in early/mid January.  I believe he is mostly on PTO until Jan 5.

Also, I think we should move bugs like this out of "Firefox::General" to either Core::WebRTC-Audio/Video or to a new bugzilla component like "Firefox:camera/microphone permissions" so bugs like this one are easier to find and track.

I have specific questions for Alexandra (in QA) and for Florian below.

----------------------------
Detailed reply:

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #4)
> Brad/Maire what do you want to do here?  I'm assuming that it's not
> worthwhile to backout 973001 if that is indeed the cause.  

We can back it out in Fx35 to see if it fixes the problem, but I was just looking at the patches on bug 973001 (especially the first patch, which is likely where the regression is) and what other code may depend on them.  The backout (and verification that everything is "ok") could get messy.  I'm just debating if the risk and time to back it out is worth it given the bug that we want to avoid -- which seems unlikely for a user to hit.

> We're down to our last desktop beta today and I'm inclined to wontfix this for 35 though it's
> unfortunate to ship a new user-facing regression.

My big question is: are there other, more common test cases that can trigger this bug?  Or does the user need to physically disconnect his hardware in between 2 getUserMedia/permission requests in order to see this?  
This doesn't seem like something the user could get socially engineered into doing or something that a user would do more than once in a long time, if ever.  

I'm doing a needinfo to Alexandra (the reporter of this bug) to ask:  are there more common use cases that could trigger this bug?  Also, Alexandra -- does the "global indicator" (the microphone/camera icon that appears on top of all browser windows) appear and disappear also, or does it stay around?  I didn't see it appear at all in the screencast you provided; were you using multiple monitors?  (I'm trying to figure out why we didn't see it in the screencast.)

Given that this bug seems very edge-case and that we're pretty close to release, I think we should seriously consider "won't fixing" this for Fx35, especially if we try a backout and the backout gets messy.  I would be ok with wontfixing this for Fx35.

Florian -- do you have the bandwidth once you're back from PTO to fix this for early Fx36 beta?  (We'd be targeting the week of Jan 5th or 12th for a fix.)  

Lastly, this bug is filed under "Firefox::General" and bugs like this one (dealing with microphone/camera permission which can have privacy implications) will tend to get lost/forgotten in that category.  I think it and bugs like it should get moved to a more specific category. 

(FYI: Although I was copied on the e10s bug that depends on this, I didn't know about this bug until today when Lukas needinfo'd me.  The fact that this had privacy implications was not clear without really reading this bug; a big part of the problem IMO is the category it's in (Firefox::General).)

If we want to use an existing category for bugs like this, we can use Core::WebRTC:Audio/Video.  I and the WebRTC/getUserMedia team actively monitor that category for bugs like this.  If folks prefer to keep bugs like this under "Firefox", then let's create a special bugzilla component -- e.g. "Firefox::doorhanger" or "Firefox::camera/microphone permissions", especially for bugs that could have A/V privacy issues or implications.  One way or other though, we should get bugs like this out of Firefox::General so that they get better visibility and attention.

Thanks.

	Florian Quèze [:florian] Assignee
	Comment 7 • 10 years ago

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #6)
> tl;dr -- We can back out bug 973001 from Fx35, but given how unlikely it
> seems it is for a user to hit this bug and where we are in the release cycle
> (and how many folks are on PTO), I would be ok with "wontfixing" this for
> Fx35, especially if the backout and verification got messy (which they might
> given the size of the patches and what the patches appear to touch).

Bug 973001 had a large patch, and several follow-ups. I don't see how a backout there could be clean, and I really don't think it could be a reasonable thing to do this late in the cycle. The bug here also seems very edgecasy.

> Florian -- do you have the bandwidth once you're back from PTO to fix this
> for early Fx36 beta?  (We'd be targeting the week of Jan 5th or 12th for a
> fix.)

I'll arrange to have the bandwidth to debug it. Can't promise about fixing it until I know what's going wrong ;-).

> If folks prefer to keep bugs like
> this under "Firefox", then let's create a special bugzilla component

These are really Firefox bugs; I would also like a bugzilla component to watch.

> -- e.g. "Firefox::doorhanger" or "Firefox::camera/microphone permissions",

The former may be confused with "Toolkit :: Notifications and Alerts", the latter doesn't obviously include screensharing.

Suggestion: Firefox::Device permissions

Still not an ideal name :-/.

	Ada [:adalucinet] Reporter
	Comment 8 • 10 years ago

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #6)
> I'm doing a needinfo to Alexandra (the reporter of this bug) to ask: 
> are there more common use cases that could trigger this bug?  

No, did not encounter any other scenarios besides this case.

> Also, Alexandra -- does the "global indicator" (the microphone/camera icon that appears
> on top of all browser windows) appear and disappear also, or does it stay around? I didn't 
> see it appear at all in the screencast you provided; 

The global indicator is indeed present and it stays around, but, after reproducing this bug, 'TypeError: notif is null' is thrown by webrtcUI.jsm:92 in Browser Console every time I click on it. 

> were you using multiple monitors?  (I'm trying to figure out why we didn't see it in the screencast.)
Yes, I am using multiple monitors :)

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 9 • 10 years ago

> Bug 973001 had a large patch, and several follow-ups. I don't see how a
> backout there could be clean, and I really don't think it could be a
> reasonable thing to do this late in the cycle. The bug here also seems very
> edgecasy.


Thanks for the feedback, all, happy with a wontfix over a messy backout for an edge case.

	Florian Quèze [:florian] Assignee
	Comment 10 • 9 years ago

What's happening here is that the recording-window-ended is fired from the frame that has the NO_DEVICES_FOUND error, causing this code http://hg.mozilla.org/mozilla-central/annotate/206205dd8bd1/browser/modules/ContentWebRTC.jsm#l206 to clear all the browser-specific sharing indicators.

I don't have a working patch yet, as my first patch attempt regresses some other edge cases.

	Sylvestre Ledru [:Sylvestre]
	Comment 11 • 9 years ago

Florian, do you expect to fix this bug for 36? Thanks

	Sylvestre Ledru [:Sylvestre]
	Comment 12 • 9 years ago

(don't hesitate to unassign it if you are not going to work on this)

	Florian Quèze [:florian] Assignee
	Comment 13 • 9 years ago

(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Florian, do you expect to fix this bug for 36? Thanks

I expect to fix this in the 38 cycle on central. For 36, I don't know yet if the patch will be safe enough.

	Sylvestre Ledru [:Sylvestre]
	Comment 14 • 9 years ago

As we already shipped 35 with this bug, we are going to wait for 37.

	Justin [:JW_SoftvisionQA]
	Comment 15 • 8 years ago

I can not reproduce this bug on Mozilla/5.0 (Windows NT 6.3; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

The only error I get in the console is TypeError: video.mozSrcObject is null. I get this after step 3.

I will close this bug. If this issue persists, please reopen.

	Florian Quèze [:florian] Assignee
	Comment 16 • 8 years ago

I can still reproduce.

To reproduce I can't unplug my macbook's camera, so instead I comment out the lines 95 to 99 here: http://hg.mozilla.org/mozilla-central/annotate/206205dd8bd1/browser/modules/ContentWebRTC.jsm#l95

	Florian Quèze [:florian] Assignee
	Comment 17 • 8 years ago

Unfortunately, I couldn't figure out how to write a test for this. I was hoping I could just use constraints to restrict the set of devices to an empty set, but this causes a different OverconstrainedError, which doesn't reproduce the bug.

	:Felipe Gomes (needinfo for replies!)
	Comment 18 • 8 years ago

Comment on attachment 8744334 [details] [diff] [review]
Patch

Review of attachment 8744334 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/webrtcUI.jsm
@@ +228,5 @@
> +        }
> +        if (!aMessage.data.documentURI && index < webrtcUI._streams.length)
> +          webrtcUI._streams.splice(index, 1);
> +        else
> +          webrtcUI._streams[index] = {browser: aMessage.target, state: aMessage.data};

can you add a comment here for this `if` block just so it is easier to read? Explaining that the window might have died so we need to remove it from _streams, or otherwise update the right _streams entry.

It now looks strange that the previous code always unconditionally added a new entry to _streams instead of checking these things. Clear bug or it made sense before?

	Florian Quèze [:florian] Assignee
	Comment 19 • 8 years ago

(In reply to :Felipe Gomes (needinfo me!) from comment #18)

> It now looks strange that the previous code always unconditionally added a
> new entry to _streams instead of checking these things. Clear bug or it made
> sense before?

The code previously had a bug that caused streams to be left in the _streams array in some edge cases involving iframes. In the general cases, either we had a recording-device-events event that caused us to clear the _streams array and rebuild it from scratch, or we were in cases where the tab was closed and it didn't matter anymore.

	Florian Quèze [:florian] Assignee
	Comment 20 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/8c55d8beba757967f6f5f7ff386499e8c6823c11
Bug 1104054 - The 'recording-window-ended' notification from a subframe shouldn't remove the sharing indicator of the whole browser, r=felipe.

	Carsten Book [:Tomcat]
	Comment 21 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8c55d8beba75

	Farhadur Reja Fahim
	Comment 22 • 8 years ago

I have reproduced this bug with Nightly 36.0a1 (2014-11-24) on Ubuntu 14.04, 64 bit!

The bug's fix is now verified on latest Beta 49.0b3.

Beta 49.0b3:
Build ID 	20160811031722
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160812]

	Md. Majedul islam
	Comment 23 • 8 years ago

I have reproduced this bug with Nightly 36.0a1 (2014-11-24) on windows 10, 64 bit!

The bug's fix is now verified on latest Beta 49.0b7.


Build ID 	20160825132718
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160826]

	Ciprian Georgiu, Desktop QA
	Comment 24 • 8 years ago

Thank you Fahim and Md.Majedul isalm for your implication and for verifying this bug. Per comment 22 and comment 23, I will change the status flag for 49 to verified and closing this bug as verified fixed.