Closed
Bug 1278763
Opened 8 years ago
Closed 8 years ago
Geolocation permission doorhanger doesn't appear inside RDM
Categories
(DevTools :: Responsive Design Mode, defect, P1)
DevTools
Responsive Design Mode
Tracking
(firefox51 verified)
Tracking | Status | |
---|---|---|
firefox51 | --- | verified |
People
(Reporter: jryans, Assigned: bigben)
References
(Blocks 2 open bugs, )
Details
(Whiteboard: [multiviewport][reserve-rdm])
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bigben
:
review+
|
Details | Diff | Splinter Review |
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+
Updated•8 years ago
|
QA Contact: mihai.boldan
Assignee | ||
Comment 1•8 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•8 years ago
|
Assignee: nobody → bchabod
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jryans)
Reporter | ||
Comment 3•8 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•8 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•8 years ago
|
||
Attachment #8777398 -
Flags: feedback?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8775978 -
Attachment is obsolete: true
Reporter | ||
Comment 6•8 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•8 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•8 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•8 years ago
|
||
Attachment #8779301 -
Flags: review?(jryans)
Assignee | ||
Updated•8 years ago
|
Attachment #8777398 -
Attachment is obsolete: true
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8779302 -
Flags: review?(jryans)
Reporter | ||
Comment 11•8 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•8 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•8 years ago
|
||
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•8 years ago
|
Attachment #8779302 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 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 15•8 years ago
|
||
bugherder landing |
https://hg.mozilla.org/integration/fx-team/rev/356bb4c55f4d
https://hg.mozilla.org/integration/fx-team/rev/c4a79e57209a
Keywords: checkin-needed
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/356bb4c55f4d
https://hg.mozilla.org/mozilla-central/rev/c4a79e57209a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•8 years ago
|
Iteration: --- → 51.1 - Aug 15
Priority: P3 → P1
Comment 17•8 years ago
|
||
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•8 years ago
|
Summary: Permission doorhanger doesn't appear inside RDM → Geolocation permission doorhanger doesn't appear inside RDM
Reporter | ||
Comment 18•8 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)
Comment 19•8 years ago
|
||
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.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•