[Mac] Guard null FocusManager deref in OnWindowKeyStatusChanged [chromium/src : main]
Yuanjun Zhu (Gerrit)
Yuanjun Zhu added 1 comment![]( "Open in Gerrit")
Hi @a...@chromium.org, @kere...@chromium.org,
Could you please help review this change? I'm investigating an Edge Mac browser-process crash in NativeWidgetMac::OnWindowKeyStatusChanged and it traces to ui/views/widget/native_widget_mac.mm.
The focus-restore behavior is correct — but removing the if (!is_content_first_responder) { return; } early-return exposed a latent null-deref a layer down, and I'd love your eyes on the fix.
A synchronous re-entrant key-window notification (child widget reparented + shown in one turn → makeKeyAndOrderFront: → key-status re-entry) hits exactly that window, and the unguarded deref crashes.
I don't have manual repro steps — it's a low-rate timing race surfaced from Edge 149 (Stable) crash telemetry. But I added a deterministic views_unittests case (NativeWidgetMacTest.KeyStatusChangeWithNoTopLevelDoesNotCrash) that reproduces it: it builds a child widget, detaches its host parent so GetFocusManager() returns null, then drives OnWindowKeyStatusChanged(). It crashes (SIGSEGV) on both branches without the fix and passes with it.
Thanks in advance.
Related details
- Avi Drissman
- Keren Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Avi Drissman
| Code-Review | +1 |
1 comment
Seems reasonable; approving for when Keren also approves
Related details
- Keren Zhu
- Yuanjun Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu voted Code-Review+1![]( "Open in Gerrit")
- Yuanjun Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuanjun Zhu (Gerrit)
Yuanjun Zhu voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Mac] Guard null FocusManager deref in OnWindowKeyStatusChanged
A re-entrant key-status notification can reach a child widget whose
top-level linkage is transiently broken during reparent+show (e.g. a
tab-modal dialog on tab activation). Widget::GetFocusManager() then
returns null and the unguarded deref crashes. Guard both the restore
(is_key) and store (else) branches; OnNativeFocus()/OnNativeBlur() side
effects are preserved.
Adds NativeWidgetMacTest.KeyStatusChangeWithNoTopLevelDoesNotCrash,
which crashes on either branch without the fix and passes with it.
- M ui/views/widget/native_widget_mac.mm
- M ui/views/widget/native_widget_mac_unittest.mm
Code-Review: +1 by Avi Drissman, +1 by Keren Zhu
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |