1196706 - [e10s] Address bar can be hidden in popup windows

Status: RESOLVED WORKSFORME

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced with:
*latest Aurora, build ID: 20150819004006
*latest Nightly, build ID: 20150819030206.

Reproduced on:
*Windows 7 64-bit, Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.10.4

Steps to reproduce:
1. Open Firefox using a clean profile (make sure e10s is enabled).
2. Go to www.facebook.com and log in.
3. Select a friend and start a video call with him.

Expected Results:
The device sharing pop-up is properly displayed and the user can select which camera/microphone to share. There should be no UI issues.

Actual Results:
The device sharing pop-up is incorrectly placed.
*Windows and Ubuntu: http://i.imgur.com/WdUiXyT.png
*Mac OS X: https://drive.google.com/file/d/0B1qNNKqwHiFfXzg4NzF3SVFhYTQ/view

Notes:
On Mac, the user is unable to share devices at all, therefore video calling is blocked.
This issue is e10s related.

Comment 1

[Maire Reavy [:mreavy]]

I think the issue here is that Facebook is popping up a window with no URL bar and so the front end code isn't sure where to put the doorhanger.  Not sure if this is actually a bug in "toolkit::notifications and alerts" or "firefox::devicepermissions" so I'm cc'ing the folks who can look into it and make that assessment.   Florian is on PTO until end of August; so I'm cc'ing (and needinfo'ing) Matt, Mike, and Dave to make a first pass assessment on where the problem is and how best to fix it.   

FYI: If I'm correct, I suspect all doorhangers would be broken in this way when they appear in a window with no URL bar -- if it's even possible to open them without a URL bar.  gUM is somewhat unique in that it automatically opens when the request is made.

Comment 2

[Dave Townsend [:mossop]]

This probably is a bug in the popup notifications code. Easy things are probably to just put it up at the top left pointing to the title bar or to remove the arrow and just have it float in the middle but we should have UX make a call here. Stephen can you do that or bounce it to whoever is appropriate?

Comment 3

[Stephen Horlander [:shorlander]]

I think the proper solution is to not allow sites to hide the location bar. It's a security problem and a UX problem.

Comment 4

[Matthew N. [:MattN]]

(In reply to Stephen Horlander [:shorlander] from comment #3)
> I think the proper solution is to not allow sites to hide the location bar.
> It's a security problem and a UX problem.

Yeah, the no address bar case should never happen and that's a bug on its own which is most likely a regression. It sounds similar to bug 1189554 which is also about a regression regarding window open arguments (which control toolbar visibility). I see now that this was reported as e10s related so it's possible this never worked yet with e10s and we need a new bug on that in some other component. e.g. Core or Firefox.

For the OSs where the addressbar is visible this is probably a bug in PopupNotifications (possibly also a regression; maybe related to somewhat recent iconbox changes.) This bug can handle that case. Can we get a regression range on it?

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Chad, is the front end team taking this? How important do you think it is to the e10s roll out?

Comment 7

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #4)
> For the OSs where the addressbar is visible this is probably a bug in
> PopupNotifications (possibly also a regression; maybe related to somewhat
> recent iconbox changes.) This bug can handle that case. Can we get a
> regression range on it?

Mozilla-Central:
Last good revision: bfd82015df48 (2015-06-11)
First bad revision: 0093691d3715 (2015-06-12)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bfd82015df48&tochange=0093691d3715

Mozilla-Inbound:
Last good revision: 30e375742c0a
First bad revision: 7c28bef40970
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=30e375742c0a&tochange=7c28bef40970

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

From comment 0, this blocks Hello working on Macs on Nightly and Aurora and the regression is from 41 onwards. (Even if 41 beta/release users won't be affected for now).   

I can't tell who should be working on this but I do think it looks important.

Comment 9

[Matthew N. [:MattN]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #8)
> I can't tell who should be working on this but I do think it looks important.

I would start with mconley since it's related to bug 1114299, it's and e10s regression, and the regression range includes bug 1171537 which I'm guessing isn't the actual cause but perhaps this testcase didn't work before that date due to the crash.

Since mconley is on PTO, maybe smaug or bholley who reviewed related patches.

Comment 10

[Matthew N. [:MattN]]

Can we get this on the security team radar so we don't ship with a security regression in e10s?

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Can one of you take this on?

Comment 12

[Daniel Veditz [:dveditz]]

Even though the window.open() arguments nominally allow turning everything off we long ago discovered that led to easy phishing for sites that depend on popups so we always show at least a read-only urlbar (with security notifications like the lock and the Larry dropdown). Other browsers followed suit.

With the rise of 3rd party services like payments and authentication this is even more important. Several of these types of services use popups because they are more secure than in-page iframes where the user has no real idea whether the frame is showing the legit content or is trying to phish them. These services now rely on the cross-browser behavior of showing a reliable address. Not showing that information needs to be fixed before we ship.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Matt, did you have a question for me?

Comment 14

[Matthew N. [:MattN]]

Finding someone to take this while mconley is out.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

e10s is not enabled by default for FF41 so this cannot be a release blocker. Given that we are a week away from building 41 RC, I think it is too late to take a fix in Beta41. I prefer resolving this as wontfix for 41. If we do have a patch ready in the next week, we can do a risk/benefit analysis.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Stop tracking as we will disable e10s in aurora in the next few days.

Comment 17

[:Gijs (he/him)]

I actually can't reproduce this on OSX? Am I missing something? Tried both on actual FB and with a simple testcase of

<body>
<button id="foo">Click me</button>
<script>
document.getElementById("foo").addEventListener("click", function() { window.open("http://www.mozilla.org/", "_blank", "location=no"); });
</script>
</body>


which doesn't exhibit a lack of location bar, either. Did this get fixed in the meantime? Cornel?

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

Actually, this does not reproduce anymore using latest builds, across platforms. It seems that something fixed this.

Closing as WFM. Thanks!

Comment 19

[:Gijs (he/him)]

... yeah, except I can't reproduce on the broken build with the testcase I posted in comment #17, nor with Facebook. Feels like we're missing something still. I'm a little uncomfortable marking this fixed if we're not 100% sure what the issue was and how it got fixed. Matt, could you reproduce this originally?

Comment 20

[Matthew N. [:MattN]]

No, I didn't attempt to reproduce this but it sounded very similar to bug 1189554 (also about the window arguments) which I could reproduce (and still can). I also can't reproduce this with a 2015-08-19 build on Windows 8.

Comment 21

[Matthew N. [:MattN]]

The bug report really should have captured the window.open arguments as it's possible that there is still a bug but Facebook changed the arguments.

Comment 22

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #21)
> The bug report really should have captured the window.open arguments as it's
> possible that there is still a bug but Facebook changed the arguments.

right, this is what I am afraid of...

Comment 23

[:Gijs (he/him)]

zpao, any chance you can check if FB made changes here in the arguments to window.open that would help us figure out why this was broken before and is now working with the same copy of Firefox? (in particular, if that can help us figure out if it's still broken and under what circumstances?)

Comment 24

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Best I can identify - no, the features arguments to window.open did not change at that call site. The URL that we opened did change slightly and maybe that changed when we request permissions.

Comment 25

[:Gijs (he/him)]

(In reply to Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) from comment #24)
> Best I can identify - no, the features arguments to window.open did not
> change at that call site. The URL that we opened did change slightly and
> maybe that changed when we request permissions.

Thanks!

Then I fear this will remain a mystery until/unless we find a way to reproduce on the old or new builds...

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Untracking since we don't have a conclusive result. If this reopens please do renominate it.