1386308 - Stop trying to alter display management (sleep) code in content processes

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[Alex Gaynor [:Alex_Gaynor]]

Currently when we want to affect power management (e.g. disable display sleeping because you're watching a video), this is done in both the content and parent process, as far as I can tell.

This happens here: https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsAppShell.mm#74-78

We should stop trying to do this in the content process, as it triggers the "com.apple.PowerManagement.control" mach-service permission. If we remove the permission, everything will keep working, since the parent process duplicates the work, but it's silly and a waste of CPU cycles :-)

We should stop using MacWakeLockListener in the content process, and then remove the "com.apple.PowerManagement.control" mach permission.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsAppShell.mm#667-682 is where we set it up -- but I'm still learning the code to know if it's more appropriate to stop using nsAppShell, or to just wrap AddScreenWakeLockListener in `if (XRE_IsParentProcess())`.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This sounds like the Mac equivalent of bug 1360069.

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1386308 - stop trying to change the display sleep settings from the content process;

Before this change we were trying to change the settings from both the content
and parent processes, so this doesn't change any functionality. This allows to
remove access to the com.apple.PowerManagement.control mach service from the
content process.

Review commit: https://reviewboard.mozilla.org/r/164506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164506/

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8022074b4efd4d6259dbf0d4e734a3b5c579f08e&group_state=expanded

Comment 5

[Haik Aftandilian [:haik]]

Comment on attachment 8893387 [details]
Bug 1386308 - stop trying to change the display sleep settings from the content process;

https://reviewboard.mozilla.org/r/164506/#review169958

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a73050e842c2
stop trying to change the display sleep settings from the content process; r=haik

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Dumb question, but are we forwarding wake lock requests from the content process to the parent process? Since requests to disable the screen saver while playing video come from the content process.

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

I didn't look at the exact mechanism it was happening by, but I confirmed that all the requests to disable sleep were happening in both content and parent, so there shouldn't be a behavior change here.