1278763 - Geolocation permission doorhanger doesn't appear inside RDM

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The permission doorhanger (triggered by features like geolocation) doesn't show up inside RDM.

Test page: http://html5demos.com/geo (should appear on load)

Comment 1

[Benoit Chabod [:bigben]]

Okay, I've been investigating this bug because it's getting in the way of Bug 1069882.
When a web page calls a service from navigator.geolocation, nsContentPermissionUtils::AskPermission is called [1], which itself calls other functions (depending on process configuration), and it ends up in nsBrowserGlue.

The prompt() function in nsBrowserGlue checks if the browser has a PopupNotifications attribute [2].
When we enable RDM, the browser is the viewport, a HTMLIFrameElement that doesn't have a PopupNotifications attribute, so the code bails out here.

I have written a quick fix that copies the PopupNotifications attribute from the original browser in the iframe (called the innerBrowser), and I will post it shortly after.

I tested it and it fixes the bug. But maybe a getter would be better than a copy?

Besides, I also noticed that this missing PopupNotifications attribute may be caused by a problem in the swap content magic used by RDM. In the documentation, there's a swapping step regarding PopupNotifications [3].
I don't have enough knowledge about the swapping thing, but maybe we could fix this instead of copying the object?

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/geolocation/nsGeolocation.cpp#212
[2]: https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#2790
[3]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsive.html/docs/browser-swap.md#45

Comment 2

[Benoit Chabod [:bigben]]

Attachment: Patch - add a PopupNotifications backend on the RDM browser

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks for tracing this back to the root cause, this helps a lot! :)

Your initial patch suggests it is fine for PopupNotifications (on the RDM UI / viewport owner window) to be unavailable during the swapping itself.  Can you confirm that we do actually reach the end of PopupNotifications._swapBrowserNotifications when opening and closing RDM (perhaps add a dump call to the end of the function and check the log)?  There are two swaps during open and two swaps during close, so you should see it logged 4 times after opening and closing RDM.

Assuming that is working, then it appears to be fine fix up PopupNotifications after the swap as part of the tunnel.js module which contains the pluming to wire up the outer browser window UI to the viewport.  Let's add a PopupNotifications getter at the end of tunnel.start() and remove it during tunnel.stop().

Comment 4

[Benoit Chabod [:bigben]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Thanks for tracing this back to the root cause, this helps a lot! :)

No problem! :)

> Your initial patch suggests it is fine for PopupNotifications (on the RDM UI
> / viewport owner window) to be unavailable during the swapping itself. 

I'm not sure I follow you here. How is my patch suggesting this?

> Can you confirm that we do actually reach the end of
> PopupNotifications._swapBrowserNotifications when opening and closing RDM
> (perhaps add a dump call to the end of the function and check the log)? 
> There are two swaps during open and two swaps during close, so you should
> see it logged 4 times after opening and closing RDM.

Well, it doesn't reach the end of PopupNotifications._swapBrowserNotifications, because there is no PopupNotifications available on the iframe ownerDocument.defaultView. It bails out here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#1002

> Assuming that is working, then it appears to be fine fix up
> PopupNotifications after the swap as part of the tunnel.js module which
> contains the pluming to wire up the outer browser window UI to the viewport.
> Let's add a PopupNotifications getter at the end of tunnel.start() and
> remove it during tunnel.stop().

Even if _swapBrowserNotifications doesn't finish properly, I'll post a patch using this method shortly after. It works well!

Comment 5

[Benoit Chabod [:bigben]]

Attachment: Patch 2 - Add a PopupNotifications backend on the RDM browser

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Benoit Chabod [:bigben] from comment #4)
> > Your initial patch suggests it is fine for PopupNotifications (on the RDM UI
> > / viewport owner window) to be unavailable during the swapping itself. 
> 
> I'm not sure I follow you here. How is my patch suggesting this?

Your initial patch set up PopupNotifications _after_ all the other steps in swap.start(), which means it comes after the two browser swaps that happen as part of that process.  If it was necessary for it to be pre-defined _before_ the swapping, then your initial patch wouldn't have worked.  So, that's what suggested to me that it was okay to define it sometime later, after the actual browser swap steps.

> > Can you confirm that we do actually reach the end of
> > PopupNotifications._swapBrowserNotifications when opening and closing RDM
> > (perhaps add a dump call to the end of the function and check the log)? 
> > There are two swaps during open and two swaps during close, so you should
> > see it logged 4 times after opening and closing RDM.
> 
> Well, it doesn't reach the end of
> PopupNotifications._swapBrowserNotifications, because there is no
> PopupNotifications available on the iframe ownerDocument.defaultView. It
> bails out here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/
> PopupNotifications.jsm#1002

That's not exactly right... it's true that we don't reach the end of PopupNotifications._swapBrowserNotifications, but it's not because of the check you pointed at.  I added some logging statements locally (without any of your patches applied), and it seems to get past the `otherBrowser.ownerDocument.defaultView.PopupNotifications` check.  Then it bails out on the next block because there are no notifications to swap, which is fine.

It's somewhat ironic that it gets past the `otherBrowser.ownerDocument.defaultView.PopupNotifications` check.  I believe it's actually because of a typo / small bug, that reverses the order of arguments.  It is always passed the current browser as `outerBrowser` and the RDM viewport frame ends up as `ourBrowser`, which is never checked (it would fail if it was).  So, now we'll be depending on this probable bug...  but it's a small thing, I am not that worried about it.

Anyway, that's why we're able to set up PopupNotifications after the swaps have already happened.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8777398 [details] [diff] [review]
Patch 2 - Add a PopupNotifications backend on the RDM browser

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

