684sigma Reporter
	Description • 7 years ago

I have a problem with Firefox Beta 52. It doesn't happen on Firefox ESR 45.
Sometimes when I click on permissions block floating at the top of the screen to open exact tab that has permission to use my microphone/camera, sometimes it opens popup with site permissions, and sometimes it doesn't.
It happens unpredictably, however, I noticed one specific scenario when it happens

1. Open https://mozdevs.github.io/MediaRecorder-examples/record-live-audio.html , allow to use the microphone
2. Select all text in location bar, type "."
3. Click on the right side of floating block with camera/microphone permissions

Result: Firefox window blinks (looses focus, then gets focus)
Expected: Panel with site permissions should open

Floating block with camera/microphone permissions is a small rectangle that appears at the top of the screen and contains of 2 halves: the left side displays Firefox logo, the right part displays white microphone/camera icon on orange background

	Johann Hofmann [:johannh]
	Comment 1 • 7 years ago

Thanks for reporting this, I agree that it should be possible to cancel the camera/microphone access even if the urlbar is in "invalid" state. We encountered this problem in bug 1300755 but decided that showing the security indicators while the urlbar is invalid is too much of a security risk since it enables much easier URL spoofing.

A solution for this that came up in our discussion of this bug was to reset the urlbar state and open the identity popup when the user clicks on the (i) icon while a sharing indicator is shown and pageproxystate=invalid. The potential data loss is not ideal but I think it's the best compromise in this situation and this is still pretty much an edge case.

	YF (Yang)
	Comment 2 • 7 years ago

