Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
359443002:
Elevated install of recovery component when needed (Closed)
|
Created:
6 years, 6 months ago by xiaoling Modified:
6 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionElevated install of recovery component when needed.
First tries to do recovery non-elevated. When the process finds that elevation is needed, pop up a UI to notify user of UAC prompt. If permission is granted, run recovery component installer again with elevated privilege.
The CL does not have the UI changes. A separate CL (https://codereview.chromium.org/325433002/) is created for that.
This CL includes:
(1) When elevation is needed to recover, save the installation source for retry, set a flag in preference to indicate that.
(2) Provide API for elevated install
BUG=381801
Committed: https://crrev.com/30c9f3f4fb1eccaef28e0438fe12352a012e591b
Cr-Commit-Position: refs/heads/master@{#307372}
Patch Set 1 #Patch Set 2 : Removed recovery_execute app. #Patch Set 3 : #Patch Set 4 : directly elevate ChromeRecovery.exe #Patch Set 5 #Patch Set 6 #Patch Set 7 : directly invoke exe #
Total comments: 28
Patch Set 8 #Patch Set 9 #Patch Set 10 #Patch Set 11 : addressed comments #
Total comments: 10
Patch Set 12 : address comments round 2 #
Total comments: 4
Patch Set 13 : Use workerpool thread to wait #Patch Set 14 : comment out unused function on chromeos #
Messages
Total messages: 23 (3 generated)
Uploaded another patch set. The main change is removed recovery_exectue app. Use chrome.exe to copy installer crx to secure location and do patch.
Xiaoling, I'm very sorry it has taken so long to review this. Overall I am concerned about the addition of InstallExternally() to the installer API, the additional complexity in ComponentUnpacker, and persisting the metadata to disk. Do we need InstallExternally? All it looks like it does is write a prefs - can't we just do that in the Recovery Component's normal Install() before it fails (or post a task to do it when we know it will fail), and hide this detail from the other installers? About the ComponentUnpacker, I realize that the way the code is written makes this unfortunately more complex than it ought to be (e.g. ComponentUnpacker also kicks off the install). But, maybe it's better to save the copy of the CRX in component_updater_service.cc? We could adapt this mechanism into a general cache for downloaded items. On the other hand, I think Sorin and I will be creating / adapting such a cache in the new combined updater-in-Chrome, so maybe this would wind up being wasted work (or maybe we could reuse whatever you write). Sorin, any opinion here? Finally, I believe the code has changed/moved enough that serializing in_process as metadata will no longer be relevant. I'm happy to discuss this if you want.
Thanks Josh for your great feedback. CIL. On Mon, Aug 25, 2014 at 9:56 AM, <waffles@chromium.org> wrote: > Xiaoling, I'm very sorry it has taken so long to review this. > > Overall I am concerned about the addition of InstallExternally() to the > installer API, the additional complexity in ComponentUnpacker, and > persisting > the metadata to disk. > > Do we need InstallExternally? All it looks like it does is write a prefs - > can't > we just do that in the Recovery Component's normal Install() before it > fails (or > post a task to do it when we know it will fail), and hide this detail from > the > other installers? > Yes, that's what I was debating with myself. I tried to do similar way as you suggested. See the unsent CL https://codereview.chromium.org/321473003/diff/1/chrome/browser/component_upd... . The biggest reason is that installer doesn't know the crx file path and fingerprint. And they are needed to do the backup work. So we either save the crx from the unpacker, or pass the info from unpacker to installer and let installer do it. They both look ugly though. > About the ComponentUnpacker, I realize that the way the code is written > makes > this unfortunately more complex than it ought to be (e.g. > ComponentUnpacker also > kicks off the install). Yes, I wished Unpacker is a standalone class that just does the unpack.Then we can put fingerprint (and other installation related information) into a class that manages installation separately. That will make Unpacker class more ready for reuse, and I don't need to add the function such as CreateFromBackup(). > But, maybe it's better to save the copy of the CRX in > component_updater_service.cc? We could adapt this mechanism into a general cache > for downloaded items. On the other hand, I think Sorin and I will be > creating / > adapting such a cache in the new combined updater-in-Chrome, so maybe this > would > wind up being wasted work (or maybe we could reuse whatever you write). > Sorin, > any opinion here? > Good to know that. Just curious, why do we need the general cache? What need to be cached (cache crx files only, or other related information from the update response, such as fingerprint)? > Finally, I believe the code has changed/moved enough that serializing > in_process > as metadata will no longer be relevant. > Thanks, I noticed that, and going to do a merge. > > I'm happy to discuss this if you want. > > https://codereview.chromium.org/359443002/ > -- Xiaoling To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Now directly launches ChromeRecovery.exe elevated. This CL does not include the crx caching part, which probably will happen in the CUS part. At this moment, the on-demand update check will re-down recovery crx and re-install.
PTAL. The implementation is much simpler now and most changes are within one file.
lgtm https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:54: enum ChromeRecvoeryExitCode { typo: "Recvoery"
thank you Xiaoling! https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:54: enum ChromeRecvoeryExitCode { Just simply write: // ChromeRecovery process exit codes. Also consider documenting the values if it is appropriate. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:63: const CommandLine& cmd_line = *CommandLine::ForCurrentProcess(); we could use a pointer type here, no need to dereference. return CommandLine::ForCurrentProcess()->HasSwitch(... https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:70: scoped_ptr<base::Value> root(serializer.Deserialize(NULL, &error)); Are we including "base/memory/scoped_ptr.h" anywhere? https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:71: if (!root.get()) this is mostly a matter of preference but you could do: if (root.get() && root->IsType(base::Value::TYPE_DICTIONARY)) return scoped_ptr<base::DictionaryValue>( static_cast<base::DictionaryValue*>(root.release())).Pass() return scoped_ptr<base::DictionaryValue>(); or you could do: if (!root.get() || !root->IsType(base::Value::TYPE_DICTIONARY))) return scoped_ptr<base::DictionaryValue>(); return scoped_ptr<base::DictionaryValue>( static_cast<base::DictionaryValue*>(root.release())).Pass(); The call to Pass may not be needed, see @50 in base/memory/scoped_ptr.h. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:80: base::FilePath main_file = path.Append(kRecoveryFileName); Can any of these locals in this function be declared const? https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:104: cmdline.AppendSwitchASCII("version", version.GetString()); I always use {} in cases where the conditional and its condition do not fit in one line. "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces." https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:116: BrowserThread::PostTask( In general, we wanted to get rid of executing code on the FILE thread, does it make sense to get a thread from the blocking pool, like this? content::BrowserThread::GetBlockingPool() ->GetSequencedTaskRunnerWithShutdownBehavior( content::BrowserThread::GetBlockingPool()->GetSequenceToken(), base::SequencedWorkerPool::SKIP_ON_SHUTDOWN); https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:178: void RecoveryUpdateElevationHelper(bool elevation_needed, PrefService* prefs) { We can be more specific here and rename this function to: SetRecoveryComponentNeedsElevationPrefs. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:186: prefs->SetFilePath(prefs::kRecoveryComponentUnpackPath, unpack_path); Same as above. SetRecoveryComponentNeedsUnpackPath. The reason is that the code @252 becomes easier to read. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:241: #if defined(OS_WIN) Refactor as discussed. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.h (right): https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.h:25: // Notifies recovery component that user agreed elevated install, so that it the user https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.h:30: // Notifies recovery component that elevated install is declined. // Notifies the recovery component that the elevated install has been declined. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.h:31: // Clears preference flag prefs::kRecoveryComponentNeedsElevation. Clears the flag prefs::kRecoveryComponentNeedsElevation.
Thank you! https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:54: enum ChromeRecvoeryExitCode { On 2014/12/05 00:59:48, waffles wrote: > typo: "Recvoery" Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:54: enum ChromeRecvoeryExitCode { On 2014/12/05 02:10:36, Sorin Jianu wrote: > Just simply write: > // ChromeRecovery process exit codes. > > Also consider documenting the values if it is appropriate. Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:63: const CommandLine& cmd_line = *CommandLine::ForCurrentProcess(); On 2014/12/05 02:10:36, Sorin Jianu wrote: > we could use a pointer type here, no need to dereference. > > return CommandLine::ForCurrentProcess()->HasSwitch(... Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:70: scoped_ptr<base::Value> root(serializer.Deserialize(NULL, &error)); On 2014/12/05 02:10:36, Sorin Jianu wrote: > Are we including "base/memory/scoped_ptr.h" anywhere? Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:71: if (!root.get()) On 2014/12/05 02:10:36, Sorin Jianu wrote: > this is mostly a matter of preference but you could do: > > if (root.get() && root->IsType(base::Value::TYPE_DICTIONARY)) > return scoped_ptr<base::DictionaryValue>( > static_cast<base::DictionaryValue*>(root.release())).Pass() > > return scoped_ptr<base::DictionaryValue>(); > > or you could do: > > if (!root.get() || !root->IsType(base::Value::TYPE_DICTIONARY))) > return scoped_ptr<base::DictionaryValue>(); > > return scoped_ptr<base::DictionaryValue>( > static_cast<base::DictionaryValue*>(root.release())).Pass(); > > The call to Pass may not be needed, see @50 in base/memory/scoped_ptr.h. Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:80: base::FilePath main_file = path.Append(kRecoveryFileName); On 2014/12/05 02:10:36, Sorin Jianu wrote: > Can any of these locals in this function be declared const? Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:104: cmdline.AppendSwitchASCII("version", version.GetString()); On 2014/12/05 02:10:36, Sorin Jianu wrote: > I always use {} in cases where the conditional and its condition do not fit in > one line. > > "In general, curly braces are not required for single-line statements, but they > are allowed if you like them; conditional or loop statements with complex > conditions or statements may be more readable with curly braces." Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:116: BrowserThread::PostTask( On 2014/12/05 02:10:36, Sorin Jianu wrote: > In general, we wanted to get rid of executing code on the FILE thread, does it > make sense to get a thread from the blocking pool, like this? > > content::BrowserThread::GetBlockingPool() > ->GetSequencedTaskRunnerWithShutdownBehavior( > content::BrowserThread::GetBlockingPool()->GetSequenceToken(), > base::SequencedWorkerPool::SKIP_ON_SHUTDOWN); Done. Used WorkerPool thread to avoid hassle of maintaining ref counter of the task runner. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:178: void RecoveryUpdateElevationHelper(bool elevation_needed, PrefService* prefs) { On 2014/12/05 02:10:36, Sorin Jianu wrote: > We can be more specific here and rename this function to: > SetRecoveryComponentNeedsElevationPrefs. Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:186: prefs->SetFilePath(prefs::kRecoveryComponentUnpackPath, unpack_path); On 2014/12/05 02:10:36, Sorin Jianu wrote: > Same as above. > SetRecoveryComponentNeedsUnpackPath. > > The reason is that the code @252 becomes easier to read. Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:241: #if defined(OS_WIN) On 2014/12/05 02:10:36, Sorin Jianu wrote: > Refactor as discussed. Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.h (right): https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.h:25: // Notifies recovery component that user agreed elevated install, so that it On 2014/12/05 02:10:36, Sorin Jianu wrote: > the user Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.h:30: // Notifies recovery component that elevated install is declined. On 2014/12/05 02:10:36, Sorin Jianu wrote: > // Notifies the recovery component that the elevated install has been declined. Done. https://codereview.chromium.org/359443002/diff/110001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.h:31: // Clears preference flag prefs::kRecoveryComponentNeedsElevation. On 2014/12/05 02:10:36, Sorin Jianu wrote: > Clears the flag prefs::kRecoveryComponentNeedsElevation. Done.
Thank you X! https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:64: const CommandLine* cmd_line = CommandLine::ForCurrentProcess(); do you still want to have the local? https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:94: Version version(proposed_version.c_str()); const? https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:217: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, we could refactor this as two calls which occur here as a pair as one function. https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:301: if (SimulatingElevatedRecovery()) { do we need to recovery for Chrome OS too? https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.h (right): https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.h:25: // Notifies recovery component that the user agreed elevated install, so that it We still need better wording. How about: Notifies the recovery component that the user has accepted the elevation prompt. Clears the state of prefs::kRecoveryComponentNeedsElevation after the notification.
Thank you. Josh, can you please help to double check the thread model? RecoveryComponentInstaller::Install() is executed by the blocking task runner from CUS. DoElevatedInstallRecoveryComponent() is executed by WorkerPool thread. https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:64: const CommandLine* cmd_line = CommandLine::ForCurrentProcess(); On 2014/12/06 00:41:53, Sorin Jianu wrote: > do you still want to have the local? Done. https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:94: Version version(proposed_version.c_str()); On 2014/12/06 00:41:53, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:217: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, On 2014/12/06 00:41:53, Sorin Jianu wrote: > we could refactor this as two calls which occur here as a pair as one function. Done. https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:301: if (SimulatingElevatedRecovery()) { On 2014/12/06 00:41:52, Sorin Jianu wrote: > do we need to recovery for Chrome OS too? Done. https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.h (right): https://codereview.chromium.org/359443002/diff/190001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.h:25: // Notifies recovery component that the user agreed elevated install, so that it On 2014/12/06 00:41:53, Sorin Jianu wrote: > We still need better wording. How about: > > Notifies the recovery component that the user has accepted the elevation prompt. > Clears the state of prefs::kRecoveryComponentNeedsElevation after the > notification. Done.
https://codereview.chromium.org/359443002/diff/210001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/359443002/diff/210001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:114: base::WorkerPool::PostTask( I think this is OK. Threads from the worker pool are never joined during shutdown, so DoElevatedInstallRecoveryComponent must be able to function in an environment where the rest of Chrome has already been destructed. https://codereview.chromium.org/359443002/diff/210001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:207: const base::TimeDelta kMaxWaitTime = base::TimeDelta::FromSeconds(600); In the worst case, this can block Chrome shutdown for up to 10 minutes. In that case, the UI thread will already be stopped by the time this returns, so we can't set the prefs anyways. So my thinking is that it's probably better to post this out to the worker pool (or to the sequenced worker pool with a different shutdown behavior). Of course, that means that the install can complete before the process is launched. So we should be extra careful with the concurrency, we've had bugs there before.
Thanks for the suggestion. https://codereview.chromium.org/359443002/diff/210001/chrome/browser/componen... File chrome/browser/component_updater/recovery_component_installer.cc (right): https://codereview.chromium.org/359443002/diff/210001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:114: base::WorkerPool::PostTask( On 2014/12/06 01:20:35, waffles wrote: > I think this is OK. > > Threads from the worker pool are never joined during shutdown, so > DoElevatedInstallRecoveryComponent must be able to function in an environment > where the rest of Chrome has already been destructed. Acknowledged. https://codereview.chromium.org/359443002/diff/210001/chrome/browser/componen... chrome/browser/component_updater/recovery_component_installer.cc:207: const base::TimeDelta kMaxWaitTime = base::TimeDelta::FromSeconds(600); On 2014/12/06 01:20:35, waffles wrote: > In the worst case, this can block Chrome shutdown for up to 10 minutes. > > In that case, the UI thread will already be stopped by the time this returns, so > we can't set the prefs anyways. So my thinking is that it's probably better to > post this out to the worker pool (or to the sequenced worker pool with a > different shutdown behavior). > > Of course, that means that the install can complete before the process is > launched. So we should be extra careful with the concurrency, we've had bugs > there before. Done.
lgtm thank you!
lgtm
The CQ bit was checked by xiaolingbao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/359443002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiaolingbao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/359443002/250001
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/30c9f3f4fb1eccaef28e0438fe12352a012e591b Cr-Commit-Position: refs/heads/master@{#307372} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
