1563918 - Massive performance issue after maximizing/windowing when pointer/mouse cursor is still at maximize/window button and blinking/flickering of maximize/window button after landing patch from bug #1551961

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: screencast.mp4

STR:

1. Open Mozilla Firefox browser
2. Use windowed mode but enlarge browser window like in maximized mode
3. Press maximize/window button few times
   and enjoy massive performance issue when pointer/mouse cursor is still at maximize/window button and blinking/flickering of maximize/window button

Moving pointer/mouse cursor from maximize/window button fixes this issue.

Gecko Profiler profile:
https://perfht.ml/2YCEV1B

Regression caused by:
Bug #1551961

Differential Revision:
https://phabricator.services.mozilla.com/D36060

@ Mike Conley (:mconley) - Seems like your patch caused this regression, could you please look at it? Thanks!

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Thanks, Virtual.

Does this build show any difference for you?

32-bit: https://queue.taskcluster.net/v1/task/LaPiwNkpQPmAb-CnDkYHGg/runs/0/artifacts/public/build/target.zip
64-bit: https://queue.taskcluster.net/v1/task/FeudReCGTvmlcZmDyEF2HA/runs/0/artifacts/public/build/target.zip

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

No, none, I can still reproduce this issue on this build ( https://hg.mozilla.org/try/rev/e1f42b28e3f78d7f3cdcf690a5e924c667ef1e68 ).

Comment 3

[Mike Conley (:mconley) (:⚙️)]

No dice reproducing this on Windows 10. Dusting off my Windows 7 VM to see if I can reproduce there.

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Graphics section from about;support.txt

I'm attaching Graphics section from about:support, maybe this will be some clue, like disabled WebRender etc.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Okay, I could reproduce this easily on Windows 7. Starting investigation...

Comment 6

[Mike Conley (:mconley) (:⚙️)]

I think I've figured this out. Strangely, using WindowFromPoint via WindowAtMouse here causes us to re-enter the WM_NCMOUSELEAVE message handler, so we just keep processing WM_NCMOUSELEAVE again and again and again.

We've got an alternative function, EventIsInsideWindow defined here that does much the same thing as WindowAtMouse, but does so without WindowFromPoint. Using that, I can no longer reproduce the issue. I'll have a patch posted tomorrow for testing.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Hey Virtual, are you able to reproduce the issue with this build?:

32-bit: https://queue.taskcluster.net/v1/task/DlkMnb_OTiaoqsryrfZT1A/runs/0/artifacts/public/build/target.zip
64-bit: https://queue.taskcluster.net/v1/task/BreQiHh2Qke4QbiNyfVV2A/runs/0/artifacts/public/build/target.zip

Comment 8

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

No, I can't reproduce this bug anymore with build from comment #7.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Great, thanks!

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1563918 - Avoid spamming native message loop with WM_NCMOUSELEAVE messages. r?jmathies

In bug 1551961, a WM_NCMOUSELEAVE message handler was added to the Windows widget
backend so that we can detect when the mouse leaves a draggable region (since
draggable regions are hittest'ed as HTCAPTION, and are therefore "non-client").

Inside that message handler, we used the WindowAtMouse handler to determine whether
or not the WM_NCMOUSELEAVE was firing because the mouse was moving off of the
non-client area to client area, or moving out of the window entirely.

For reasons that are not particularly clear, on Windows Aero Glass, when clicking on
one of the min/max/close caption buttons, a WM_NCMOUSELEAVE message fires, and the
::WindowFromPoint call that WindowAtMouse uses causes another WM_NCMOUSELEAVE message
to be queued immediately after, so we end up in this situation where the message
loop is getting spammed by WM_NCMOUSELEAVE messages. Moving the mouse away from
the caption buttons seems to stop the spamming.

We have a function similar to WindowAtMouse called EventIsInsideWindow which
does not use ::WindowFromPoint, and this seems to sidestep the issue, while
being functionally equivalent.

Depends on D37421

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d303fb1c4374
Avoid spamming native message loop with WM_NCMOUSELEAVE messages. r=jmathies

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d303fb1c4374

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Hey Virtual,

Can you verify that this is now fixed in the most recent Nightly? If so, I'll request uplift to beta. Thanks!

Comment 14

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Sure. I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 70.0a1 (2019-07-11), so I'm marking this bug as VERIFIED.
Thank you very much one more time! \o/

P.S. In your free time please see also bug #1564540, which I'm suspecting that it's also caused by bug #1551961.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Commented in bug 1564540.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9076812 [details]
Bug 1563918 - Avoid spamming native message loop with WM_NCMOUSELEAVE messages. r?jmathies

Beta/Release Uplift Approval Request

• User impact if declined: Users using Windows Aero on Windows 7 can get into situations where the browser window becomes unresponsive while their mouse cursor is over the caption (min/max/close) buttons.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This works around a weird quirk in how a win32 API (WindowFromPoint) works, by using a different method that accomplishes the same task but without hitting the quirk. (The quirk is what made us spam the message loop with WM_NCMOUSELEAVE messages).
• String changes made/needed: None.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9076812 [details]
Bug 1563918 - Avoid spamming native message loop with WM_NCMOUSELEAVE messages. r?jmathies

Fixes a big perf regression in 69 for some users. Approved for 69.0b6.

Comment 18

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d81eefb325ca

Comment 19

[Rares Doghi, Desktop QA]

This issue was Verified in Beta and Nightly, I will remove the qeverify+ flags.