(In reply to Johann Hofmann [:johannh] from comment #1)
> Thanks for reporting this, I agree that it should be possible to cancel the
> camera/microphone access even if the urlbar is in "invalid" state. We
> encountered this problem in bug 1300755 but decided that showing the
> security indicators while the urlbar is invalid is too much of a security
> risk since it enables much easier URL spoofing.
> 
> A solution for this that came up in our discussion of this bug was to reset
> the urlbar state and open the identity popup when the user clicks on the (i)
> icon while a sharing indicator is shown and pageproxystate=invalid. The
> potential data loss is not ideal but I think it's the best compromise in
> this situation and this is still pretty much an edge case.

Possible to reset (when opened) and revert (when closed)?

	Peter Dolanjski [:pdol]
	Comment 3 • 7 years ago

Philipp, your input needed on this use case.  Take a look at the proposal in Comment 1.

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 4 • 7 years ago

I think reverting makes sense here.
We should be able to handle this better once we have permissions UI exposed in preferences, to which we can then link instead of the doorhanger. But reverting the URL sounds like a decent interim solution.

	Gerry Chang [:gchang]
	Comment 5 • 7 years ago

Track 53+ as regression.

	Gerry Chang [:gchang]
	Comment 6 • 7 years ago

Hi :johannh,
According to comment #4, can you help find an owner for this?

	Johann Hofmann [:johannh]
	Comment 7 • 7 years ago

Yes, we're taking care of it. Thanks :)

	Johann Hofmann [:johannh]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8852578 [details]
Bug 1345687 - Allow identity popup to be shown irrespective of pageproxystate if a permission has been granted to the current site.

https://reviewboard.mozilla.org/r/124766/#review127602

Thanks!

::: browser/base/content/browser.js:7444
(Diff revision 3)
> -    // Don't allow left click, space or enter if the location has been modified.
> -    if (gURLBar.getAttribute("pageproxystate") != "valid") {
> +    // Don't allow left click, space or enter if the location has been modified,
> +    // so long as we're not sharing any devices.
> +    // If we are sharing a device, the identity block cannot be focused (and
> +    // therefore, interacted with) by the user. However, we want to allow opening
> +    // the identity popup from the device control menu, which calls click() on the
> +    // identity button, so we don't return early.

You should mention that the identity block should be disabled via CSS so that this doesn't affect the case of the user directly clicking into it.

::: browser/base/content/browser.js:7446
(Diff revision 3)
> +    // If we are sharing a device, the identity block cannot be focused (and
> +    // therefore, interacted with) by the user. However, we want to allow opening
> +    // the identity popup from the device control menu, which calls click() on the
> +    // identity button, so we don't return early.
> +    if (!this._sharingState &&
> +        gURLBar.getAttribute("pageproxystate") != "valid") {

I don't really like that we're doing this to work around the fact that other code just calls identityBlock.click()

Ideally we wouldn't have to do this and just use an API call like showPopup to show the popup.

But I guess this is broken in beta and this solution is likely the most conflict-free way to get it uplifted.

	Pulsebot
	Comment 13 • 7 years ago

Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/665c55716bf6
Allow identity popup to be shown irrespective of pageproxystate if a permission has been granted to the current site. r=johannh

	Carsten Book [:Tomcat]
	Comment 14 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/665c55716bf6

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 7 years ago

Is this something we should consider backporting to affected branches or can it ride the trains?

	(inactive) Nihanth Subramanya [:nhnt11] Assignee
	Comment 16 • 7 years ago

Comment on attachment 8852578 [details]
Bug 1345687 - Allow identity popup to be shown irrespective of pageproxystate if a permission has been granted to the current site.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: If the URL bar text value has been modified, no way to open the identity popup and revoke device permissions.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes. STR in comment 0. On macOS, you can navigate to the site linked in comment 0, allow the microphone, modify the URL bar text, and then in the microphone menu  (appears somewhere at the top right of the macOS menubar) choose "Control Sharing". Without the patch, nothing happens. With the patch, the identity popup is shown and from there the permission can be revoked.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simply allows programmatic click()'s to be called when sharing a device even if pageproxystate is invalid.
[String changes made/needed]: none

	(inactive) Nihanth Subramanya [:nhnt11] Assignee
	Comment 17 • 7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Is this something we should consider backporting to affected branches or can
> it ride the trains?

I had the approval request all filled out but apparently left the tab open without submitting it. smh.

	Hani Yacoub, Desktop QA
	Comment 18 • 7 years ago

Build ID: 20170402030202
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Windows 7 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.

	Gerry Chang [:gchang]
	Comment 19 • 7 years ago

Comment on attachment 8852578 [details]
Bug 1345687 - Allow identity popup to be shown irrespective of pageproxystate if a permission has been granted to the current site.

Fix a regression and was verified. Aurora54+ & Beta53+.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/1a46ff5995df

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/5995da420038

	Hani Yacoub, Desktop QA
	Comment 22 • 7 years ago

Build ID: 20170404004003
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on Firefox Aurora 54.0a2 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
I will verify this also on Beta when the build with the fix will be available.

	Hani Yacoub, Desktop QA
	Comment 23 • 7 years ago

Build ID: 20170404030727
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0 

Verified as fixed on Firefox Beta tinderbox-builds 53.0b9 on 
Windows 10 x 64 (https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win64/1491300447/) and on Mac OS X 10.12 (https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-macosx64/1491300447/).
I will test on Ubuntu 16.04 x64 when the build with the fix will be available.

	Hani Yacoub, Desktop QA
	Comment 24 • 7 years ago

Build ID: 20170407033734
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

Verified as fixed on Firefox Beta 53.0b10 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 7 years ago

Comment on attachment 8852578 [details]
Bug 1345687 - Allow identity popup to be shown irrespective of pageproxystate if a permission has been granted to the current site.

See comment 16.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 26 • 7 years ago

Comment on attachment 8852578 [details]
Bug 1345687 - Allow identity popup to be shown irrespective of pageproxystate if a permission has been granted to the current site.

I don't think this is a severe enough problem to warrant porting a fix to ESR52. We typically limit ESR uplifts to critical regressions and security issues. This is what ESR users warrant, dot releases that are stable and consistent quality. 

If a user runs into this regression (which seems like an edge case, see comment 1), they will workaround it by opening up the same site in a new tab and changing device sharing permission on the doorhanger UI without updating URL bar in parallel.