CRD multi-process Linux: Fetch systemd environment in desktop process [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
Related details
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lambros Lambrou (Gerrit)
Lambros Lambrou added 1 comment![]( "Open in Gerrit")
systemd environment before the desktop process is launched. So, instead
of launching a separate helper process (`user_systemd_env`), the desktop
process now directly fetches these variables using a new
`SystemdUserEnvFetcher` utility class.This approach feels fragile. We are doing some Chromium initialization, then changing the local environment, then doing more initialization.
I would prefer that we launch a clean process with the changed environment. Otherwise we are trying to split the Chromium initialization into 2 parts, and I don't think the Chromium tooling was designed for this use-case. Even if it works today, I can see Chromium changing in ways that break us.
We don't need a separate process type or command-line. We could do this:
```
if WAYLAND_DISPLAY,DISPLAY not found in env:
wait for them to show up in systemd
exec() with modified environment (maybe no need to fork)
else
Initialize and run desktop process normally
end
```
This way, the 2nd process falls into the "else" case, because it has the correct environment set by the 1st process.
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
systemd environment before the desktop process is launched. So, instead
of launching a separate helper process (`user_systemd_env`), the desktop
process now directly fetches these variables using a new
`SystemdUserEnvFetcher` utility class.This approach feels fragile. We are doing some Chromium initialization, then changing the local environment, then doing more initialization.
I would prefer that we launch a clean process with the changed environment. Otherwise we are trying to split the Chromium initialization into 2 parts, and I don't think the Chromium tooling was designed for this use-case. Even if it works today, I can see Chromium changing in ways that break us.
We don't need a separate process type or command-line. We could do this:
```
if WAYLAND_DISPLAY,DISPLAY not found in env:
wait for them to show up in systemd
exec() with modified environment (maybe no need to fork)
else
Initialize and run desktop process normally
end
```This way, the 2nd process falls into the "else" case, because it has the correct environment set by the 1st process.
We are doing some Chromium initialization, then changing the local environment, then doing more initialization.
None of the Chromium initialization code relies on the systemd environment variables. Only things like `gtk_init()` do, but that's our code. I could technically reorganize the code so that Chromium initialization code goes before the systemd setter.
Even if it works today, I can see Chromium changing in ways that break us.
Could you give an example?
Or I can switch to use libsystemd synchronized D-Bus helpers instead, which does not need Chromium initialization and I can move it to the top of the main function. WDYT?
Related details
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lambros Lambrou (Gerrit)
Lambros Lambrou added 1 comment![]( "Open in Gerrit")
systemd environment before the desktop process is launched. So, instead
of launching a separate helper process (`user_systemd_env`), the desktop
process now directly fetches these variables using a new
`SystemdUserEnvFetcher` utility class.Yuwei HuangThis approach feels fragile. We are doing some Chromium initialization, then changing the local environment, then doing more initialization.
I would prefer that we launch a clean process with the changed environment. Otherwise we are trying to split the Chromium initialization into 2 parts, and I don't think the Chromium tooling was designed for this use-case. Even if it works today, I can see Chromium changing in ways that break us.
We don't need a separate process type or command-line. We could do this:
```
if WAYLAND_DISPLAY,DISPLAY not found in env:
wait for them to show up in systemd
exec() with modified environment (maybe no need to fork)
else
Initialize and run desktop process normally
end
```This way, the 2nd process falls into the "else" case, because it has the correct environment set by the 1st process.
We are doing some Chromium initialization, then changing the local environment, then doing more initialization.
None of the Chromium initialization code relies on the systemd environment variables. Only things like `gtk_init()` do, but that's our code. I could technically reorganize the code so that Chromium initialization code goes before the systemd setter.
Even if it works today, I can see Chromium changing in ways that break us.
Could you give an example?
Or I can switch to use libsystemd synchronized D-Bus helpers instead, which does not need Chromium initialization and I can move it to the top of the main function. WDYT?
The example I had in mind was: we create UI threads and a `RunLoop` before `DISPLAY` or `WAYLAND_DISPLAY` are set. That might work today, but in future the implementation might try to connect to a Wayland server.
We might address this by only using IO threads. But after we set the env variables, we want the proper UI threads. Does Chromium let us change the type of the main thread mid-process?
Another issue is that `setenv()` is not safe with multiple threads. In `base/environment.h`, there is a warning about this for `SetVar()`
The libsystemd helpers might work if we can use them before any Chromium initialization. That seems more effort than calling exec() to restart the process with the correct environment.
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
systemd environment before the desktop process is launched. So, instead
of launching a separate helper process (`user_systemd_env`), the desktop
process now directly fetches these variables using a new
`SystemdUserEnvFetcher` utility class.Yuwei HuangThis approach feels fragile. We are doing some Chromium initialization, then changing the local environment, then doing more initialization.
I would prefer that we launch a clean process with the changed environment. Otherwise we are trying to split the Chromium initialization into 2 parts, and I don't think the Chromium tooling was designed for this use-case. Even if it works today, I can see Chromium changing in ways that break us.
We don't need a separate process type or command-line. We could do this:
```
if WAYLAND_DISPLAY,DISPLAY not found in env:
wait for them to show up in systemd
exec() with modified environment (maybe no need to fork)
else
Initialize and run desktop process normally
end
```This way, the 2nd process falls into the "else" case, because it has the correct environment set by the 1st process.
Lambros LambrouWe are doing some Chromium initialization, then changing the local environment, then doing more initialization.
None of the Chromium initialization code relies on the systemd environment variables. Only things like `gtk_init()` do, but that's our code. I could technically reorganize the code so that Chromium initialization code goes before the systemd setter.
Even if it works today, I can see Chromium changing in ways that break us.
Could you give an example?
Or I can switch to use libsystemd synchronized D-Bus helpers instead, which does not need Chromium initialization and I can move it to the top of the main function. WDYT?
The example I had in mind was: we create UI threads and a `RunLoop` before `DISPLAY` or `WAYLAND_DISPLAY` are set. That might work today, but in future the implementation might try to connect to a Wayland server.
We might address this by only using IO threads. But after we set the env variables, we want the proper UI threads. Does Chromium let us change the type of the main thread mid-process?
Another issue is that `setenv()` is not safe with multiple threads. In `base/environment.h`, there is a warning about this for `SetVar()`
The libsystemd helpers might work if we can use them before any Chromium initialization. That seems more effort than calling exec() to restart the process with the correct environment.
Just switched to use the libsystemd functions. It wasn't difficult at all with the help of Gemini :)
Related details
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lambros Lambrou (Gerrit)
Lambros Lambrou voted and added 4 comments![]( "Open in Gerrit")
Votes added by Lambros Lambrou
| Code-Review | +1 |
4 comments
struct SdBusError {Can this object be safely copied? If so, can you add the default copy-operations to document this? Otherwise, please make this move-only.
sd_bus_error* get() { return &error; }Since the returned ptr can never be null, it's more idiomatic to return an object reference, and to name the getter method as `error()` to match the field name. I feel that `get()` is more suited to classes that wrap a pointer, rather than an object. Up to your discretion, since my suggestion does make the call-site more ugly 😊
sd_bus_error* operator->() { return &error; }Is this used anywhere? This seems confusing as the class doesn't behave like a smart pointer.
env_map[std::string(split_result->first)] = split_result->second;Do you need the explicit cast to `std::string` here?
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 6 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +2 |
6 comments
Can this object be safely copied? If so, can you add the default copy-operations to document this? Otherwise, please make this move-only.
Done
Can this object be safely copied? If so, can you add the default copy-operations to document this? Otherwise, please make this move-only.
Done. Deleted the copy constructor.
Since the returned ptr can never be null, it's more idiomatic to return an object reference, and to name the getter method as `error()` to match the field name. I feel that `get()` is more suited to classes that wrap a pointer, rather than an object. Up to your discretion, since my suggestion does make the call-site more ugly 😊
Acknowledged.
I feel that `get()` is more suited to classes that wrap a pointer
Whether it wraps a pointer feels like an implementation detail to me, and the caller shouldn't care. An example of this pattern can be found [here](https://source.chromium.org/chromium/chromium/src/+/main:base/allocator/partition_allocator/src/partition_alloc/partition_alloc_base/no_destructor.h;l=110;drc=4dfdbe14e150479629e696c5b0ce8e9140ce0c70).
Is this used anywhere? This seems confusing as the class doesn't behave like a smart pointer.
Yes, in `error->message`. Again, I don't think callers should care whether the scoped object is a smart pointer or not, and this class is anonymous anyway. This pattern can be found [here](https://source.chromium.org/gn/gn/+/main:src/base/value_iterators.h;l=40;drc=69ec4fca1fa69ddadae13f9e6b7507efa0675263).
env_map[std::string(split_result->first)] = split_result->second;Do you need the explicit cast to `std::string` here?
Yes. There is no implicit string_view->string conversion by design.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
16 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: remoting/host/linux/systemd_user_env_setter.cc
Insertions: 2, Deletions: 0.
@@ -39,6 +39,8 @@
})>;
struct SdBusError {
+ SdBusError() = default;
+ SdBusError(const SdBusError&) = delete;
~SdBusError() {
// This only frees resources held by `error`, but not `error` itself.
sd_bus_error_free(&error);
```
Change information
CRD multi-process Linux: Fetch systemd environment in desktop process
With the session reporter logic removed, there is no need to fetch the
systemd environment before the desktop process is launched. So, instead
of launching a separate helper process (`user_systemd_env`), the desktop
process now directly fetches these variables via a new
`SetSystemdUserEnvironment` helper function.
`SetSystemdUserEnvironment` connects to the user's session bus, polls
for the systemd environment, and waits for the display variables to be
set. The fetched environment variables are then applied to the desktop
process's environment. This eliminates the need for the
`kProcessTypeUserSystemdEnv` process type and associated switches.
- M remoting/host/base/switches.cc
- M remoting/host/base/switches.h
- M remoting/host/desktop_process_main.cc
- M remoting/host/host_main.cc
- M remoting/host/linux/BUILD.gn
- M remoting/host/linux/desktop_session_factory_linux.cc
- M remoting/host/linux/remote_display_session_manager.cc
- M remoting/host/linux/remote_display_session_manager.h
- A remoting/host/linux/systemd_user_env_setter.cc
- A remoting/host/linux/systemd_user_env_setter.h
- D remoting/host/linux/user_systemd_env.cc
Code-Review: +1 by Lambros Lambrou
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |