Geolocation permission doorhanger doesn't appear inside RDM

VERIFIED FIXED in Firefox 51

Status

P1
normal
VERIFIED FIXED
3 years ago
6 months ago

People

(Reporter: jryans, Assigned: bigben)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: [multiviewport][reserve-rdm], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Test page: http://html5demos.com/geo (should appear on load)
Flags: qe-verify+
QA Contact: mihai.boldan
(Assignee)

Updated

2 years ago
Blocks: 1069882
(Assignee)

Comment 1

2 years ago
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
(Assignee)

Updated

2 years ago
Assignee: nobody → bchabod
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8775978 [details] [diff] [review]
Patch - add a PopupNotifications backend on the RDM browser
(Assignee)

Updated

2 years ago
Flags: needinfo?(jryans)
(Reporter)

Comment 3

2 years ago
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().
Flags: needinfo?(jryans)
(Assignee)

Comment 4

2 years ago
(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!
(Assignee)

Comment 5

2 years ago
Created attachment 8777398 [details] [diff] [review]
Patch 2 - Add a PopupNotifications backend on the RDM browser
Attachment #8777398 - Flags: feedback?(jryans)
(Assignee)

Updated

2 years ago
Attachment #8775978 - Attachment is obsolete: true
(Reporter)

Comment 6

2 years ago
(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.
(Reporter)

Comment 7

2 years ago
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.
Attachment #8777398 - Flags: feedback?(jryans) → review+
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
Created attachment 8779301 [details] [diff] [review]
Part 1: Add a PopupNotifications backend on the RDM browser
Attachment #8779301 - Flags: review?(jryans)
(Assignee)

Updated

2 years ago
Attachment #8777398 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
Created attachment 8779302 [details] [diff] [review]
Part 2: Add tests for permission prompts
Attachment #8779302 - Flags: review?(jryans)
(Reporter)

Comment 11

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

Thanks, looks good!
Attachment #8779301 - Flags: review?(jryans) → review+
(Reporter)

Comment 12

2 years ago
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.
Attachment #8779302 - Flags: review?(jryans) → review+
(Assignee)

Comment 13

2 years ago
Created attachment 8779752 [details] [diff] [review]
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.
Attachment #8779752 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8779302 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
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 :)
Keywords: checkin-needed

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/356bb4c55f4d
https://hg.mozilla.org/mozilla-central/rev/c4a79e57209a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Iteration: --- → 51.1 - Aug 15
Priority: P3 → P1
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.
QA Whiteboard: [qe-rdm]
Flags: needinfo?(jryans)
(Reporter)

Updated

2 years ago
Summary: Permission doorhanger doesn't appear inside RDM → Geolocation permission doorhanger doesn't appear inside RDM
(Reporter)

Comment 18

2 years ago
(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!
Flags: needinfo?(jryans)
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.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
(Reporter)

Updated

2 years ago
Blocks: 1345495

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.