Enforce Permissions-Policy and permission checks for Screen Wake Lock [chromium/src : main]
0 views
Skip to first unread message
Hongchan Choi (Gerrit)
May 29, 2026, 5:22:44 PM (2 days ago) May 29
to Alvin Ji, Chromium LUCI CQ, chromium...@chromium.org, Raphael Kubo da Costa, mattreyno...@chromium.org
Attention needed from Alvin Ji
Hongchan Choi added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Alvin Ji
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alvin Ji (Gerrit)
May 29, 2026, 6:00:42 PM (2 days ago) May 29
to Hongchan Choi, Chromium LUCI CQ, chromium...@chromium.org, Raphael Kubo da Costa, mattreyno...@chromium.org
Attention needed from Hongchan Choi
Alvin Ji voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alvin Ji
| Code-Review | +1 |
4 comments
Patchset-level comments
Alvin Ji . resolved
LGTM
Alvin Ji . resolved
LGTM with nits.
File content/browser/wake_lock/wake_lock_service_impl.cc
Line 39, Patchset 6 (Latest):
if (reason == device::mojom::WakeLockReason::kOther) {Alvin Ji . unresolved
nit: Perhaps we can add a comment explaining `WakeLockReason::kOther` represent requests from web content.
Line 68, Patchset 6 (Latest):
} else {
if (type != device::mojom::WakeLockType::kPreventDisplaySleep &&
type != device::mojom::WakeLockType::kPreventDisplaySleepAllowDimming) {
ReportBadMessageAndDeleteThis(
"Invalid WakeLockType for internal request.");
return;
}
}Alvin Ji . unresolved
nit: Since `kAudioPlayback` wake locks are initiated and managed entirely browser-side (via `MediaWebContentsObserver`), the renderer has no legitimate reason to ever send a `GetWakeLock` request with `reason == WakeLockReason::kAudioPlayback`.
To minimize the attack surface, we should explicitly restrict the `else` branch to only allow `kVideoPlayback`, and trigger `ReportBadMessage` for `kAudioPlayback` or any other unexpected reasons.
Something like:
```
} else if (reason == device::mojom::WakeLockReason::kVideoPlayback) {
if (type != device::mojom::WakeLockType::kPreventDisplaySleep &&
type != device::mojom::WakeLockType::kPreventDisplaySleepAllowDimming) {
ReportBadMessageAndDeleteThis("Invalid WakeLockType for video playback.");
return;
}
} else {
ReportBadMessageAndDeleteThis("Invalid WakeLockReason.");
return;
}
```
Related details
Attention is currently required from:
- Hongchan Choi
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
May 29, 2026, 6:17:45 PM (2 days ago) May 29
to Bo Liu, Alvin Ji, Chromium LUCI CQ, chromium...@chromium.org, Raphael Kubo da Costa, mattreyno...@chromium.org
Attention needed from Bo Liu
Hongchan Choi added 3 comments![]( "Open in Gerrit")
Patchset-level comments
Hongchan Choi . resolved
PTAL
File content/browser/wake_lock/wake_lock_service_impl.cc
nit: Perhaps we can add a comment explaining `WakeLockReason::kOther` represent requests from web content.
Hongchan Choi
Done
Line 68, Patchset 6:
} else {
if (type != device::mojom::WakeLockType::kPreventDisplaySleep &&
type != device::mojom::WakeLockType::kPreventDisplaySleepAllowDimming) {
ReportBadMessageAndDeleteThis(
"Invalid WakeLockType for internal request.");
return;
}
}Alvin Ji . resolved
nit: Since `kAudioPlayback` wake locks are initiated and managed entirely browser-side (via `MediaWebContentsObserver`), the renderer has no legitimate reason to ever send a `GetWakeLock` request with `reason == WakeLockReason::kAudioPlayback`.
To minimize the attack surface, we should explicitly restrict the `else` branch to only allow `kVideoPlayback`, and trigger `ReportBadMessage` for `kAudioPlayback` or any other unexpected reasons.
Something like:
```
} else if (reason == device::mojom::WakeLockReason::kVideoPlayback) {
if (type != device::mojom::WakeLockType::kPreventDisplaySleep &&
type != device::mojom::WakeLockType::kPreventDisplaySleepAllowDimming) {
ReportBadMessageAndDeleteThis("Invalid WakeLockType for video playback.");
return;
}
} else {
ReportBadMessageAndDeleteThis("Invalid WakeLockReason.");
return;
}
```
Attention is currently required from:
- Bo Liu
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
May 31, 2026, 10:45:59 PM (2 hours ago) May 31
to Hongchan Choi, Reilly Grant, Bo Liu, Alvin Ji, Chromium LUCI CQ, chromium...@chromium.org, Raphael Kubo da Costa, mattreyno...@chromium.org
Attention needed from Hongchan Choi and Reilly Grant
Bo Liu added 3 comments![]( "Open in Gerrit")
Patchset-level comments
Bo Liu . resolved
+reillyg as blink wake_lock owner, to take a look that this is all sane
code search is throwing errors, so makes checking things hard, gonna try again tomorrow to see if cs is fixed
File content/browser/wake_lock/wake_lock_service_impl.cc
Line 42, Patchset 8 (Latest):
if (reason == device::mojom::WakeLockReason::kOther) {Bo Liu . unresolved
prefer to use a switch statement without default so this gets recompiled if the enum changes
Line 43, Patchset 8 (Latest):
if (type == device::mojom::WakeLockType::kPreventDisplaySleep) {Bo Liu . unresolved
ditto switch statement
Related details
Attention is currently required from:
- Hongchan Choi
- Reilly Grant
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages