Closed Bug 1252262 Opened 4 years ago Closed 4 years ago

E10s effects Spotify.com Sign Up Login Window

Categories

(Core :: DOM: Content Processes, defect)

46 Branch
All
Windows
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
e10s m9+ ---
firefox45 --- disabled
firefox46 --- verified
firefox47 --- verified
firefox48 --- verified

People

(Reporter: mfunches, Assigned: kats)

References

Details

(Whiteboard: btpp-followup-2016-03-08)

Attachments

(2 files, 2 obsolete files)

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
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.
Component: Audio/Video: Playback → Event Handling
I'm guessing bug 1153023.
Flags: needinfo?(bugmail.mozilla)
file to large, it is zipped to compress
Whiteboard: btpp-followup-2016-03-08
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.
Assignee: nobody → bugmail.mozilla
Component: Event Handling → DOM: Content Processes
Flags: needinfo?(bugmail.mozilla)
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.
Depends on: 1252974
Attached patch Possible fix (obsolete) — Splinter Review
This is what I think should fix this. Applying this causes the e10s path to hit bug 1252974 as well.
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.
FYI: I did apply this try build and I could not reproduce on Windows 10 or Windows XP
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
Looking good - not reproduced on Windows XP or Windows 10
If there are additional features/sites you would like me to confirm, I will
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.
@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.
No, just on a regular nightly. Glad you can confirm it's fixed with the Try build, though :)
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.
Flags: in-testsuite?
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8725827 - Attachment is obsolete: true
Attachment #8726875 - Flags: review?(wmccloskey)
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...
Attachment #8726875 - Flags: review?(wmccloskey)
Attached patch PatchSplinter Review
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).
Attachment #8726875 - Attachment is obsolete: true
Attachment #8726885 - Flags: review?(wmccloskey)
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.
Attachment #8726885 - Flags: review?(wmccloskey) → review?(jmathies)
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
(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 on attachment 8726885 [details] [diff] [review]
Patch

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8726885 - Flags: review?(jmathies) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/79d962afab8a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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 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
Attachment #8726885 - Flags: approval-mozilla-beta?
Attachment #8726885 - Flags: approval-mozilla-aurora?
Michell, could you please verify this issue is fixed as expected on a latest Nightly build (03-10 should have the fix)? Thanks!
Flags: needinfo?(mfunches)
Given the risk factor mentioned in comment 27, I want to wait for Michelle's verification before uplifting to Fx47.
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
Flags: needinfo?(mfunches)
Status: RESOLVED → VERIFIED
Clarifying: Pardon lack of clarity - this test results are good with E10s On and Off
(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 on attachment 8726885 [details] [diff] [review]
Patch

Fix was verified, taking it in Aurora47.
Attachment #8726885 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8726885 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing for Beta.
Flags: needinfo?(bugmail.mozilla)
Confirmed fixed on Aurora 47.0a2 (2016-03-14) and Nightly 48.0a1 (2016-03-11), using Windows 10 x64.
Setting the flag for verification on 46.
Flags: qe-verify+
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
You need to log in before you can comment on or make changes to this bug.