JonnyHaystack / i3-resurrect

Simple solution to saving and restoring i3 workspaces
GNU General Public License v3.0
379 stars 19 forks source link

Programs that modify their own cmdline are not restored #55

Closed JonnyHaystack closed 4 years ago

JonnyHaystack commented 4 years ago

Describe the bug Sometimes processes rewrite their own cmdline which often means they are no longer separated by the NUL character, which causes issues running the command.

See https://github.com/JonnyHaystack/i3-resurrect/issues/35#issuecomment-552984191

JonnyHaystack commented 4 years ago

Really can't think of any way to solve this that doesn't cause even more issues.. Frustrating. For now I guess the only way around it is to create a custom command mapping in the i3-resurrect config file.

eater commented 4 years ago

I'm not 100% sure I know what you mean by "modify their own cmdline" but in the case of Signal (for instance), the command that i3-resurrect saves is the same one that I type into the terminal. If I launch Signal with /opt/Signal/signal-desktop then the JSON programs file contains:

  "command": [
      "/opt/Signal/signal-desktop"
    ],

If I run /opt/Signal/signal-desktop --no-sandbox in the terminal, then the saved JSON looks like

  "command": [
      "/opt/Signal/signal-desktop --no-sandbox"
    ],
JonnyHaystack commented 4 years ago

This shouldn't be closed oops.

Anyways, the cmdline (which is retrieved from /proc/<pid>/cmdline is usually supposed to be an array of arguments separated by null-bytes. As you can see in the json, the command is an array. If you have written a command line program before, you will know that command line arguments are passed as an array e.g. argv in C.

When you run /opt/Signal/signal-desktop --no-sandbox, argv initially looks like this:

["/opt/Signal/signal-desktop", "--no-sandbox"]

But for some reason it modifies its own cmdline (argv) and puts everything in argv[0]. This means now argv looks like:

["/opt/Signal/signal-desktop --no-sandbox"]

Which is what you see in the <workspace>_programs.json file.

I don't know of a reliable way to detect if this has happened. If I did, then I could easily make it split the command up correctly.

eater commented 4 years ago

Got it, thanks. And it would be too much of an assumption for i3-resurrect to consider any component of a command line beginning with a -- to be a separate argument and split it?

JonnyHaystack commented 4 years ago

That would only solve part of the problem.

The best trade-off for now is probably to assume that if the cmdline has one argument, I should try to split it. If I did that it would work for almost every case, except it would not work for the case where there is only one argument and that argument has spaces (i.e. the path to the executable contains spaces).

I think it's fairly uncommon to have an excutable path that contains spaces on Linux, but it's hard to make assumptions with all the crazy things linux users do :smile:

For now I will do this, and those people who run executables with no arguments and spaces in the excutable path can make custom command mappings. It seems like your issue is a lot more common and unfeasible to solve with custom command mappings, because the new command mapping cmdline interpolation feature won't even work if the cmdline has been modified.

eater commented 4 years ago

I have no idea if I'm a typical user, but just glancing at my setup I have two annoying exceptions already. All the Chrome windows have multiple arguments:

    "command": [
      "/opt/google/chrome/chrome --profile-directory=Default --app=http://instacalc.com --user-data-dir=.config"
    ],

and one of my everyday apps has a space in the path:

    "command": [
      "/opt/Pulse SMS/pulse-sms"
    ],
JonnyHaystack commented 4 years ago

Yeah so if I make this change, those Chrome ones will be fixed, but the Pulse SMS one will be broken. But with the Pulse SMS one you can add a window command mapping to your config like so:

{
  "window_command_mappings": [
    {
      "class": "<the window class of pulse sms>",
      "command": "/opt/Pulse SMS/pulse-sms"
    }
  ]
}

And then both will work fine.

eater commented 4 years ago

Sounds good! I thought what you were saying might mean that commands with multiple arguments (like Chrome) would need custom editing.

JonnyHaystack commented 4 years ago

Yeah it's an important goal of this project that you shouldn't ever have to manually edit the program/layout files, which is why I instead try to make the configuration as flexible as possible.

By the way, just realised the example mapping I just gave is wrong, it should be either:

{
  "window_command_mappings": [
    {
      "class": "<the window class of pulse sms>",
      "command": ["/opt/Pulse SMS/pulse-sms"]
    }
  ]
}

or

{
  "window_command_mappings": [
    {
      "class": "<the window class of pulse sms>",
      "command": "'/opt/Pulse SMS/pulse-sms'"
    }
  ]
}

So that you are explicitly specifying it as one argument.

eater commented 4 years ago

For some reason it's still not splitting this one (it's splitting all the rest!)

    "command": [
      "/opt/google/chrome/chrome --profile-directory=Default --app=http://instacalc.c
om"
    ],
JonnyHaystack commented 4 years ago

Is that definitely the latest version from the master branch?

JonnyHaystack commented 4 years ago

Someone I was talking to about this problem had the idea to check if the single argument is an existing executable in order to know if it should be split or not. This would be nice as it would not require me to break saving of all single argument processes with spaces in the executable path. I'll reopen this until that's implemented.

eater commented 4 years ago

