1736049 - We call -[NSCursor set] on every mouse move

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Markus Stange [:mstange]]

We have this code in -[nsCursorManager setMacCursor:]:

  // Some plugins mess with our cursors and set a cursor that even
  // [NSCursor currentCursor] doesn't know about. In case that happens, just
  // reset the state.
  [[NSCursor currentCursor] set];

This means we call -[NSCursor set] on every mouse move. On macOS Monterey, with cursor accessibility coloring enabled, this function call is expensive: https://share.firefox.dev/3FQNKuH

It also leaks memory in the current macOS Monterey Beta, see bug 1735345.

This code is a work-around which I added 12 years ago to work around a problem with plug-ins in bug 496601. We don't support plug-ins anymore, so it should be possible to just remove it. We'll just need to double-check that doing so won't regress bug 496601 or bug 1423275.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1736049 - Stop calling `[[NSCursor currentCursor] set]` on every mouse move. r=#mac-reviewers

This was only necessary when we had binary plug-ins overriding the cursor from under us.
We no longer support plug-ins.

Calling -[NSCursor set] has a performance cost on macOS Monterey when
cursor accessibility coloring is enabled, so we want to avoid calling it
unnecessarily. It also leaks memory in the current Monterey Beta (see
bug 1735345), so calling it less often will leak less memory.

I have checked the testcases of bug 496601 and of bug 1423275, they still
work as expected with this fix.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff6d6594f7b0
Stop calling `[[NSCursor currentCursor] set]` on every mouse move. r=emilio

Comment 3

[Iulian Moraru]

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

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 9246166 [details]
Bug 1736049 - Stop calling [[NSCursor currentCursor] set] on every mouse move. r=#mac-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: There is a new mouse cursor accessibility coloring feature in the upcoming version of macOS, macOS Monterey. However, in the current macOS Beta, this feature has a memory leak, see bug 1735345.
  We don't know when macOS Monterey will be released - it could happen today because there's an Apple event today.

The extremely high memory usage and higher-than-necessary CPU usage only affects users who have mouse cursor accessibility coloring enabled, and it might be fixed by the time macOS Monterey ships, but we don't know that.

This patch mitigates the memory leak, and fixes the higher-than-necessary CPU usage during mouse moves.

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Install the macOS Monterey Beta.

2. In the macOS system preferences, under Accessibility -> Display -> Pointer, customize the pointer stroke or fill color.
3. Use Firefox normally.

Expected results: Regular memory usage, low CPU usage during mouse motions.
Actual results: Extremely high memory usage after a while, medium CPU usage during mouse motions.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch is removing a workaround which was added for plug-ins that were messing with our cursor. It is extremely unlikely that the removal of the workaround could have observable consequences (e.g. missed cursor changes).
• String changes made/needed: none

Comment 5

[Markus Stange [:mstange]]

https://www.apple.com/macos/monterey/ now says that macOS Monterey will be released in 7 days, on October 25th.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 9246166 [details]
Bug 1736049 - Stop calling [[NSCursor currentCursor] set] on every mouse move. r=#mac-reviewers

Given that we don't know whether Apple's fix for the leak will make it into the macOS Monterey release version, the safer assumption is that it won't make it. So we may want to consider this patch for a dot release.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9246166 [details]
Bug 1736049 - Stop calling [[NSCursor currentCursor] set] on every mouse move. r=#mac-reviewers

Fixes a bad memory leak and issues with macOS Monterey shipping next week. Approved for 94.0b8.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/26e15cdf9728

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Are we going to want this on ESR91 also?

Comment 10

[Anca Soncutean, Desktop QA]

Reproduced the issue with Fx 95.0a1 (20211015095004) on macOS 12.0 (beta version 21A5552a).
Verified fixed on the same macOS beta version with Fx 95.0a1 (20211020093007) and Fx 94.0b8.

I will not change the overall status tot verified and keep the qa+ flag until the question from comment 9 will have an answer.

Comment 11

[Markus Stange [:mstange]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Are we going to want this on ESR91 also?

I can't think of a reason not to, so yes, I'll request ESR uplift as well.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 9246166 [details]
Bug 1736049 - Stop calling [[NSCursor currentCursor] set] on every mouse move. r=#mac-reviewers

see comment 4

Comment 13

[Pascal Chevrel:pascalc]

Added to Firefox 93.0 release notes in the unresolved section:

For users on macOS Monterey, enabling mouse cursor coloring in the system accessibility preferences can lead to excessive memory usage in Firefox. This problem will be fixed in Firefox 94 due for release on November 2.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9246166 [details]
Bug 1736049 - Stop calling [[NSCursor currentCursor] set] on every mouse move. r=#mac-reviewers

Approved for 91.3esr.

Comment 15

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/eb35ad1d25cb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9246166 [details]
Bug 1736049 - Stop calling [[NSCursor currentCursor] set] on every mouse move. r=#mac-reviewers

We won't be building a 93.0.1 release. This change will ride 94.

Comment 17

[Anca Soncutean, Desktop QA]

Verified fixed on macOS 12.0.1 (version 21A559) with the treeherder 91.3.0esr build.