Session suspension and restoration protocol

[Roman Gilg]

On Tue, Jun 19, 2018 at 11:19 AM Markus Ongyerth <wl at ongy.net> wrote:
>
> Hey Roman,
>
> first a general remark:
> I don't see any mention of which state is supposed to be restored. The
> workspace (virtual desktops), geometry (session-restore), just client-side
> window (the embedded usecase)?
> Is this left implementation-defined intentionally? If so, I think it would be
> nice to have a note about that.
Should allow restoration of workspace and geometry by compositor and
client-side window by client.

> While the sleep version of this has a way for a client ot overwrite and
> restore itself, I'm missing that functionality on quit/shutdown.
> The current state is always compositor triggered on those. While it might make
> sense for e.g. multi-window applications or other fancy things to use a
> restoration protocol (at least for window geometry) on their own startup.
> It *should* work as is, but I'd like to see it explicitly.

The rational was that as soon as the client has agreed to the
quit/shutdown state, it should have no business anymore to change
this. Because in general it is meant to now quit the client connection
and its own process only waiting for the Restore call by the
compositor. If there is no process, it can no longer cancel the state.
So it's not allowed. A client which will quit itself after some
internal process ended should therefore not go into the quit state,
otherwise it might get restored at some later point although it has
nothing more to do.

> On 2018/6月/18 05:05, Roman Gilg wrote:
> > * using D-Bus interface only to secure against sandboxed clients
> What? Why exactly? When I first read this, I expected that the client is
> supposed to use the portal stuff to call out of a sandbox, but reading through
> the actual protocol, I think the only usage of D-Bus is to launch the
> applications from the compositor?
> Since this needs explicit client support either way, IMO this could be
> implmented by adding a required cmdline argument (e.g.
> --org-freedsktop-restore=UID) instead.
> Parsing .desktop files, and launching applications in a helper shouldn't bee
> too much of a burdon.
> I know that GNOME/KDE employ D-Bus either way, but others want to avoid it as
> much as possible.

The first draft was written exactly this way: parse the desktop file,
execute the Exec line with a special argument. But GNOME people said,
that D-Bus Activation should be used. And I think they are right in
theory. But if there is a large potion of potential users not willing
to support D-Bus Activation one could offer the possibility to execute
the Exec line or do D-Bus Activation. Client and compositor just need
to make sure that they support whatever method they want to use (and
if they don't use at least one of the methods at the same time, don't
try to use the protocol). Should be easy to do with another event in
the manager interface and another argument to its
register_session_suspension call. And personally I would at least
recommend in the specs to use D-Bus Activation instead of Exec.

> >
> >     <enum name="error">
> >       <entry name="already_exists" value="0"
> >              summary="an object for the reference already exists"/>
> >       <entry name="defunct_children" value="1"
> >              summary="parent object destroyed before child objects"/>
> >       <entry name="suspension_violation" value="1"
> value="2" might be better to differentiate this from defunct_children :)
> >              summary="unallowed request sent while session suspended"/>
> I'd suggest another entry:
>                 <entry name="unsupported_suspension" value="3"
>                                 summary="client tried to create a suspesion object that's not
>                                 uspported by the compositor"/>

My plan in this case was to just not send the respective event (i.e
let the client create the sleep object but not send the sleep event if
the compositor does not support sleep). But probably it's nicer to put
out an error directly.

> >         [0] https://specifications.freedesktop.org/desktop-entry-spec
> >       </description>
> >       <arg name="id" type="new_id" interface="zxdg_session_suspension"
> >            summary="register a new restore session"/>
> >       <arg name="restore_id" type="uint"
> >            summary="for restoration must match client_id of previous session"/>
> What if there is no previous session?
Then it should be 0. If we decide to use strings instead, it's an
empty string. Needs to get mentioned in the description though.

> >         Calling this request on an xdg_toplevel for which a zxdg_session_surface
> >         object already exists results in a protocol error.
> >       </description>
> >       <arg name="id" type="new_id" interface="zxdg_session_surface"
> >            summary="session surface object"/>
> >       <arg name="surface" type="object" interface="xdg_toplevel"/>
> >     </request>
> What's your rational to have the surface_restore_id server allocated and sent
> as event? This should be in a "client namespace" (i.e. bound to the
> protocol-global restore_id) either way, so the client can savely manage them
> as well.

You are right, that the surface ids could be generated by the client,
since they are bound by the restore_id. In this case the compositor
would need to make sure though, that the client does not send for
different surfaces the same id. Maybe there doesn't need to be sent
any id and client and compositor can just agree on counting upwards
starting at 1 for every new surface registered?

> >     <request name="restore_surface">
> >       <description summary="ask to restore the surface">
> >         Requests the provided xdg_toplevel surface to be restored with the data
> >         the compositor has saved for the surface_restore_id in combination with
> >         the restore_id set earlier in the call to create the
> >         zxdg_session_suspension object.
> >
> >         If the compositor does not find any data for this restore_id,
> >         surface_restore_id combination, it will ignore the request without
> >         further notice.
> >
> >         This request must be sent before the first commit to the xdg_toplevel
> >         and before a register_surface request for the surface has been sent,
> >         otherwise the request will be ignored by the compositor.
> >
> >         Calling this request while the client is in a suspension state results
> >         in a protocol error.
> >
> >         If the compositor applies the saved settings associated with the
> >         surface_restore_id it will eventually only send a normal configure
> >         event to the xdg_toplevel.
> >       </description>
> >       <arg name="surface" type="object" interface="xdg_toplevel"/>
> >       <arg name="surface_restore_id" type="uint"
> >            summary="must match surface_id from before suspension"/>
> I would guess (really no solid data) that clients that restore would like to
> register the freshly restored surfaces to upcoming restoration as well, so I'd
> create a fresh zxdg_session_surface here as well.
I think it's cleaner to do this explicitly in a second call to register_surface.