Yes, just pulled. And there's another weird new glitch: If I have multiple Chrome apps open, they both get saved with the first one's command line. So I launch opt/google/chrome/chrome --profile-directory=Default --app=http://instacalc.com and opt/google/chrome/chrome --profile-directory=Default --app=http://192.168.50.106:9002 and save and get

    "command": [
      "/opt/google/chrome/chrome --profile-directory=Default --app=http://instacalc.com"
    ],
    "working_directory": "/home/eater"
  },
  {
    "command": [
      "/opt/google/chrome/chrome --profile-directory=Default --app=http://instacalc.com"
    ],
    "working_directory": "/home/eater"
  }
JonnyHaystack commented 4 years ago

That's very odd, the cmdline should be split... I'm not sure how it's even possible that they are ending up in one string unless you have added a mapping to your i3-resurrect config.

eater commented 4 years ago

OK, just pulled and built 1.4.1 and it's still not right. Let me know if you'd rather I put this stuff in a different issue.

Now all the command line arguments are getting removed entirely from Chrome windows:

  {
    "command": [
      "/opt/google/chrome/chrome"
    ],
    "working_directory": "/home/eater"
  },
  {
    "command": [
      "/opt/google/chrome/chrome"
    ],
    "working_directory": "/home/eater"
  },

But not, I tested, from Chromium windows:

  {
    "command": [
      "/usr/lib/chromium-browser/chromium-browser",
      "--enable-pinch",
      "--app=https://calendar.google.com/calendar/render?pli=1#main_7",
      "--user-data-dir=.config/google-calendar"
    ],
    "working_directory": "/home/eater"
  },

And this is my ~/.config/i3-resurrect/config.json, which I've never modified:


{
  "window_command_mappings": [
    {
      "class": "Gnome-terminal",
      "command": "gnome-terminal"
    }
  ],
  "window_swallow_criteria": {},
  "terminals": [
    "Gnome-terminal",
    "Alacritty"
  ]
}
JonnyHaystack commented 4 years ago

There must be something different about Chrome's cmdline then. Can you try doing the hexdump of a chrome process's cmdline like you did for Signal here https://github.com/JonnyHaystack/i3-resurrect/issues/35#issuecomment-552987947

eater commented 4 years ago
00000000  2f 6f 70 74 2f 67 6f 6f  67 6c 65 2f 63 68 72 6f  |/opt/google/chro|
00000010  6d 65 2f 63 68 72 6f 6d  65 20 2d 2d 70 72 6f 66  |me/chrome --prof|
00000020  69 6c 65 2d 64 69 72 65  63 74 6f 72 79 3d 44 65  |ile-directory=De|
00000030  66 61 75 6c 74 20 2d 2d  61 70 70 3d 68 74 74 70  |fault --app=http|
00000040  3a 2f 2f 69 6e 73 74 61  63 61 6c 63 2e 63 6f 6d  |://instacalc.com|
00000050  00 00 00 00 00 00 00 00                           |........|
00000058
JonnyHaystack commented 4 years ago

Ok what I notice first there is that there's a whole bunch of null bytes at the end instead of just one. psutil expects only one null character at the end of the string, and will remove that null character, then it splits by the null character. This means that command is split into an array containing the entire command as the first argument, then a load of empty strings afterwards. Then i3-resurrect sees it's got multiple arguments so it assumes it's a valid command and does not split it further, but it does remove all the empty string elements from the array, which is why you don't see them in the programs file, and it sets cmdline[0] = exe, which is why you ended up with only the executable path.

All I need to do is add a line to remove empty strings from the cmdline before I process it, not just afterwards.

eater commented 4 years ago

I see a new commit so I tried it. Now back to the case where all the saved Chrome windows have the same command line flags (they all have the same flags as the first one I launch) even though they were launched with different --app= flags:

    "command": [
      "/opt/google/chrome/chrome",
      "--profile-directory=Default",
      "--app=http://instacalc.com"
    ],
    "working_directory": "/home/eater"
  },
  {
    "command": [
      "/opt/google/chrome/chrome",
      "--profile-directory=Default",
      "--app=http://instacalc.com"
    ],

OR

    "command": [
      "/opt/google/chrome/chrome",
      "--profile-directory=Default",
      "--app=http://192.168.50.106:9002"
    ],
    "working_directory": "/home/eater"
  },
  {
    "command": [
      "/opt/google/chrome/chrome",
      "--profile-directory=Default",
      "--app=http://192.168.50.106:9002"
    ],
JonnyHaystack commented 4 years ago

That might just be what chrome does (on your system at least). Does it do that in 1.3.2 as well? You could also check the cmdline for each of the processes to see if they are in fact set to the same thing.

If this is the case, I can't change that. However, what you can do is create class + title -> command mappings like so:

"window_command_mappings": [
  {
    "class": "Google-chrome",
    "title": "infocalc.com",
    "command": [
      "/opt/google/chrome/chrome",
      "--profile-directory=Default",
      "--app=http://instacalc.com"
    ]
  },
  {
    "class": "Google-chrome",
    "title": http://192.168.50.106:9002",
    "command": [
      "/opt/google/chrome/chrome",
      "--profile-directory=Default",
      "--app=http://192.168.50.106:9002"
    ]
  }
]

Of course this is just an example, and the class/titles are probably not exactly what I have written. You may even be able to use instance instead of title, because I noticed before that on your system Chrome seems to set the instance depending on the page that is open.

eater commented 4 years ago

Weird, you're right, I just noticed it yesterday but apparently it's a systemwide Chrome weirdness.