Thanks, this makes sense to me!  Seems fine to land as-is, but feel free to add a test if you like.

Comment 8

[Benoit Chabod [:bigben]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> That's not exactly right... it's true that we don't reach the end of
> PopupNotifications._swapBrowserNotifications, but it's not because of the
> check you pointed at.  I added some logging statements locally (without any
> of your patches applied), and it seems to get past the
> `otherBrowser.ownerDocument.defaultView.PopupNotifications` check.  Then it
> bails out on the next block because there are no notifications to swap,
> which is fine.
> 
> It's somewhat ironic that it gets past the
> `otherBrowser.ownerDocument.defaultView.PopupNotifications` check.  I
> believe it's actually because of a typo / small bug, that reverses the order
> of arguments.  It is always passed the current browser as `outerBrowser` and
> the RDM viewport frame ends up as `ourBrowser`, which is never checked (it
> would fail if it was).  So, now we'll be depending on this probable bug... 
> but it's a small thing, I am not that worried about it.

Wow, my bad.
I added logging statements at lines 996 and 1002, and since the one at line 1002 didn't appear I was positive that the early return was due to the missing PopupNotifications object, not to an empty notification list.

Anyway, since we'll be depending on this, I've added a RDM test that checks if permission popups appear inside RDM. I've made minor changes in the first patch (just a few things in the comments), I will post this soon.

Comment 9

[Benoit Chabod [:bigben]]

Attachment: Part 1: Add a PopupNotifications backend on the RDM browser

Comment 10

[Benoit Chabod [:bigben]]

Attachment: Part 2: Add tests for permission prompts

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8779301 [details] [diff] [review]
Part 1: Add a PopupNotifications backend on the RDM browser

Thanks, looks good!

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8779302 [details] [diff] [review]
Part 2: Add tests for permission prompts

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

Test looks good to me, thanks for adding it!  Feel to land after making the small change I noted.

::: devtools/client/responsive.html/test/browser/browser_permission_doorhanger.js
@@ +34,5 @@
> +  ok(true, "Permission doorhanger appeared without RDM enabled");
> +
> +  // Lets switch back to the dummy website and enable RDM
> +  yield load(browser, DUMMY_URL);
> +  let { manager } = yield openRDM(tab);

The `ui` object returned here should be what you need, so you can skip the `getResponsiveUIForTab` step.

Comment 13

[Benoit Chabod [:bigben]]

Attachment: Part 2: Add tests for permission prompts, v2

Thanks for the review!
Here's a second version of the test, using the UI object as you suggested.

Comment 14

[Benoit Chabod [:bigben]]

And this is the corresponding try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b418baa4e0c7

It looks like failures are related to intermittent bugs, so I guess we're good to go :)

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/356bb4c55f4d
https://hg.mozilla.org/integration/fx-team/rev/c4a79e57209a

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/356bb4c55f4d
https://hg.mozilla.org/mozilla-central/rev/c4a79e57209a

Comment 17

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 50.0a1 (2016-07-10), under Windows 10x64, using the STR from Comment 0.

The issue is no longer reproducible on Firefox 51.0a1 (2016-08-17) using STR from Comment 0, but I also performed a few tests using other web pages that trigger a permission doorhanger and noticed that in some cases, the doorhanger is not displayed:
- http://mozilla.github.io/webrtc-landing/gum_test.html - if Video/Audio/Audio & Video options are selected
- http://webaudioplayground.appspot.com/# - if the Live Input option from '+ADD SOURCE' is selected
- https://webrtc.github.io/samples/src/content/getusermedia/gum/

Also, I noticed that when trying to install an Add-on with the RDM enabled, a doorhanger is displayed, informing the user that "Nightly prevented this site from asking you to install software on your computer." and the add-on is not installed. (Eg. https://addons.mozilla.org/en-US/firefox/addon/adblock-plus/?src=cb-dl-users) . Is this an expected behavior?

Should I log new bugs for the issues described above?

The tests were performed under Windows 10x64, Mac OS X  10.11.1 and under Ubuntu 14.04 x64.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Mihai Boldan, QA [:mboldan] from comment #17)
> I managed to reproduce this issue on Firefox 50.0a1 (2016-07-10), under
> Windows 10x64, using the STR from Comment 0.
> 
> The issue is no longer reproducible on Firefox 51.0a1 (2016-08-17) using STR
> from Comment 0, but I also performed a few tests using other web pages that
> trigger a permission doorhanger and noticed that in some cases, the
> doorhanger is not displayed:
> - http://mozilla.github.io/webrtc-landing/gum_test.html - if
> Video/Audio/Audio & Video options are selected
> - http://webaudioplayground.appspot.com/# - if the Live Input option from
> '+ADD SOURCE' is selected
> - https://webrtc.github.io/samples/src/content/getusermedia/gum/

Thanks for spotting this.  I renamed this bug to be specific to geolocation.  Please file a new bug for this WebRTC specific doorhanger issue.  It looks like another small tweak is needed for this case.

> Also, I noticed that when trying to install an Add-on with the RDM enabled,
> a doorhanger is displayed, informing the user that "Nightly prevented this
> site from asking you to install software on your computer." and the add-on
> is not installed. (Eg.
> https://addons.mozilla.org/en-US/firefox/addon/adblock-plus/?src=cb-dl-
> users) . Is this an expected behavior?

Interesting!  It's probably not expected, but I think let's ignore this one for now.  If someone else hits this one, we can reconsider.

Thanks for testing this!

Comment 19

[Mihai Boldan, Desktop QA [:mboldan]]

Since Bug 1297040 was logged separately for the issue related to WebRTC specific doorhanger, and since not action is needed for now, related to the Add-ons doorhanger, I am marking this issue Verified Fixed.