[Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure

Wed Jan 16 10:52:41 UTC 2019

Hi,

On Wed, Jan 16, 2019 at 02:33:49PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jan 16, 2019 at 1:18 PM Victor Toso <victortoso at redhat.com> wrote:
> >
> > Hi,
> >
> > On Wed, Jan 16, 2019 at 01:07:27PM +0400, Marc-André Lureau wrote:
> > > > Not true. Possible client-side bug, reproducible on X11, as I
> > > > discussed before. As mentioned in the comment, we are sending
> > > > wrong selection-grab to the guest.
> > >
> > > Sorry, I don't follow "we are sending wrong selection-grab" ?
> >
> > I think I tried too hard already to make you understand that on
> > X11, that's happening while user is copying in the guest VM.
> 
> Sorry, I don't know what you are talking about. When there is a
> complicated question/reply, you should update the comments or
> commit message, so we don't have to dig in old conversations.

Old... Yesterday and the day before and last week...

    https://lists.freedesktop.org/archives/spice-devel/2019-January/047192.html

Commit log:

    A way to reproduce is copy-paste big clipboard data between
    two spice clients.

It is missing 'on x11'.

> > > > > Not sure sending a reply is going to help much in that case...
> > > >
> > > > It helps. Instead of blocking the application, it fails to paste
> > > > the clipboard and all is good.
> > >
> > > But if the agent logic is already wrong, sending a reply isn't going
> > > to help, it could do anything..
> >
> > It might have been a valid request at the time it was sent, if
> > you accept that we sent the selection-grab.
> 
> "we" == the client?
>
> 
> In this case, clipboard_by_guest[] should be false.

I give up.... Feel free to ignore.

> 
> >
> > > > > > +     * sent a bad selection-grab to the agent or the agent is buggy. */
> > > > > > +    if (s->clipboard_by_guest[selection]) {
> > > > > > +        SPICE_DEBUG("clipboard: agent request: grab on hold by agent, possible race");
> > > > > > +        goto notify_agent;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* The selection-request by agent should happen only if the clipboard data is set
> > > > > > +     * by client */
> > > > > > +    if (!s->clip_grabbed[selection]) {
> > > > > > +        SPICE_DEBUG("clipboard: agent request: data set by agent, possible race");
> > > > > > +        goto notify_agent;
> > > > > > +    }
> > > > >
> > > > > This could be adding more race conditions. clip_grabbed is set
> > > > > asynchronously after owner changed, and indicate if the grab
> > > > > message was sent to the agent,
> > > >
> > > > We send a selection-release on owner-change. If we receive a
> > > > selection-request before agent receives the selection-release,
> > > > this is expected but we should notify the guest, afaics.
> > > >
> > > > > as you correctly say. It doesn't mean you can't request client
> > > > > clipboard content.
> > > >
> > > > > I understand the racy case, but the condition seems wrong, it
> > > > > should attempt to request current client clipboard content, and
> > > > > fail/succeed after.
> > > >
> > > > No. It should request the content from the grab that was sent. If
> > >
> > > There is no such thing as clipboard ID/nth in any clipboard protocol I
> > > know of. User simply request current clipboard content (not the
> > > nth), whether it changed since last grab is not important: it
> > > will retrieve something of the requested type or nothing.
> >
> > Not about the id, but selection metadata itself or the @type
> > parameter in clipboard_request()
> 
> If the clipboard content (of the requested type) can't be retrieve, it
> will fail. It shouldn't be handled here.
> 
> >
> > > > clipboard changed, previous grab gets invalidated and a
> > > > selection-release is sent and another selection-grab is sent with
> > > > the metadata of the new grab.
> > > >
> > > > Proving recent data from old grab seems wrong.
> > >
> > > It is not.
> > >
> > > > >
> > > > > > +    /* Ready only, still should reply to agent to avoid it waiting for data */
> > > > >
> > > > > No, read-only shouldn't reply. We are lacking a way to tell the guest
> > > > > that the client is read-only though. So this may be acceptable for
> > > > > now, but we should have a TODO/FIXME..
> > > >
> > > > Well, it should not happen anyway as on read-only we should not
> > > > send selection-grab. I'll fix and resend.
> > >
> > > Right
> > >
> > > >
> > > > Thanks for the quick review.
> > > >
> > > > > > +    if (read_only(self)) {
> > > > > > +        g_warning("clipboard: agent request: read only, deny request");
> > > > > > +        goto notify_agent;
> > > > > > +    }
> > > > > >
> > > > > >      if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
> > > > > >          gtk_clipboard_request_text(cb, clipboard_received_text_cb,
> > > > > > @@ -1039,6 +1057,10 @@ static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
> > > > > >      }
> > > > > >
> > > > > >      return TRUE;
> > > > > > +
> > > > > > +notify_agent:
> > > > > > +    spice_main_channel_clipboard_selection_notify(s->main, selection, type, NULL, 0);
> > > > > > +    return FALSE;
> > > > > >  }
> > > > > >
> > > > > >  static void clipboard_release(SpiceMainChannel *main, guint selection,
> > > > > > --
> > > > > > 2.20.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > Spice-devel mailing list
> > > > > > Spice-devel at lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Marc-André Lureau
> > >
> > >
> > >
> > > --
> > > Marc-André Lureau
> 
> 
> 
> -- 
> Marc-André Lureau
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190116/4178a273/attachment.sig>