Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15915003:
Removing deprecated DCHECK's that require App Launcher to be user-level only. (Closed)
|
Created:
7 years, 7 months ago by huangs Modified:
7 years, 6 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemoving deprecated DCHECK's that require App Launcher to be user-level only.
With Chrome / App Launcher unification, the App Launcher can be installed at system-level. Old DCHECK's to enforce otherwise are creating bogus failures, and should be removed.
This bug is triggered by https://chromiumcodereview.appspot.com/14711006/ , which adds "shadow" App Launcher Clients/ClientState keys to support 7DA tracking for App Launcher. Unfortunately, these keys are read, and are interpreted as the presence of App Launcher (thus triggering DCHECK).
A separate issue is that installation_validator complains about missing information in the "shadow" registry key. This is less urgent, and hence not part of this CL.
BUG=243432
TEST=Install system-level Chrome with debug build, see that no DCHECK occurs.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202605
Patch Set 1 #Patch Set 2 : Deleting bogus warning message. #
Total comments: 9
Patch Set 3 : Replacing DCHECK in shortcut creation with warning. #
Total comments: 2
Patch Set 4 : Decided to leave install.cc alone. #
Messages
Total messages: 23 (0 generated)
I think the DCHECK is a more serious issue. The installation_validator issue can be handled later. PTAL.
Sam, please stop adding both me and greg in parallel to code reviews, we both get more than enough code reviews as it is. If it's because you want to make sure we both see it, put one reviewer and one on the CC line (to be explicit who should review). If you put both of us, be explicit about who is expected to look at what when publishing the CL. In any case, this lgtm provided the upcoming changes which will make App Launcher install at system-level (or is this already the case?). Cheers, Gab
Acknowledged. Sorry for the habit; this is the first time I got feedback, that I recall. App Launcher is not installed per se; it's the presence of "shadow" App Launcher Clients key that triggered this.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/15915003/2001
https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:546: CURRENT_USER, app_launcher_shortcut_operation); will this ever be hit? it's very bad for this to try to create per-user shortcuts for a system-level install.
https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:546: CURRENT_USER, app_launcher_shortcut_operation); On 2013/05/24 01:18:07, grt wrote: > will this ever be hit? it's very bad for this to try to create per-user > shortcuts for a system-level install. I don't think that's true. We already do this for Chrome (i.e. taskbar shortcuts are per-user) and if it was up to me I'd make all shortcuts per-user, it avoids problems with people deleting other users' shortcuts (which is a problem with all-users shortcuts) -- we tried to do this, but failed because old versions of Chrome unconditionally delete the per-user Chrome shortcuts on self-destruct which would result in no shortcuts for that user (http://crbug.com/164655)... Per-user shortcuts are a problem only on uninstall since we can't clean them, but Windows takes care of prompting users using invalid shortcuts post-uninstall so I think that's ok (even if we were to fix it for some Chrome shortcuts, we can't fix it for user created shortcuts (profile/app shortcuts) unless doing a full sweep of every user's shortcut folders on uninstall (which we don't want to do)). Per-user shortcuts also have a nice feature that an admin can uninstall and re-install Chrome without messing up users' shortcuts (provided he re-installs before anyone uses the shortcuts...).
@grt: This is independent of this CL, right? Should we just CQ? This is blocking other people now (i.e. asvitkine).
https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:546: CURRENT_USER, app_launcher_shortcut_operation); On 2013/05/24 12:32:33, gab wrote: > On 2013/05/24 01:18:07, grt wrote: > > will this ever be hit? it's very bad for this to try to create per-user > > shortcuts for a system-level install. > > I don't think that's true. The installer may run as SYSTEM, where there is no such thing as a user. If Sam's previous change (that this is supposed to fix) results in the installer trying to create a per-user shortcut for a system-level install, then that's what needs to be fixed. By itself, removing the DCHECK here is wrong. > We already do this for Chrome (i.e. taskbar shortcuts > are per-user) and if it was up to me I'd make all shortcuts per-user, it avoids > problems with people deleting other users' shortcuts (which is a problem with > all-users shortcuts) -- we tried to do this, but failed because old versions of > Chrome unconditionally delete the per-user Chrome shortcuts on self-destruct > which would result in no shortcuts for that user (http://crbug.com/164655)... > > Per-user shortcuts are a problem only on uninstall since we can't clean them, > but Windows takes care of prompting users using invalid shortcuts post-uninstall > so I think that's ok (even if we were to fix it for some Chrome shortcuts, we > can't fix it for user created shortcuts (profile/app shortcuts) unless doing a > full sweep of every user's shortcut folders on uninstall (which we don't want to > do)). > > Per-user shortcuts also have a nice feature that an admin can uninstall and > re-install Chrome without messing up users' shortcuts (provided he re-installs > before anyone uses the shortcuts...).
https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:546: CURRENT_USER, app_launcher_shortcut_operation); On 2013/05/24 12:32:33, gab wrote: > On 2013/05/24 01:18:07, grt wrote: > > will this ever be hit? it's very bad for this to try to create per-user > > shortcuts for a system-level install. > > I don't think that's true. We already do this for Chrome (i.e. taskbar shortcuts > are per-user) and if it was up to me I'd make all shortcuts per-user, it avoids > problems with people deleting other users' shortcuts (which is a problem with > all-users shortcuts) -- we tried to do this, but failed because old versions of > Chrome unconditionally delete the per-user Chrome shortcuts on self-destruct > which would result in no shortcuts for that user (http://crbug.com/164655)... > > Per-user shortcuts are a problem only on uninstall since we can't clean them, > but Windows takes care of prompting users using invalid shortcuts post-uninstall > so I think that's ok (even if we were to fix it for some Chrome shortcuts, we > can't fix it for user created shortcuts (profile/app shortcuts) unless doing a > full sweep of every user's shortcut folders on uninstall (which we don't want to > do)). > > Per-user shortcuts also have a nice feature that an admin can uninstall and > re-install Chrome without messing up users' shortcuts (provided he re-installs > before anyone uses the shortcuts...). My bad, I didn't get Greg's point correctly at first. Indeed the installer can't create per-user shortcuts on system-level installs since it runs as SYSTEM in most cases. This is not the DCHECK we are currently hitting though (the ones in installation_validator.cc and uninstall.cc) are the problems.
https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:546: CURRENT_USER, app_launcher_shortcut_operation); We're not hitting this DCHECK, but I need to do more tracing to be sure. The current priority is to get things to work, so I'd say, we should remove this check, wrap shortcut creation inside "if (user_level)", and log warning if system-level is detected (plus TODO). This will be properly fixed once we figure out Chrome / App Launcher unification stuff. I'll be in a position (better internet) to implement this in an hour or two; please let me know in the meantime if you're okay with the plan.
https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:546: CURRENT_USER, app_launcher_shortcut_operation); On 2013/05/24 15:54:30, huangs1 wrote: > We're not hitting this DCHECK, but I need to do more tracing to be sure. The > current priority is to get things to work, so I'd say, we should remove this > check, wrap shortcut creation inside "if (user_level)", and log warning if > system-level is detected (plus TODO). This will be properly fixed once we figure > out Chrome / App Launcher unification stuff. > > I'll be in a position (better internet) to implement this in an hour or two; > please let me know in the meantime if you're okay with the plan. Aren't you on vacation?! There is no rush really since the few of us with this issue can comment out the DCHECK for now. Take a break and we can talk about it next week ;)!
https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:546: CURRENT_USER, app_launcher_shortcut_operation); On 2013/05/24 15:54:30, huangs1 wrote: > We're not hitting this DCHECK, but I need to do more tracing to be sure. The > current priority is to get things to work, so I'd say, we should remove this > check, wrap shortcut creation inside "if (user_level)", and log warning if > system-level is detected (plus TODO). If CheckAppHostPreconditions is to be believed, the app host can't be installed at system-level. Will AddExistingMultiInstalls cause this code to be hit when a system-level install that has the app host "pv" (for user tracking) is updated? If so, that's possibly a bigger problem than this. What are the implications there? > This will be properly fixed once we figure > out Chrome / App Launcher unification stuff. > > I'll be in a position (better internet) to implement this in an hour or two; > please let me know in the meantime if you're okay with the plan.
On 2013/05/24 15:57:55, gab wrote: > https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... > File chrome/installer/setup/install.cc (right): > > https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... > chrome/installer/setup/install.cc:546: CURRENT_USER, > app_launcher_shortcut_operation); > On 2013/05/24 15:54:30, huangs1 wrote: > > We're not hitting this DCHECK, but I need to do more tracing to be sure. The > > current priority is to get things to work, so I'd say, we should remove this > > check, wrap shortcut creation inside "if (user_level)", and log warning if > > system-level is detected (plus TODO). This will be properly fixed once we > figure > > out Chrome / App Launcher unification stuff. > > > > I'll be in a position (better internet) to implement this in an hour or two; > > please let me know in the meantime if you're okay with the plan. > > Aren't you on vacation?! > > There is no rush really since the few of us with this issue can comment out the > DCHECK for now. > > Take a break and we can talk about it next week ;)! Seconded. It's more important to do this right than to do it quickly. The only reason for haste would be if what's currently out there will break users when it is updated. If you think that's the case, let us know and Gab and I can try to sort it out.
https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:546: CURRENT_USER, app_launcher_shortcut_operation); Okay... I'm back on Tuesday though. A new concern now is that with the "quick-fix" IIRC this code is not hit at first install. But on update I'll have to test! Just want to test this and put it to rest so I can sleep better. :)
https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... File chrome/installer/setup/install.cc (right): https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... chrome/installer/setup/install.cc:546: CURRENT_USER, app_launcher_shortcut_operation); Did some testing and investigation, verified that App Launcher shortcut will NOT get added during update. Reason: The "shadow" Products key makes App Launcher get added to InstallationState; this triggers calls in validator and uninstall, which is where our DCHECK() get called. But during update, since the "shadow" key has no uninstall command, so product->is_multi_install() returns false, and App Launcher is NOT added to InstallerState => hence no shortcut update (another layer of protection is that no new shortcuts get created on update). In short, my feared scenario (Chrome user magically gets App Launcher after update) will not happen; I can relax for now. :)
On 2013/05/24 18:39:46, huangs1 wrote: > https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... > File chrome/installer/setup/install.cc (right): > > https://codereview.chromium.org/15915003/diff/2001/chrome/installer/setup/ins... > chrome/installer/setup/install.cc:546: CURRENT_USER, > app_launcher_shortcut_operation); > Did some testing and investigation, verified that App Launcher shortcut will NOT > get added during update. > > Reason: The "shadow" Products key makes App Launcher get added to > InstallationState; this triggers calls in validator and uninstall, which is > where our DCHECK() get called. But during update, since the "shadow" key has no > uninstall command, so product->is_multi_install() returns false, and App > Launcher is NOT added to InstallerState => hence no shortcut update (another > layer of protection is that no new shortcuts get created on update). > > In short, my feared scenario (Chrome user magically gets App Launcher after > update) will not happen; I can relax for now. :) Thanks for checking that out. Now go enjoy your vacation!
This change removes the DCHECK without changing functionality (the code path is for the deprecated --app-launcher flow, which will get cleaned up anyway). PTAL.
https://codereview.chromium.org/15915003/diff/35001/chrome/installer/setup/in... File chrome/installer/setup/install.cc (left): https://codereview.chromium.org/15915003/diff/35001/chrome/installer/setup/in... chrome/installer/setup/install.cc:540: DCHECK(!installer_state.system_install()); I don't think we need to remove this DCHECK for now, it is not part of the ones that we are hitting at Chrome install/uninstall due to the shadow keys, right?
https://codereview.chromium.org/15915003/diff/35001/chrome/installer/setup/in... File chrome/installer/setup/install.cc (left): https://codereview.chromium.org/15915003/diff/35001/chrome/installer/setup/in... chrome/installer/setup/install.cc:540: DCHECK(!installer_state.system_install()); Agreed; there's no need to wipe out the same TODO in the same CL. Removing change and clicking "commit" (to run tests).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/15915003/40002
Thanks, re-lgtm, FYI to "run tests" you don't need to check the commit-box; use "git cl try" from your local branch instead which will launch basically all the same tests on the CL (running against LKGR). Cheers, Gab
lgtm++
Message was sent while issue was closed.
Change committed as 202605 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