> >     <enum name="suspension_level" bitfield="true">
> >         <description summary="TODO">
> >           Enum bitfield of session suspension levels. Used in the registered
> >           event to indicate which levels the compositor supports.
> >         </description>
> >       <entry name="sleep" value="0">
> >       </entry>
> >       <entry name="quit" value="1">
> >       </entry>
> >       <entry name="shutdown" value="2">
> >       </entry>
> >     </enum>
>
> >     <event name="registered">
> >       <description summary="client state being tracked">
> Most protocols that have such an event have a line like "this is emitted on
> object creation" and a note about whether it can be emitted at a later point
> if some internal state changes.
> It should probably also mention if this can be emitted multiple times with
> changes supported_levels or changed (non-zero) client_id.

Very true. Must be added to the description. It's meant to be
submitted only once on object creation.

> >   <interface name="zxdg_session_sleep" version="1">
> >     <description summary="opt-in to client sleep">
> >       While this object exists, the client is registered to the server as being
> >       ready to go into the sleep suspension state.
> >
> >       The sleep suspension state is active after the client has received the
> >       sleep event and untill it receives the wakeup event or it destroys this
> >       object.
> >
> >       If the client registered to a higher level of suspension as well, it must
> >       be ready to go directly from this state into the higher level without
> >       receiving the wakeup event before and therefore without restoring its
> >       surfaces in between. The sleep state is implicitly unset in this case.
> >     </description>
> >
> >     <request name="destroy" type="destructor">
> >       <description summary="destroy the sleep object">
> >         Used by the client to notify the server that it will no longer use this
> >         object. That means the client can no longer go into the sleep state via
> >         this object.
> >
> >         If the sleep state is active while this request is sent the client will
> >         have the opportunity to recover its suspended surfaces.
> I'm a bit unhappy with this. Just naivly looking at it, it forces the
> compositor to keep the state, even if the destroy request was sent.
> I think something along the lines of
> "if a client wants to wake up on its own, it starts the sequence as if woken
> up. This will put the xdg_session_sleep object into an inert state, and it
> should be destroyed after full restoration. It will no longer be used to send
> events and any other request will result in an error"
One could say the client is implicitly woken up. But yea, if the sleep
object gets destroyed the compositor doesn't know when it can release
the surface restoration information. I mean it's not infinite because
the compositor does know that it can destroy them on
session_suspension object destruction (if the client did not went into
quit or shutdown state in between).

Maybe another problem is though what happens when the client requests
to restore a surface in this case while at the same time the
compositor wants to send it into quit, shutdown or (after a new sleep
object has been create) in sleep state again. I don't think the
sequence "as if woken up" solves this problem. How is this sequence
defined and when does it end? When all surfaces are restored? What if
the client doesn't do that? Maybe instead there needs to be a separate
wakeup_now request that allows the client to get out of sleep state,
make the object inert and can restore its surfaces until it destroys
the sleep object. This rule should apply to the wakeup event as well.

> >   <interface name="zxdg_session_quit" version="1">
> >     <description summary="opt-in to connection quitting">
> >       While this object exists, the client is registered to the server as being
> >       ready to go into the quit suspension state.
> >
> >       The quit state becomes active, when the client responds with an ack
> >       request on receiving the quit event. The client can cancel going
> >       into the quit state by sending the destroy request instead of ack. If the
> >       client does not react to the quit event the compositor might deem it
> >       broken and cancel any suspension by sending the client_id 0 through the
> >       registered event of the zxdg_session_suspension interface. In order to
> >       make sure that the server did not deem the client broken, the client
> >       should therefore listen for one more event cycle that this did not
> >       happen before destroying the zxdg_session_suspension interface.
> All of this is a bit weird and conoluted IMO. This protocol requries the
> client to use xdg-shell either way, so it's guaranteed that the PING-PONG
> mechanism in that protocol is supported by the client. I don't think it's a
> good idea to add another way to become unresponsive here. Especially because
> there's some weird restrictions on it.
If we can reuse the xdg-shell ping-pong, sure let's go for it.

> >       interface and its child interfaces or a D-Bus signal to ressurect before
> >       that. In this case the quit state is implicitly unset and the client is
> >       restarted only in a new Wayland session and possibly after a reboot of
> >       the whole system.
> I'm a bit confused here. How would the client even get another state change?
> IF I understand this correctly, then the client is supposed to shut down
> itself, so there's no way for the compositor to send events, right?
Correct. The client would never get another state change. One could
say it basically gets out of the shutdown/quit state when the
compositor dbus-activates it again (but then having a new client
connection it's not the same client anymore so the client from before
never gets out).