Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2149983003:
Refactoring Elevated Host Communication Channel into its own class. (Closed)
|
Created:
4 years, 5 months ago by joedow Modified:
4 years, 4 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@process_helper Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactoring Elevated Host Communication Channel into its own class.
This change is the third refactoring needed to allow the Me2Me and It2Me hosts
to share the logic used to launch elevated/uiaccess enabled processes and
communicate between the native message client and the privileged host.
BUG=617185
Committed: https://crrev.com/882831c1d5efc2cbfd7cf9712a21eed795fa61e0
Cr-Commit-Position: refs/heads/master@{#411211}
Patch Set 1 #Patch Set 2 : merging with another set of changes #Patch Set 3 : Merging with ToT #Patch Set 4 : Adding ability to specify elevated host lifetime #Patch Set 5 : Fixing a non-windows build break #Patch Set 6 : Merging with ToT changes #Patch Set 7 : Merging again (including previous dependency CLs that have been checked in). #
Total comments: 8
Patch Set 8 : Addressing CR feedback #Patch Set 9 : Merging with upstream changes #Patch Set 10 : Merging upstream changes. #
Depends on Patchset: Messages
Total messages: 53 (45 generated)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Refactoring Elevated Host Communication Channel into its own class. This change is the second refactoring needed to allow the Me2Me and It2Me hosts to share the logic used to launch elevated/uiaccess enabled processes and communicate between the native message client and the privileged host. BUG=617185 ========== to ========== Refactoring Elevated Host Communication Channel into its own class. This change is the third refactoring needed to allow the Me2Me and It2Me hosts to share the logic used to launch elevated/uiaccess enabled processes and communicate between the native message client and the privileged host. BUG=617185 ==========
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
joedow@chromium.org changed reviewers: + sergeyu@chromium.org
PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping!
https://codereview.chromium.org/2149983003/diff/120001/remoting/host/setup/me... File remoting/host/setup/me2me_native_messaging_host.cc (right): https://codereview.chromium.org/2149983003/diff/120001/remoting/host/setup/me... remoting/host/setup/me2me_native_messaging_host.cc:556: if (elevated_host_->EnsureElevatedHostCreated()) { EnsureElevatedHostCreated() may call client_->CloseChannel() in case of failure, but then the caller of DelegateToElevatedHost() will still send an error response to the original request. https://codereview.chromium.org/2149983003/diff/120001/remoting/host/win/elev... File remoting/host/win/elevated_native_messaging_host.cc (right): https://codereview.chromium.org/2149983003/diff/120001/remoting/host/win/elev... remoting/host/win/elevated_native_messaging_host.cc:55: if (elevated_channel_) { Do we need to support the case when EnsureElevatedHostCreated() is called multiple times? Maybe rename it to Initialize() or Start() and allow it to be called only once? https://codereview.chromium.org/2149983003/diff/120001/remoting/host/win/elev... remoting/host/win/elevated_native_messaging_host.cc:66: client_->CloseChannel(std::string()); I'm not sure it's safe to call CloseChannel() here - see my comment in me2me_native_messaging_host.cc https://codereview.chromium.org/2149983003/diff/120001/remoting/remoting_host... File remoting/remoting_host_win.gypi (right): https://codereview.chromium.org/2149983003/diff/120001/remoting/remoting_host... remoting/remoting_host_win.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved. don't need to update gyp files anymore.
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Addressed feedback and fixed problems with OnError() hang in an upstream CL. https://codereview.chromium.org/2149983003/diff/120001/remoting/host/setup/me... File remoting/host/setup/me2me_native_messaging_host.cc (right): https://codereview.chromium.org/2149983003/diff/120001/remoting/host/setup/me... remoting/host/setup/me2me_native_messaging_host.cc:556: if (elevated_host_->EnsureElevatedHostCreated()) { On 2016/08/02 21:18:27, Sergey Ulanov wrote: > EnsureElevatedHostCreated() may call client_->CloseChannel() in case of failure, > but then the caller of DelegateToElevatedHost() will still send an error > response to the original request. Done. https://codereview.chromium.org/2149983003/diff/120001/remoting/host/win/elev... File remoting/host/win/elevated_native_messaging_host.cc (right): https://codereview.chromium.org/2149983003/diff/120001/remoting/host/win/elev... remoting/host/win/elevated_native_messaging_host.cc:55: if (elevated_channel_) { On 2016/08/02 21:18:27, Sergey Ulanov wrote: > Do we need to support the case when EnsureElevatedHostCreated() is called > multiple times? Maybe rename it to Initialize() or Start() and allow it to be > called only once? We need to support the elevated host being created multiple times during the instances lifetime. Whether that is in this method or a different one is the question. For example, the Me2Me NMH launches the elevated process to start/stop the host service, but the elevated process is torn down after 5 minutes. If the user enables the host (for example) which requires elevation, then leaves the webapp open for 6 minutes, then decides to disable the host, then a new elevated process will be created to handle that task. I think it is cleaner to wrap that logic in this method so the consumer of this class does not know if an elevated process exists or not. https://codereview.chromium.org/2149983003/diff/120001/remoting/host/win/elev... remoting/host/win/elevated_native_messaging_host.cc:66: client_->CloseChannel(std::string()); On 2016/08/02 21:18:27, Sergey Ulanov wrote: > I'm not sure it's safe to call CloseChannel() here - see my comment in > me2me_native_messaging_host.cc Done. Removed in an upstream change actually. https://codereview.chromium.org/2149983003/diff/120001/remoting/remoting_host... File remoting/remoting_host_win.gypi (right): https://codereview.chromium.org/2149983003/diff/120001/remoting/remoting_host... remoting/remoting_host_win.gypi:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2016/08/02 21:18:27, Sergey Ulanov wrote: > don't need to update gyp files anymore. Yeah, I did this work before the announcement that it was deprecated. Time well spent ;)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2149983003/#ps180001 (title: "Merging upstream changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Refactoring Elevated Host Communication Channel into its own class. This change is the third refactoring needed to allow the Me2Me and It2Me hosts to share the logic used to launch elevated/uiaccess enabled processes and communicate between the native message client and the privileged host. BUG=617185 ========== to ========== Refactoring Elevated Host Communication Channel into its own class. This change is the third refactoring needed to allow the Me2Me and It2Me hosts to share the logic used to launch elevated/uiaccess enabled processes and communicate between the native message client and the privileged host. BUG=617185 Committed: https://crrev.com/882831c1d5efc2cbfd7cf9712a21eed795fa61e0 Cr-Commit-Position: refs/heads/master@{#411211} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/882831c1d5efc2cbfd7cf9712a21eed795fa61e0 Cr-Commit-Position: refs/heads/master@{#411211} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
