STR: 1. Open http://jsfiddle.net/srn9db4h/ 2. Observe grey camera icon in address bar and permission request. 3. Click "Share Selected Devices". 4. Observe pulsing red camera icon in address bar. 5. Refresh the page. Expected result: - Red pulsing camera icon disappears. Goto 2. Actual result: - Red pulsing camera icon remains. Goto 2 (i.e. both red and gray icons). This misbehavior is a regression that appears to have arrived with the new pulsing look.

Workaround: Selecting "Don't Share" in the new prompt makes both icons go away.

Unfortunately my test profile had e10s disabled. I can reproduce only if I enable e10s. I'm really surprised by this though, as I thought this was covered by automated tests.

So more details here: - This only happens when the device is used by a subframe. - Reloading the frame itself correctly clears the sharing indicator, it's only reloading the whole page that causes the indicator to remain visible.

In the specific case where the bug is reproduced, this test doesn't pass and the message is never sent to the UI: http://searchfox.org/mozilla-central/rev/b92ae9a263a05efefc0110eaad5e56369c9f1b10/browser/modules/ContentWebRTC.jsm#323 So it's likely that the changes in bug 1206233 made visible a bug in an edge case where it wasn't noticeable before (reloading the whole page clears all PopupNotification icons).

When the sharing indicator was a PopupNotification, the indicator got dropped automatically by the PopupNotifications.locationChange call... and the reference to the stream and browser remained in the webrtcUI module. This patch cleans up more explicitly.

Comment on attachment 8788929 [details] [diff] [review] Patch Review of attachment 8788929 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js @@ +213,5 @@ > + PopupNotifications.panel.firstChild.button.click(); > + }); > + yield expectObserverCalled("getUserMedia:response:allow"); > + yield expectObserverCalled("recording-device-events"); > + is((yield getMediaCaptureState()), "CameraAndMicrophone", nit: does (yield getMediaCaptureState()) really have to be in parentheses here? ::: browser/modules/webrtcUI.jsm @@ +128,5 @@ > } > }, > > + forgetStreamsFromBrowser: function(aBrowser) { > + let found = false; I think you can remove this :) @@ +134,5 @@ > + this._streams = this._streams.filter(stream => stream.browser != aBrowser); > + if (this._streams.length < length) { > + let tabbrowser = aBrowser.ownerGlobal.gBrowser; > + if (tabbrowser) > + tabbrowser.setBrowserSharing(aBrowser, {}); I don't understand why we need to make sure we actually removed some streams to reset the sharingState of the tab. In browser.js we already check if the tab has a sharingState. Shouldn't we reset this in any case? This patch would be much smaller (and IMO clearer) if you could skip the checks and move this line to browser.js.

(In reply to Johann Hofmann [:johannh] from comment #7) > browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js > @@ +213,5 @@ > > + PopupNotifications.panel.firstChild.button.click(); > > + }); > > + yield expectObserverCalled("getUserMedia:response:allow"); > > + yield expectObserverCalled("recording-device-events"); > > + is((yield getMediaCaptureState()), "CameraAndMicrophone", > > nit: does (yield getMediaCaptureState()) really have to be in parentheses > here? Probably not. I vaguely remember adding these parentheses to make a linter happy, but I can't reproduce it now... Still I would prefer to not change it here as I prefer to keep the file consistent (it was just a copy/paste here).

Comment on attachment 8789313 [details] [diff] [review] Patch v2 Review of attachment 8789313 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!

https://hg.mozilla.org/integration/fx-team/rev/beda515683970341d1338dbc687bb929c0ce291f Bug 1294576 - Remove device sharing indicator on location changes, r=johannh.

Verified fixed FX 51.0a1 (2016-09-14) Win 7