Closed Bug 1736049 Opened 3 years ago Closed 3 years ago

We call -[NSCursor set] on every mouse move

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect

Tracking

()

VERIFIED FIXED
95 Branch
Tracking Status
firefox-esr91 --- verified
firefox93 + wontfix
firefox94 --- verified
firefox95 --- verified

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(1 file)

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.

Blocks: 1735345

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.

Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/ff6d6594f7b0
Stop calling `[[NSCursor currentCursor] set]` on every mouse move. r=emilio
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch

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.
  1. In the macOS system preferences, under Accessibility -> Display -> Pointer, customize the pointer stroke or fill color.
  2. 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
Attachment #9246166 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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

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.

Attachment #9246166 - Flags: approval-mozilla-release?
QA Whiteboard: [qa-triaged]

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.

Attachment #9246166 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Are we going to want this on ESR91 also?

Flags: needinfo?(mstange.moz)

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.

(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.

Flags: needinfo?(mstange.moz)

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

see comment 4

Attachment #9246166 - Flags: approval-mozilla-esr91?

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 on attachment 9246166 [details]
Bug 1736049 - Stop calling [[NSCursor currentCursor] set] on every mouse move. r=#mac-reviewers

Approved for 91.3esr.

Attachment #9246166 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+

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.

Attachment #9246166 - Flags: approval-mozilla-release? → approval-mozilla-release-

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1739352
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: