1252262 - E10s effects Spotify.com Sign Up Login Window

Status: VERIFIED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Michelle Funches - QA]

1. Access spotify.com 
2. Select "Play Free" option
3. Select Sign Up with Facebook" option
4. Enter credentials and click Log In
5. Move the mouse over the drop down button that displays "Friends"
Observe: The dialog (pop-up)window moves horizontally across the screen.
Expectation: Window is stable and allows access to the drop down w/o interference.
Note: This does not occur when Multi-process is disabled. Ovserved in Windows 10 and Windows XP

Comment 1

[Michelle Funches - QA]

mozRegression:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4cd40d0c7860e70674e1136f3720abf2ae841d6e&tochange=fcbc5a4f5bb19c05af12753ac3733d7c7bee9332

Changing to Core: Event Handling: please review I don't see any other items related to this behavior so component may need to change.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

I'm guessing bug 1153023.

Comment 3

[Michelle Funches - QA]

Attachment: SignupWithFacebook_E10s_Aurora.7z

file to large, it is zipped to compress

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I can repro and it's a rather entertaining effect. Chasing it down now. Relatedly, if I try to follow the STR with e10s disabled, the facebook login window that spawns isn't visible. I can see it in the alt-tab switcher, and I can close it from the taskbar, but I can't see it as a "window" on the screen anywhere. That's probably a separate bug, perhaps a regression. I'll file something at some point for it.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I filed bug 1252974 for the non-e10s bug I mentioned in comment 4. It's more important now because with the patch that I think should fix this bug, I start running into that one. So that will probably need to be fixed in order to properly fix this one.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Possible fix

This is what I think should fix this. Applying this causes the e10s path to hit bug 1252974 as well.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Using a secondary monitor I was able to bypass bug 1252974 and verify that this does fix the problem. I'll do a try push.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b47eba5cf525

Comment 9

[Michelle Funches - QA]

FYI: I did apply this try build and I could not reproduce on Windows 10 or Windows XP

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Thanks for testing! Unfortunately it looks like it also causes some test failures. I think I know why, here's another try push that hopefully fares better:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7be092dd3ba

Comment 11

[Michelle Funches - QA]

Looking good - not reproduced on Windows XP or Windows 10
If there are additional features/sites you would like me to confirm, I will

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I can reproduce this by clicking one of the "Share on Facebook" icons on http://www.cnn.com/2016/03/02/us/flight-attendant-fire/index.html, but only when I'm logged in.

Comment 13

[Michelle Funches - QA]

@Ryan: please clarify this was with the try_build? I access that Flight site on Windows XP & Windows 10 but could not reproduce on the try_build, however I can reproduce with Aurora and Nightly.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

No, just on a regular nightly. Glad you can confirm it's fixed with the Try build, though :)

Comment 15

[Kartikaya Gupta (email:[e-mail])]

I tracked down the test failure locally and fixed it (I hope). Latest try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1c9759ee2fa&group_state=expanded

I'll put the patch up for review.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Tagging billm since he reviewed the patch that supposedly regressed this, but please redirect as appropriate.

The fix is fairly straightforward: when sending the window's outer rect position from the parent to child, keep the client offset separate. On the child, combine it in on the codepaths that need it, but don't otherwise. The bug was caused by this clientoffset getting re-added when the content was doing window.moveBy(0,0) or something of that nature which should have been a no-op.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8726875 [details] [diff] [review]
Patch

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

Oh wait that's the wrong version of the patch...

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Here we go, re-tested locally to make sure it still works. (Sorry for the churn, I wrote the patch on my Linux machine and had to test it on my Windows machine, but screwed up the version I used).

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8726885 [details] [diff] [review]
Patch

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

I totally don't understand what's going on here. Maybe Jim can review.

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 8726885 [details] [diff] [review]
Patch

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

::: widget/PuppetWidget.cpp
@@ +1233,5 @@
>    }
>  
>    int32_t winX, winY, winW, winH;
>    NS_ENSURE_SUCCESS(GetOwningTabChild()->GetDimensions(0, &winX, &winY, &winW, &winH), nsIntPoint());
> +  return nsIntPoint(winX, winY) + GetOwningTabChild()->GetClientOffset().ToUnknownPoint();

Is this going to break mac plugin event coordinates?

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp#1048

Comment 21

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jim Mathies [:jimm] from comment #20)
> Comment on attachment 8726885 [details] [diff] [review]
> Patch
> 
> Review of attachment 8726885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/PuppetWidget.cpp
> @@ +1233,5 @@
> >    }
> >  
> >    int32_t winX, winY, winW, winH;
> >    NS_ENSURE_SUCCESS(GetOwningTabChild()->GetDimensions(0, &winX, &winY, &winW, &winH), nsIntPoint());
> > +  return nsIntPoint(winX, winY) + GetOwningTabChild()->GetClientOffset().ToUnknownPoint();
> 
> Is this going to break mac plugin event coordinates?
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/
> nsPluginInstanceOwner.cpp#1048

I don't think so. Previously the client offset was included in the values returned by the GetDimensions call (because it was added on the parent side before the IPC). With my change the GetDimensions call doesn't include the client offset any more (because it just returns GetOuterRect) and so I add it in manually to make sure the value being returned by PuppetWidget::GetWindowPosition() is the same as it was before.

I actually discovered this by a test failure on the try push in comment 10 - there was an APZ test which was using nsGlobalWindow::GetMozInnerInnerScreenX/Y, and that ends up calling PuppetWidget::GetWindowPosition() via PuppetWidget::WidgetToScreenOffset. So if GetWindowPosition() returns a different value from before it breaks that test.

The actual fix for this bug comes from some other call sites to TabChild::GetOuterRect/GetDimensions, which now return a value without the client offset.

That being said, if you have a way to test this mac plugin events stuff locally please let me know and I can verify.

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8726885 [details] [diff] [review]
Patch

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> 
> That being said, if you have a way to test this mac plugin events stuff
> locally please let me know and I can verify.

I usually just look for issues with mouse hover over buttons on youtube or other test sites.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jim Mathies [:jimm] from comment #22)
> I usually just look for issues with mouse hover over buttons on youtube or
> other test sites.

Did this on a local mac build and everything seemed fine.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79d962afab8a

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79d962afab8a

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Since this is e10s-only, marking 45 as disabled because it doesn't have e10s enabled in GA. 46 and 47 are affected if e10s is enabled.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8726885 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1153023
[User impact if declined]: with e10s enabled: on spotify.com, logging in via facebook and then mousing over the friends dropdown causes the popup window to shift until it reaches the right edge of the screen. it's entertaining but not correct behaviour. it could happen on other websites as well although there haven't been other reports of it.
[Describe test coverage new/current, TreeHerder]: most of the code involved is exercised via existing automation tests, verified the fix locally
[Risks and why]: a tad risky since this code has been fiddled with a number of times. but given the badness i think it's worth uplifting.
[String/UUID change made/needed]: none

Comment 28

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

Michell, could you please verify this issue is fixed as expected on a latest Nightly build (03-10 should have the fix)? Thanks!

Comment 29

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

Given the risk factor mentioned in comment 27, I want to wait for Michelle's verification before uplifting to Fx47.

Comment 30

[Michelle Funches - QA]

Spotify and CNN/Flight meet expectations when Multiprocess Windows is enabled :)
Please let me know if you want additional OS Verification

Version 	48.0a1
Build ID 	20160310030242
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0


Version 	48.0a1
Build ID 	20160310030242
User Agent 	Mozilla/5.0 (Windows NT 5.1; rv:48.0) Gecko/20100101 Firefox/48.0


Not detailed here but has also passed on 48.0a1 for Macintosh 10.11

Comment 31

[Michelle Funches - QA]

Clarifying: Pardon lack of clarity - this test results are good with E10s On and Off

Comment 32

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

(In reply to Michelle Funches - QA from comment #31)
> Clarifying: Pardon lack of clarity - this test results are good with E10s On
> and Off

Awesome! Thanks for a prompt verification, will approve Aurora uplift now.

Comment 33

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

Comment on attachment 8726885 [details] [diff] [review]
Patch

Fix was verified, taking it in Aurora47.

Comment 34

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

Comment on attachment 8726885 [details] [diff] [review]
Patch

It's good to have login menus work with e10s, let's take this for beta 2.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e20a701d730d

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Beta.

Comment 38

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Confirmed fixed on Aurora 47.0a2 (2016-03-14) and Nightly 48.0a1 (2016-03-11), using Windows 10 x64.

Comment 39

[Kartikaya Gupta (email:[e-mail])]

Rebased and landed:

https://hg.mozilla.org/releases/mozilla-beta/rev/5cdbd68d927e

Comment 40

[Andrei Vaida [:avaida]]

Setting the flag for verification on 46.

Comment 41

[Michelle Funches - QA]

Tested: Windows 10x64 with Firefox x32 & x64 also Windows XP and Mac 10.11
Multiprocess Windows 	2/2 (Enabled by default) [browser.tabs.remote.autostart.2; true]
Functionality is good with Multiprocess Enabled and Disabled
Version 	46.0b2
Build ID 	20160314144540

Comment 42

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

Setting flags based on comment 38 and comment 41.