remoting: Refactor SecurityKeyExtensionSession for parallel transport [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang added 4 comments![]( "Open in Gerrit")
std::unique_ptr<SecurityKeyExtension> security_key_extension_;To avoid a dangling pointer crash, `security_key_session_` and `security_key_extension_` should be declared *before* `extension_manager_`.\n\nIn C++, members are destroyed in the reverse order of their declaration. `extension_manager_` holds a `raw_ptr` to the `security_key_extension_` (via `active_extensions`). If the extension is destroyed first, the `raw_ptr` inside `extension_manager_` becomes dangling. This will trigger a crash during destruction in Chromium's BackupRefPtr (BRP) dangling pointer detector.
scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;`file_task_runner_` is no longer used in `SecurityKeyExtension`. It was previously passed to `SecurityKeyExtensionSession`, but now it's only needed by `SecurityKeySession`. We can remove this member and drop it from the constructor.
ClientSessionDetails* client_session_details,`client_session_details` is completely unused in `SecurityKeyExtensionSession`. It used to be passed into `SecurityKeyAuthHandler::Create()`, but that is now handled by `SecurityKeySession`. We can safely remove this parameter (and ignore it when calling the constructor from `SecurityKeyExtension::CreateExtensionSession`).
raw_ptr<protocol::ClientStub> client_stub_ = nullptr;`client_stub_` is unused inside `SecurityKeySession` (the actual message sending is delegated to `SecurityKeyExtensionSession`, which retains its own `client_stub_`). You can safely remove this member and drop the parameter from the constructor.
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. |
Yuwei Huang (Gerrit)
Yuwei Huang added 4 comments![]( "Open in Gerrit")
std::unique_ptr<SecurityKeyExtension> security_key_extension_;To avoid a dangling pointer crash, `security_key_session_` and `security_key_extension_` should be declared *before* `extension_manager_`.\n\nIn C++, members are destroyed in the reverse order of their declaration. `extension_manager_` holds a `raw_ptr` to the `security_key_extension_` (via `active_extensions`). If the extension is destroyed first, the `raw_ptr` inside `extension_manager_` becomes dangling. This will trigger a crash during destruction in Chromium's BackupRefPtr (BRP) dangling pointer detector.
Done
scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;`file_task_runner_` is no longer used in `SecurityKeyExtension`. It was previously passed to `SecurityKeyExtensionSession`, but now it's only needed by `SecurityKeySession`. We can remove this member and drop it from the constructor.
Done
`client_session_details` is completely unused in `SecurityKeyExtensionSession`. It used to be passed into `SecurityKeyAuthHandler::Create()`, but that is now handled by `SecurityKeySession`. We can safely remove this parameter (and ignore it when calling the constructor from `SecurityKeyExtension::CreateExtensionSession`).
Done
`client_stub_` is unused inside `SecurityKeySession` (the actual message sending is delegated to `SecurityKeyExtensionSession`, which retains its own `client_stub_`). You can safely remove this member and drop the parameter from the constructor.
Done
Related details
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")
local_session_policies->allow_gnubby_forwarding = false;This is probably not going to work. If you read `ClientSession::OnConnectionAuthenticated`, the effective policies are not cascaded, but rather replaced. Also, it is a good idea to pass support state independent of the policy itself.
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. |