remoting: More refactoring for security key [chromium/src : main]
0 views
Skip to first unread message
Yuwei Huang (Gerrit)
Jun 23, 2026, 5:44:00 PM (5 days ago) Jun 23
to android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Yuwei Huang added 2 comments![]( "Open in Gerrit")
File remoting/host/security_key/security_key_auth_handler.cc
Line 11, Patchset 4 (Latest):
#include "base/check.h"Yuwei Huang . unresolved
NIT: `#include <utility>`, `#include "base/check.h"`, and `#include "base/no_destructor.h"` appear to be unused in this file.
Line 19, Patchset 4 (Latest):
#include "base/task/sequenced_task_runner.h"Yuwei Huang . unresolved
NIT: `#include "base/task/sequenced_task_runner.h"` and `#include "base/task/thread_pool.h"` are also unused here since `Create()` no longer takes a task runner and just calls `std::make_unique<SecurityKeyAuthHandlerPosix>()`. They can be safely removed.
Related details
Attention set is empty
Submit Requirements:
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)
Jun 23, 2026, 5:48:54 PM (5 days ago) Jun 23
to android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Yuwei Huang added 2 comments![]( "Open in Gerrit")
File remoting/host/security_key/security_key_auth_handler.cc
NIT: `#include <utility>`, `#include "base/check.h"`, and `#include "base/no_destructor.h"` appear to be unused in this file.
Yuwei Huang
Done
NIT: `#include "base/task/sequenced_task_runner.h"` and `#include "base/task/thread_pool.h"` are also unused here since `Create()` no longer takes a task runner and just calls `std::make_unique<SecurityKeyAuthHandlerPosix>()`. They can be safely removed.
Yuwei Huang
Done
Related details
Attention set is empty
Submit Requirements:
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)
Jun 23, 2026, 6:16:34 PM (5 days ago) Jun 23
to Lambros Lambrou, Joe Downing, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Lambros Lambrou
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Lambros Lambrou
Submit Requirements:
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)
Jun 23, 2026, 7:26:51 PM (5 days ago) Jun 23
to Chromium LUCI CQ, Lambros Lambrou, Joe Downing, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Lambros Lambrou
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Yuwei Huang . resolved
Looks like I'll need to make one more change..
Related details
Attention is currently required from:
- Lambros Lambrou
Submit Requirements:
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)
Jun 23, 2026, 7:27:00 PM (5 days ago) Jun 23
to Lambros Lambrou, Chromium LUCI CQ, Joe Downing, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Yuwei Huang removed Lambros Lambrou from this change![]( "Open in Gerrit")
Submit Requirements:
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)
Jun 23, 2026, 7:27:01 PM (5 days ago) Jun 23
to Joe Downing, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Yuwei Huang removed Joe Downing from this change![]( "Open in Gerrit")
Deleted Reviewers:
- Joe Downing
Related details
Attention set is empty
Submit Requirements:
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)
Jun 24, 2026, 12:36:05 AM (5 days ago) Jun 24
to Lambros Lambrou, Joe Downing, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Lambros Lambrou
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 9 (Latest):
Yuwei Huang . resolved
This CL is ready for review. PTAL thanks!
Related details
Attention is currently required from:
- Lambros Lambrou
Submit Requirements:
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)
Jun 24, 2026, 1:39:22 PM (4 days ago) Jun 24
to Yuwei Huang, Joe Downing, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Yuwei Huang
Lambros Lambrou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lambros Lambrou
| Code-Review | +1 |
1 comment
File remoting/host/security_key/security_key_auth_handler_posix.h
Line 67, Patchset 11 (Latest):
friend class SecurityKeyAuthHandlerPosixTest;Lambros Lambrou . unresolved
Instead of friending a test class and using a private ctor, could you use a factory method for testing? Something like:
```
public:
static SecurityKeyAuthHandlerPosix MakeForTesting(...);
```
Related details
Attention is currently required from:
- Yuwei Huang
Submit Requirements:
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)
Jun 24, 2026, 3:08:25 PM (4 days ago) Jun 24
to Lambros Lambrou, Joe Downing, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Yuwei Huang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
2 comments
Patchset-level comments
File remoting/host/security_key/security_key_auth_handler_posix.h
Instead of friending a test class and using a private ctor, could you use a factory method for testing? Something like:
```
public:
static SecurityKeyAuthHandlerPosix MakeForTesting(...);
```
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)
Jun 24, 2026, 8:15:24 PM (4 days ago) Jun 24
to Lambros Lambrou, Joe Downing, Chromium LUCI CQ, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Yuwei Huang voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention set is empty
Submit Requirements:
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)
Jun 24, 2026, 8:20:26 PM (4 days ago) Jun 24
to Yuwei Huang, Lambros Lambrou, Joe Downing, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: remoting/host/security_key/security_key_auth_handler_posix_unittest.cc
Insertions: 2, Deletions: 2.
@@ -82,8 +82,8 @@
auto file_task_runner = base::ThreadPool::CreateSequencedTaskRunner(
{base::MayBlock(), base::TaskPriority::BEST_EFFORT});
- auth_handler_ = base::WrapUnique(new remoting::SecurityKeyAuthHandlerPosix(
- socket_path_, file_task_runner));
+ auth_handler_ = remoting::SecurityKeyAuthHandlerPosix::CreateForTesting(
+ socket_path_, file_task_runner);
auth_handler_->SetSendMessageCallback(send_message_callback_);
EXPECT_NE(auth_handler_.get(), nullptr);
}
```
```
The name of the file: remoting/host/security_key/security_key_auth_handler_posix.cc
Insertions: 9, Deletions: 0.
@@ -116,6 +116,15 @@
GetFileTaskRunnerRef() = nullptr;
}
+// static
+std::unique_ptr<SecurityKeyAuthHandlerPosix>
+SecurityKeyAuthHandlerPosix::CreateForTesting(
+ const base::FilePath& socket_name,
+ scoped_refptr<base::SequencedTaskRunner> file_task_runner) {
+ return base::WrapUnique(new SecurityKeyAuthHandlerPosix(
+ socket_name, std::move(file_task_runner)));
+}
+
SecurityKeyAuthHandlerPosix::SecurityKeyAuthHandlerPosix()
: socket_name_(GetSecurityKeySocketName()),
file_task_runner_(GetFileTaskRunner()),
```
```
The name of the file: remoting/host/security_key/security_key_auth_handler_posix.h
Insertions: 6, Deletions: 5.
@@ -28,7 +28,6 @@
namespace remoting {
class SecurityKeySocket;
-class SecurityKeyAuthHandlerPosixTest;
class SecurityKeyAuthHandlerPosix : public SecurityKeyAuthHandler {
public:
@@ -44,6 +43,12 @@
// Resets the lazy task runner between unit tests to prevent stale delegates.
static void ResetTaskRunnerForTesting();
+ // Creates a SecurityKeyAuthHandlerPosix instance for testing, allowing
+ // injection of a custom socket path and task runner.
+ static std::unique_ptr<SecurityKeyAuthHandlerPosix> CreateForTesting(
+ const base::FilePath& socket_name,
+ scoped_refptr<base::SequencedTaskRunner> file_task_runner);
+
SecurityKeyAuthHandlerPosix();
SecurityKeyAuthHandlerPosix(const SecurityKeyAuthHandlerPosix&) = delete;
@@ -63,10 +68,6 @@
void SetRequestTimeoutForTest(base::TimeDelta timeout) override;
private:
- friend class SecurityKeyAuthHandler;
- friend class SecurityKeyAuthHandlerPosixTest;
-
- // Private constructor for testing to inject dependencies.
SecurityKeyAuthHandlerPosix(
const base::FilePath& socket_name,
scoped_refptr<base::SequencedTaskRunner> file_task_runner);
```
Change information
Commit message:
remoting: More refactoring for security key
The main purpose of this CL is to eliminate the need to pass
host_context_->file_task_runner() into SecurityKeyAuthHandlerPosix so
that we don't need to add new parameters to ChromotingHost and
ClientSession. MAGI imposes more changes, which make sense, so I'm
keeping them in this CL.
- Instead of passing in the file task runner,
SecurityKeyAuthHandlerPosix will create an internal one.
- Converts the global g_use_mojo_handler flag to std::atomic<bool> to
guarantee thread-safe concurrent access across UI and Network threads.
- Protects the lazy initialization of the task runner and socket name
with a static base::Lock, preventing concurrent write races during
host startup in single-process mode.
- Changes GetSecurityKeySocketName() to return base::FilePath by value
instead of by reference, eliminating post-lock-release data races.
- Modernizes unit tests to use base::test::TaskEnvironment and
base::test::RunUntil, and uses ResetTaskRunnerForTesting() to clear
the lazy task runner between tests to prevent stale delegate errors.
Bug: 517007701
Change-Id: I73614137696759c2eb67682854a7098bab4648a6
Reviewed-by: Lambros Lambrou <lambros...@chromium.org>
Commit-Queue: Yuwei Huang <yuw...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1652083}
Files:
- M remoting/host/BUILD.gn
- M remoting/host/desktop_process_unittest.cc
- M remoting/host/desktop_session_agent.cc
- M remoting/host/remoting_me2me_host.cc
- M remoting/host/security_key/security_key_auth_handler.cc
- M remoting/host/security_key/security_key_auth_handler.h
- M remoting/host/security_key/security_key_auth_handler_posix.cc
- M remoting/host/security_key/security_key_auth_handler_posix.h
- M remoting/host/security_key/security_key_auth_handler_posix_unittest.cc
- M remoting/host/security_key/security_key_extension.cc
- M remoting/host/security_key/security_key_extension.h
- M remoting/host/security_key/security_key_extension_session.cc
- M remoting/host/security_key/security_key_extension_session.h
- M remoting/host/security_key/security_key_extension_session_unittest.cc
Change size: L
Delta: 14 files changed, 187 insertions(+), 115 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Lambros Lambrou
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages