From 06e9044b253efb8c60576cb468e0743203221b67 Mon Sep 17 00:00:00 2001 From: Ian Douglas Scott Date: Tue, 10 Dec 2024 14:25:15 -0800 Subject: [PATCH 1/3] Use `WeakOutput` when accessing data of `ZwlrOutputHeadV1` It seems cd9ff0b7bb9ef5559420ac2e91f1e97e837fcd34 broke mirroring. Though it's also a bug that if any of these if conditions is not met, the data will not be initialized. Fixes https://github.com/pop-os/cosmic-epoch/issues/1341. --- .../protocols/output_configuration/handlers/cosmic.rs | 8 +++++--- .../protocols/output_configuration/handlers/wlr.rs | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/wayland/protocols/output_configuration/handlers/cosmic.rs b/src/wayland/protocols/output_configuration/handlers/cosmic.rs index f36d002d..d7145f58 100644 --- a/src/wayland/protocols/output_configuration/handlers/cosmic.rs +++ b/src/wayland/protocols/output_configuration/handlers/cosmic.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-only use smithay::{ - output::{Mode, Output}, + output::Mode, reexports::{ wayland_protocols_wlr::output_management::v1::server::{ zwlr_output_configuration_head_v1::{self, ZwlrOutputConfigurationHeadV1}, @@ -179,7 +179,9 @@ where } => { if let Ok(obj) = obj.upgrade() { if let Some(data) = obj.data::() { - if let Some(output) = mirroring.data::() { + if let Some(output) = + mirroring.data::().and_then(|x| x.upgrade()) + { let mut pending = data.lock().unwrap(); if pending.heads.iter().any(|(h, _)| *h == head) { obj.post_error( @@ -195,7 +197,7 @@ where if let Some(conf) = c.data::() { match conf.lock().unwrap().mirroring.as_ref() { Some(mirrored) => { - head.data::().is_some_and(|o| o == mirrored) // we are already a mirror target -> invalid + head.data::().is_some_and(|o| o == mirrored) // we are already a mirror target -> invalid || *h == mirroring // our target already mirrors -> invalid } None => false, diff --git a/src/wayland/protocols/output_configuration/handlers/wlr.rs b/src/wayland/protocols/output_configuration/handlers/wlr.rs index 1ac7ee19..55b5cfc0 100644 --- a/src/wayland/protocols/output_configuration/handlers/wlr.rs +++ b/src/wayland/protocols/output_configuration/handlers/wlr.rs @@ -234,7 +234,7 @@ where if pending.heads.iter().any(|(_, c)| match c { Some(conf) => { if let Some(output_conf) = conf.data::() { - if let Some(output) = head.data::() { + if let Some(output) = head.data::() { let pending_conf = output_conf.lock().unwrap(); pending_conf.mirroring.as_ref().is_some_and(|o| o == output) } else { From 16617dff0f87ea38025a6142ed72e8a918b025e2 Mon Sep 17 00:00:00 2001 From: Ian Douglas Scott Date: Tue, 10 Dec 2024 14:53:34 -0800 Subject: [PATCH 2/3] output_configuration: Use `unwrap` in calls to `Resource::data` Having if conditions for these is unnecessary when they should never be reached. (This is commonly unwrapped in `smithay`.) Some of these else conditions fail to call `data_init.init` with a new id, so they'd result in a crash later anyway. --- .../output_configuration/handlers/cosmic.rs | 132 ++++++++---------- .../output_configuration/handlers/wlr.rs | 14 +- 2 files changed, 65 insertions(+), 81 deletions(-) diff --git a/src/wayland/protocols/output_configuration/handlers/cosmic.rs b/src/wayland/protocols/output_configuration/handlers/cosmic.rs index d7145f58..b3d23606 100644 --- a/src/wayland/protocols/output_configuration/handlers/cosmic.rs +++ b/src/wayland/protocols/output_configuration/handlers/cosmic.rs @@ -99,10 +99,9 @@ where } } zcosmic_output_manager_v1::Request::GetConfiguration { extended, config } => { - if let Some(pending) = config.data::() { - let obj = data_init.init(extended, config.downgrade()); - pending.lock().unwrap().extension_obj = Some(obj); - } + let pending = config.data::().unwrap(); + let obj = data_init.init(extended, config.downgrade()); + pending.lock().unwrap().extension_obj = Some(obj); } zcosmic_output_manager_v1::Request::GetConfigurationHead { extended, @@ -178,56 +177,49 @@ where mirroring, } => { if let Ok(obj) = obj.upgrade() { - if let Some(data) = obj.data::() { - if let Some(output) = - mirroring.data::().and_then(|x| x.upgrade()) - { - let mut pending = data.lock().unwrap(); - if pending.heads.iter().any(|(h, _)| *h == head) { - obj.post_error( - zwlr_output_configuration_v1::Error::AlreadyConfiguredHead, - format!("{:?} was already configured", head), - ); - return; - } + let data = obj.data::().unwrap(); + if let Some(output) = mirroring.data::().unwrap().upgrade() { + let mut pending = data.lock().unwrap(); + if pending.heads.iter().any(|(h, _)| *h == head) { + obj.post_error( + zwlr_output_configuration_v1::Error::AlreadyConfiguredHead, + format!("{:?} was already configured", head), + ); + return; + } - if pending.heads.iter().any(|(h, c)| { - match c.as_ref() { - Some(c) => { - if let Some(conf) = c.data::() { - match conf.lock().unwrap().mirroring.as_ref() { - Some(mirrored) => { - head.data::().is_some_and(|o| o == mirrored) // we are already a mirror target -> invalid - || *h == mirroring // our target already mirrors -> invalid - } - None => false, - } - } else { - *h == mirroring // unknown state for our mirror target -> invalid + if pending.heads.iter().any(|(h, c)| { + match c.as_ref() { + Some(c) => { + let conf = c.data::().unwrap(); + match conf.lock().unwrap().mirroring.as_ref() { + Some(mirrored) => { + head.data::().unwrap() == mirrored // we are already a mirror target -> invalid + || *h == mirroring // our target already mirrors -> invalid } + None => false, } - None => *h == mirroring, // disabled state for our mirror target -> invalid } - }) { - extension_obj.post_error( - zcosmic_output_configuration_v1::Error::MirroredHeadBusy, - format!("{:?} can't mirror, it is either a mirror target itself or {:?} is not enabled/already mirroring", head, mirroring), - ); + None => *h == mirroring, // disabled state for our mirror target -> invalid } - - let output_conf = PendingOutputConfiguration::default(); - output_conf.lock().unwrap().mirroring = Some(output.clone()); - let conf_head = data_init.init(id, output_conf); - pending.heads.push((head, Some(conf_head))); + }) { + extension_obj.post_error( + zcosmic_output_configuration_v1::Error::MirroredHeadBusy, + format!("{:?} can't mirror, it is either a mirror target itself or {:?} is not enabled/already mirroring", head, mirroring), + ); } + + let output_conf = PendingOutputConfiguration::default(); + output_conf.lock().unwrap().mirroring = Some(output.clone()); + let conf_head = data_init.init(id, output_conf); + pending.heads.push((head, Some(conf_head))); } } } zcosmic_output_configuration_v1::Request::Release => { if let Ok(obj) = obj.upgrade() { - if let Some(data) = obj.data::() { - data.lock().unwrap().extension_obj.take(); - } + let data = obj.data::().unwrap(); + data.lock().unwrap().extension_obj.take(); } } _ => {} @@ -259,40 +251,38 @@ where match request { zcosmic_output_configuration_head_v1::Request::SetScale1000 { scale_1000 } => { if let Ok(obj) = obj.upgrade() { - if let Some(data) = obj.data::() { - let mut pending = data.lock().unwrap(); - if pending.scale.is_some() { - obj.post_error( - zwlr_output_configuration_head_v1::Error::AlreadySet, - format!("{:?} already had a scale configured", obj), - ); - return; - } - pending.scale = Some((scale_1000 as f64) / 1000.0); + let data = obj.data::().unwrap(); + let mut pending = data.lock().unwrap(); + if pending.scale.is_some() { + obj.post_error( + zwlr_output_configuration_head_v1::Error::AlreadySet, + format!("{:?} already had a scale configured", obj), + ); + return; } + pending.scale = Some((scale_1000 as f64) / 1000.0); } } zcosmic_output_configuration_head_v1::Request::SetAdaptiveSyncExt { state } => { if let Ok(obj) = obj.upgrade() { - if let Some(data) = obj.data::() { - let mut pending = data.lock().unwrap(); - if pending.adaptive_sync.is_some() { - obj.post_error( - zwlr_output_configuration_head_v1::Error::AlreadySet, - format!("{:?} already had an adaptive_sync state configured", obj), - ); - return; - } - pending.adaptive_sync = match state.into_result() { - Ok(zcosmic_output_head_v1::AdaptiveSyncStateExt::Always) => { - Some(AdaptiveSync::Force) - } - Ok(zcosmic_output_head_v1::AdaptiveSyncStateExt::Automatic) => { - Some(AdaptiveSync::Enabled) - } - _ => Some(AdaptiveSync::Disabled), - }; + let data = obj.data::().unwrap(); + let mut pending = data.lock().unwrap(); + if pending.adaptive_sync.is_some() { + obj.post_error( + zwlr_output_configuration_head_v1::Error::AlreadySet, + format!("{:?} already had an adaptive_sync state configured", obj), + ); + return; } + pending.adaptive_sync = match state.into_result() { + Ok(zcosmic_output_head_v1::AdaptiveSyncStateExt::Always) => { + Some(AdaptiveSync::Force) + } + Ok(zcosmic_output_head_v1::AdaptiveSyncStateExt::Automatic) => { + Some(AdaptiveSync::Enabled) + } + _ => Some(AdaptiveSync::Disabled), + }; } } _ => {} diff --git a/src/wayland/protocols/output_configuration/handlers/wlr.rs b/src/wayland/protocols/output_configuration/handlers/wlr.rs index 55b5cfc0..4666b114 100644 --- a/src/wayland/protocols/output_configuration/handlers/wlr.rs +++ b/src/wayland/protocols/output_configuration/handlers/wlr.rs @@ -233,16 +233,10 @@ where if pending.heads.iter().any(|(_, c)| match c { Some(conf) => { - if let Some(output_conf) = conf.data::() { - if let Some(output) = head.data::() { - let pending_conf = output_conf.lock().unwrap(); - pending_conf.mirroring.as_ref().is_some_and(|o| o == output) - } else { - false - } - } else { - false - } + let output_conf = conf.data::().unwrap(); + let output = head.data::().unwrap(); + let pending_conf = output_conf.lock().unwrap(); + pending_conf.mirroring.as_ref().is_some_and(|o| o == output) } None => false, }) { From b2490983d17bbbfb98c716d6111f2484bdac178f Mon Sep 17 00:00:00 2001 From: Ian Douglas Scott Date: Tue, 10 Dec 2024 14:56:57 -0800 Subject: [PATCH 3/3] Call `data_init.init` even if weak references are not alive --- .../protocols/output_configuration/handlers/cosmic.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/wayland/protocols/output_configuration/handlers/cosmic.rs b/src/wayland/protocols/output_configuration/handlers/cosmic.rs index b3d23606..b0b768df 100644 --- a/src/wayland/protocols/output_configuration/handlers/cosmic.rs +++ b/src/wayland/protocols/output_configuration/handlers/cosmic.rs @@ -213,7 +213,13 @@ where output_conf.lock().unwrap().mirroring = Some(output.clone()); let conf_head = data_init.init(id, output_conf); pending.heads.push((head, Some(conf_head))); + } else { + let output_conf = PendingOutputConfiguration::default(); + data_init.init(id, output_conf); } + } else { + let output_conf = PendingOutputConfiguration::default(); + data_init.init(id, output_conf); } } zcosmic_output_configuration_v1::Request::Release => {