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.

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

Clipboard (croutonclip) doesn't work with Firefox, Nautilus, etc (a solution included here) #4922

Open
darrell-k opened this issue Jan 10, 2023 · 4 comments

Comments

@darrell-k
Copy link

I found that clipboard sync doesn't work for some applications in the chroot. For example, copying from gedit works, but from Firefox and Nautilus doesn't.

The cause is that xsel hangs when the X session is not active, but only for some source applications. To demonstrate, copy some data in gedit (Gnome) or mousepad (XFCE), then switch to chromeos, launch a crosh terminal, enter-chroot then
DISPLAY=:1 xsel -ob
This works. But do the same but sourcing the clipboard data from Firefox, and you will see the xsel command hangs.

This happens in bullseye, in both the gnome and xfce targets. It's not a specific crouton problem - I tried the same on a standard laptop running Debian natively and got the same behaviour when copying in the Gnome session and running xsel after switching to a VT.

So, we need to capture the clipboard data before croutoncycle switches to chromeos. I made the following changes to achieve this:

croutoncycle:

@@ -148,6 +148,15 @@ else
     done
 fi
 
+# copy clipboard to temp file because xsel hangs after switch out of X when data has been
+# copied by certain applications (eg Firefox, Nautilus). croutonclip also changed
+# to take clip data from this file rather than running xsel.
+if [ "$curdisp" != 'cros' ]; then
+       if [ "$cmd" = 'n' -o "$cmd" = 'p' ]; then
+               DISPLAY=$curdisp xsel -ob > /tmp/croutonclipdata
+       fi
+fi
+
 # List the displays if requested
 if [ "$cmd" = 'l' -o "$cmd" = 'd' ]; then
     for disp in $displist; do

croutonclip:

@@ -42,7 +42,7 @@ copyclip() {
             # Check if display is still running
             if rundisplay "$current" xdpyinfo >/dev/null 2>&1; then
                 echo -n 'R'
-                rundisplay "$current" xsel -ob
+                cat /tmp/croutonclipdata
             else
                 echo -n "EUnable to open display '$current'."
             fi

I hope this is of some use to people.

@dnschneid
Copy link
Owner

Thanks for figuring this out! I wonder if there's a way to fix this without keeping the contents of the clipboard exposed in /tmp.

I thought croutonclip/croutoncycle was already implemented to copy the clipboard when you switch, but maybe it's happening too late. @drinkcat might know.

@darrell-k
Copy link
Author

darrell-k commented Jan 10, 2023

It looks to me that by the time croutonclip is activated by the signal from croutonswich, the switch has already happened. Is it possible to move it up? Would we need to have croutoncycle wait for croutonclip to do its work? If so, maybe better to combine into a single process, but this is probably overkill for a project which is now in maintenance only.

Otherwise, maybe put the temp file in /home/chronos/user through another mount, or instead of using a temp file, write the clipboard contents from croutonswitch into a new fifo, then move this on to the websocket in croutonclip.

BTW, I debugged xsel, and the hang is happening deep within the X libraries.

@drinkcat
Copy link
Collaborator

drinkcat commented Jan 16, 2023

Sigh, I had prepared an answer to that and forgot to press comment ,-( Anyway, thanks for the trip down memory lane, I used to know how that thing works... ,-)

croutonclip gets nudged here in croutoncycle (https://github.com/dnschneid/crouton/blob/master/chroot-bin/croutoncycle#L283)... It doesn't look easy to move the nudge above the test (line 171), as croutonclip relies on the current display...

I don't think a fifo would work either, as it would probably block the writer if you write too much data to it (or actually, maybe any data...).

Putting the clipboard content in a shell variable might work (but I'm also worried it'll mangle content, we could base64 it if needed?!)... In that case we could split croutonclip in 2 parts: one handler for USR1 pre-switch that puts the content in a variable, and another one for USR2 post-switch.

Worth a shot, happy to review.

@darrell-k
Copy link
Author

Yes, a write to an intermediate fifo does block, and if you launch it into the background (&) that keeps croutoncycle alive after croutonclip is triggered with the USR1 signal, but crouton clip needs to call croutoncycle to determine the display, so locking problems.

The issue in a nutshell is that xsel needs the X display to be the current display for it to work when the clipboard data is sourced from some applications, but as you say croutonclip requires the current display to be the target display.

I tried sending a USR2 signal from croutoncycle before the display switching to tell croutonclip to do the copy into a shell variable instead of calling the copyclip() function, but the problem is that after sending the signal croutoncycle continues on and invariably beats croutonclip to the punch. Implementing some horrible wait in croutoncycle until a signal is received back from croutonclip leads to similar "deadly embrace" problems as using a fifo.

So, without a redesign of the whole process, I think the best solution is the one I suggested above, amended slightly to get rid of the /tmp file as soon as its contents are used in croutonclip, and moving the addition to croutoncycle so we don't have to test $cmd:

croutoncycle:

@@ -216,6 +216,13 @@ if [ "$destdisp" = "$curdisp" -a -z "$force" ]; then
     exit 0
 fi
 
+# copy clipboard to temp file because xsel hangs after switch out of X when data has been
+# copied by certain applications (eg Firefox, Nautilus). croutonclip also changed
+# to take clip data from this file rather than running xsel.
+if [ "$curdisp" != 'cros' ]; then
+    DISPLAY=$curdisp xsel -ob > $PIPEDIR/clipboard 
+fi
+
 # Determine if the target display is on a VT
 if [ "${destdisp#:}" = "$destdisp" ]; then
     if [ "$destdisp" != 'cros' ]; then

and croutonclip:

@ -42,10 +42,11 @@ copyclip() {
             # Check if display is still running
             if rundisplay "$current" xdpyinfo >/dev/null 2>&1; then
                 echo -n 'R'
-                rundisplay "$current" xsel -ob
+                cat $PIPEDIR/clipboard
             else
                 echo -n "EUnable to open display '$current'."
             fi
+            rm $PIPEDIR/clipboard
         fi
     } | (
         STATUS="`head -c 1`"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants