[remoting] Fix DesktopSessionAgentTest crash on POSIX [chromium/src : main]
Joe Downing (Gerrit)
Joe Downing voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joe Downing
| Auto-Submit | +1 |
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
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")
It's weird that this hadn't fail the trybot. I suspect a different code path is reached when the socket already exists and needs to be deleted.
In addition to this change, I wonder if we should call [SetSecurityKeySocketName](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/security_key/security_key_auth_handler_posix.h) to point to a temp file. The test may pass without this change, but it will destroy the SSH socket of your CRD session.
Related details
- Joe Downing
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. |
Joe Downing (Gerrit)
Joe Downing added 1 comment![]( "Open in Gerrit")
It's weird that this hadn't fail the trybot. I suspect a different code path is reached when the socket already exists and needs to be deleted.
In addition to this change, I wonder if we should call [SetSecurityKeySocketName](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/security_key/security_key_auth_handler_posix.h) to point to a temp file. The test may pass without this change, but it will destroy the SSH socket of your CRD session.
Hah, you're right. I tested it and then reconnected and my socket was fine but then running the test again showed it was cleaned up. I'll follow up once I have a better solution to review.
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 2 comments![]( "Open in Gerrit")
Is it just for fixing `DesktopSessionAgentTest`? I thought simply calling `SecurityKeyAuthHandlerPosix::SetSecurityKeySocketName` with a temporary path would work? Alternatively you could either pass an empty string to `SetSecurityKeySocketName()`, or just revert to the previous behavior where the default socket name is an empty string instead of [GetDefaultSecurityKeySocketName](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/security_key/security_key_auth_handler_posix.cc;l=66;drc=52fc662d9aba70cf27ffef3b0246cdb00f3dd59b); either of these will disable the SK handler.
base::FilePath security_key_socket_name = options.security_key_socket_name();I'm not sure if I understand this change. For the multi-process host, the security key socket name is initialized to the default path, which is `$XDG_RUNTIME_DIR/crd_ssh_auth_sock` ([cs](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/security_key/security_key_auth_handler_posix.cc;l=43;drc=52fc662d9aba70cf27ffef3b0246cdb00f3dd59b)). `kAuthSocknameSwitchName` is always empty in the network process.
If you find this confusing, you could make it so that the default path is not automatically applied, and instead needs to be passed to `SecurityKeyAuthHandlerPosix::SetSecurityKeySocketName()`, which is reverting to the behavior prior to my change.
Related details
- Joe Downing
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. |
Joe Downing (Gerrit)
Joe Downing voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joe Downing
| Auto-Submit | +0 |
2 comments
Is it just for fixing `DesktopSessionAgentTest`? I thought simply calling `SecurityKeyAuthHandlerPosix::SetSecurityKeySocketName` with a temporary path would work? Alternatively you could either pass an empty string to `SetSecurityKeySocketName()`, or just revert to the previous behavior where the default socket name is an empty string instead of [GetDefaultSecurityKeySocketName](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/security_key/security_key_auth_handler_posix.cc;l=66;drc=52fc662d9aba70cf27ffef3b0246cdb00f3dd59b); either of these will disable the SK handler.
I wasn't ready to ask you to review this, please ignore until I close the original thread and ask you to take a look ^_^
base::FilePath security_key_socket_name = options.security_key_socket_name();I'm not sure if I understand this change. For the multi-process host, the security key socket name is initialized to the default path, which is `$XDG_RUNTIME_DIR/crd_ssh_auth_sock` ([cs](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/security_key/security_key_auth_handler_posix.cc;l=43;drc=52fc662d9aba70cf27ffef3b0246cdb00f3dd59b)). `kAuthSocknameSwitchName` is always empty in the network process.
If you find this confusing, you could make it so that the default path is not automatically applied, and instead needs to be passed to `SecurityKeyAuthHandlerPosix::SetSecurityKeySocketName()`, which is reverting to the behavior prior to my change.
- 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")
Haha, I did wonder if it was vibe coding. Please add me back to the attention list once you are ready :)
Related details
- Joe Downing
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. |
Joe Downing (Gerrit)
Joe Downing added 1 comment![]( "Open in Gerrit")
Haha, I did wonder if it was vibe coding. Please add me back to the attention list once you are ready :)
It was generated by an agent and I uploaded to review, suffice to say I have provided feedback on the approach and am iterating ^_^
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. |
Joe Downing (Gerrit)
Joe Downing added 2 comments![]( "Open in Gerrit")
Joe DowningIt's weird that this hadn't fail the trybot. I suspect a different code path is reached when the socket already exists and needs to be deleted.
In addition to this change, I wonder if we should call [SetSecurityKeySocketName](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/security_key/security_key_auth_handler_posix.h) to point to a temp file. The test may pass without this change, but it will destroy the SSH socket of your CRD session.
Hah, you're right. I tested it and then reconnected and my socket was fine but then running the test again showed it was cleaned up. I'll follow up once I have a better solution to review.
Done
PTAL!
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing added 1 comment![]( "Open in Gerrit")
FYI, I've updating the BUILD files so the new functions are not built on Windows which will address the build error.
Yuwei Huang (Gerrit)
Yuwei Huang added 3 comments![]( "Open in Gerrit")
namespace remoting {From AI:
Macros are evaluated by the preprocessor and do not respect C++ namespaces. Wrapping `#define` inside `namespace remoting { ... }` is misleading and unnecessary. You should remove the `namespace remoting {` and `} // namespace remoting` lines.
// Copyright 2026 The Chromium AuthorsI wonder if we should just move `remoting/host/host_export.h` to `remoting/base`. You are already using `HOST_IMPLEMENTATION` in this file, so I wouldn't worry about having "host" in the filename.
static void SetSecurityKeySocketName(I wonder if this is still useful given you can override the default socket name with the function in `remoting/base/security_key_socket_name.h`? Is it easier to just remove "Default" from the names of those functions then remove the getter and setter in this file?
Related details
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. |
Joe Downing (Gerrit)
Joe Downing added 4 comments![]( "Open in Gerrit")
From AI:
Macros are evaluated by the preprocessor and do not respect C++ namespaces. Wrapping `#define` inside `namespace remoting { ... }` is misleading and unnecessary. You should remove the `namespace remoting {` and `} // namespace remoting` lines.
Done
I wonder if we should just move `remoting/host/host_export.h` to `remoting/base`. You are already using `HOST_IMPLEMENTATION` in this file, so I wouldn't worry about having "host" in the filename.
I think that's fine but I'd prefer to do that in a follow-up.
I wonder if this is still useful given you can override the default socket name with the function in `remoting/base/security_key_socket_name.h`? Is it easier to just remove "Default" from the names of those functions then remove the getter and setter in this file?
Acknowledged - I'd prefer to keep the getter/setter in the class which uses it and only split out the 'default' or initial value. Kinda like how we often handle constants or or initial values. The only reason the default value is in base is to keep the deps cycle clean, otherwise it would have been in this dir.
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
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
| Code-Review | +1 |
1 comment
if (is_posix) {Should it guard the whole `source_set` instead?
Related details
- Joe Downing
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. |
Joe Downing (Gerrit)
Joe Downing voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joe Downing
| Commit-Queue | +2 |
2 comments
Should it guard the whole `source_set` instead?
Sure, that's fine.
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
13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: remoting/base/BUILD.gn
Insertions: 2, Deletions: 2.
@@ -55,8 +55,8 @@
}
}
-source_set("security_key_socket_name") {
- if (is_posix) {
+if (is_posix) {
+ source_set("security_key_socket_name") {
sources = [
"security_key_socket_name.cc",
"security_key_socket_name.h",
```
Change information
[remoting] Fix DesktopSessionAgentTest crash on POSIX
DesktopSessionAgent tries to initialize the security key auth handler
on POSIX but the handler requires an IO message pump. This is not the
default so the test would fail. This CL updates the test environment
to provide one.
- M remoting/BUILD.gn
- M remoting/base/BUILD.gn
- A remoting/base/base_export.h
- M remoting/base/run_all_unittests.cc
- A remoting/base/security_key_socket_name.cc
- A remoting/base/security_key_socket_name.h
- M remoting/host/BUILD.gn
- M remoting/host/desktop_session_agent_unittest.cc
- M remoting/host/linux/remote_session_info_main.cc
- M remoting/host/security_key/BUILD.gn
- M remoting/host/security_key/security_key_auth_handler_posix.cc
- M remoting/host/security_key/security_key_auth_handler_posix.h
Code-Review: +1 by Yuwei Huang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |