Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

session-lock: lock the screen even if the client provides no lock surface #2550

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

session-lock: lock the screen even if the client provides no lock surface #2550

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7b4f78d
session-lock: lock the screen even if the client provides no lock sur…
dkondor Dec 27, 2024
1c5d5d4
session-lock: define crashed urface in one location
dkondor Feb 15, 2025
a79d191
session-lock: ensure that the crashed surface is displayed even if th…
dkondor Feb 15, 2025
4dc41b8
session-lock: address review
dkondor Mar 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 34 additions & 16 deletions plugins/protocols/session-lock.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,20 +134,29 @@ class lock_crashed_node : public lock_base_node<simple_text_node_t>
set_size(output->get_screen_size());
}

void display()
void display(std::string text)
{
wf::cairo_text_t::params params(
1280 /* font_size */,
wf::color_t{0, 0, 0, 1} /* bg_color */,
wf::color_t{0.9, 0.9, 0.9, 1} /* fg_color */);
set_text_params(params);
// TODO: make the text smaller and display a useful message instead of a big explosion.
set_text("💥");
set_text(text);
auto layer_node = output->node_for_layer(wf::scene::layer::LOCK);
wf::scene::add_back(layer_node, shared_from_this());
if (parent() == nullptr)
{
wf::scene::add_back(layer_node, shared_from_this());
}

wf::get_core().seat->set_active_node(shared_from_this());
}

void display_crashed()
{
display("💥");
}

// Ensure pointer interaction is not passed to views behind this node.
std::optional<wf::scene::input_node_t> find_node_at(const wf::pointf_t& at) override
{
Expand Down Expand Up @@ -253,7 +262,7 @@ class wf_session_lock_plugin : public wf::plugin_interface_t
output_states[output]->surface_node.reset();
if (output_states[output]->crashed_node)
{
output_states[output]->crashed_node->display();
output_states[output]->crashed_node->display_crashed();
}
}

Expand Down Expand Up @@ -285,6 +294,12 @@ class wf_session_lock_plugin : public wf::plugin_interface_t
{
disconnect_signals();
set_state(state == UNLOCKED ? DESTROYED : ZOMBIE);
if (state == ZOMBIE)
{
// ensure that the crashed node is displayed in this case as well
lock_all();
}

LOGC(LSHELL, "session lock destroyed");
});
destroy.connect(&lock->events.destroy);
Expand All @@ -310,17 +325,11 @@ class wf_session_lock_plugin : public wf::plugin_interface_t
void handle_output_added(wf::output_t *output)
{
output_states[output] = std::make_shared<output_state>(output);
if (state == LOCKED)
if ((state == LOCKED) || (state == ZOMBIE))
{
lock_output(output, output_states[output]);
}

if (state == ZOMBIE)
{
output->set_inhibited(true);
output_states[output]->crashed_node->display();
}

output->connect(&output_changed);
}

Expand All @@ -346,12 +355,17 @@ class wf_session_lock_plugin : public wf::plugin_interface_t
void lock_output(wf::output_t *output, std::shared_ptr<output_state> output_state)
{
output->set_inhibited(true);
if (output_state->surface_node)
if (state == ZOMBIE)
{
output_state->crashed_node->display_crashed();
} else if (output_state->surface_node)
{
output_state->surface_node->display();
} else
{
// if the surface node has not yet been displayed we show an empty surface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we hide the crashed node once the actual lock node is there? I think for example if one wanted to implement a blurred lockscreen, or animate the lockscreen surface, this would prevent it from working.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially, however, after doing some tests with the current implementation, I'm a bit confused how this should happen:

  • with Hyprlock: things already seem to "work": the crashed node disappears, and fade-in starts from the unlocked desktop
  • with shaderlock: the unlocked screen flashes briefly (which is weird, since I assumed that crashed node should be there), but then the crashed lock screen comes back and is the basis of further animations

This is especially strange given that both projects seem to use the wlr-screencopy protocol to get a screenshot of the current desktop contents. Do you happen to know of another lockscreen app that does not make a screenshot, but just simply displays a (partially) transparent surface? That would allow me to test this more cleanly.

On a second thought, actually it could even make sense to not show the desktop after the previous lockscreen has already crashed. Contrary to when originally locking the screen, it could be a security / privacy issue if the screen contents are revealed when restarting a crashed lockscreen (however briefly). But then it is very weird why the screen shows up with shaderlock, I'll investigate that separately a bit more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a guess, maybe they take the screenshot at different times?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that can be.

Now I realized what we have here does not actually matter, since a new instance is created when a new lock client connects:

wayfire/plugins/protocols/session-lock.cpp

Line 460 in 4dc41b8

cur_lock.reset(new wayfire_session_lock(this, wlr_lock));

So that would also explain why the desktop becomes visible even with shaderlock (I assume it takes the screenshot before creating the new lock then). Maybe it would make sense to refactor this so that the crashed node is kept until the new client creates a lock surface and thus the screen cannot be revealed accidentally. However, that would be a bigger change and IMO unrelated to the current PR -- I can create a separate issue and try writing a patch for it if you think this can be an issue. Note that I've only seen this with shaderlock, probably because it is a bit slow to display its initial lock surface (e.g. swaylock appears fast enough). In any case, this is likely not a common issue, since most lockscreens do not crash, and even in that case, the problem manifests only if there is some automated way to restart them which could trigger when an "unauthorized" user is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I've only seen this with shaderlock, probably because it is a bit slow to display its initial lock surface (e.g. swaylock appears fast enough).

Actually, I've just tried gtklock and it is also slow enough that the unlocked desktop flashes, but in this case really briefly (barely possible to notice it).

output_state->crashed_node->display("");
}

// TODO: if the surface node has not yet been displayed, display... something?
}

void lock_all()
Expand All @@ -361,8 +375,12 @@ class wf_session_lock_plugin : public wf::plugin_interface_t
lock_output(output, output_state);
}

wlr_session_lock_v1_send_locked(lock);
set_state(LOCKED);
if (state != ZOMBIE)
{
wlr_session_lock_v1_send_locked(lock);
set_state(LOCKED);
}

LOGC(LSHELL, "lock");
}

Expand Down