Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
695133005:
Temporarily disable extensions and sync while a profile is locked - Profiles Approach (Closed)
|
Created:
6 years, 1 month ago by Mike Lerman Modified:
6 years ago Reviewers:
noms (inactive), pkotwicz, Andrew T Wilson (Slow), jwd, binjin, Ilya Sherman, not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, asvitkine+watch_chromium.org, extensions-reviews_chromium.org, Ilya Sherman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionTemporarily disable extensions and sync while a profile is locked.
Place all the heavy lifting and logic in profiles code, with extensions providing a new collection for holding "locked" extensions.
This replaces CL 697143003.
BUG=354124
Committed: https://crrev.com/6a37b6a4b4914a5e762f90b673d1711ab8df7770
Cr-Commit-Position: refs/heads/master@{#305897}
Patch Set 1 #Patch Set 2 : Rebase and git cl format #Patch Set 3 : Don't disable sync. Use a locked_extensions() list. #Patch Set 4 : Move all extensions logic into extension_serivice #Patch Set 5 : Profiles only disable extensions on OSes where they exist #Patch Set 6 : Fix unit test. Don't lock policy-forced extensions. #
Total comments: 39
Patch Set 7 : Address kalman@ and noms@ comments #Patch Set 8 : Handle extensions being un-terminated or added during lock. #Patch Set 9 : extension service's block_all bit set on Profile reload #
Total comments: 36
Patch Set 10 : More comments from noms@ and kalman@ #Patch Set 11 : Don't change prefs upon block. Block works on generated sets. #Patch Set 12 : Testing Cleanup #Patch Set 13 : git cl format #Patch Set 14 : Compiling is awesome #
Total comments: 20
Patch Set 15 : Remove BLOCKED state. Other kalman@ comments. #
Total comments: 12
Patch Set 16 : Extension Service unit tests #
Total comments: 6
Patch Set 17 : Many more unit tests. Change screenLockAPI permissions. Themes fix. #
Total comments: 1
Patch Set 18 : Rebase #
Total comments: 2
Messages
Total messages: 36 (7 generated)
mlerman@chromium.org changed reviewers: + kalman@chromium.org
Hi Benjamin, This is my new CL for disabling extensions from Profiles code using the same basic paradigm as blacklisting. Thanks, Mike
mlerman@chromium.org changed reviewers: + atwilson@chromium.org, jwd@chromium.org, noms@chromium.org
Adding other reviewers: jwd - histograms.xml atwilson - c/b/background noms - c/b/profiles and Profile Lock in general Thanks for all your kind and thoughtful eyes!
https://codereview.chromium.org/695133005/diff/100001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/695133005/diff/100001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:479: default: Cleanup: could you remove this default case entirely, and change the 'break's above to 'return's, then move that NOTREACHED and Shutdown(...) to after the switch. Nicer compiler errors. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:379: // that have been disabled due to blacklisting. Update comment? https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:583: // Ignore attempts to reload a blacklisted or locked extension. Sometimes this Also update this comment. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:928: Remove redundant new line. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:937: for (ExtensionSet::const_iterator extension = extensions->begin(); You could use the new : foreach syntax? https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:947: scoped_refptr<const Extension> extension = GetInstalledExtension(*id); Unless something is very wrong, this shouldn't be possible. I don't think it's necessary to store IDs separately and loop over the extensions, though. The installed extension set is a copy, and will hold on to references to the Extensions, so you can just directly loop over that. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:956: extension.get()->location() == Manifest::EXTERNAL_COMPONENT) { Hm, it's surprising that this is the first time we've needed to check this (component + external component + policy) condition. Perhaps add a method like CannotBeDisabled, and we can justify its addition by DCHECKing in methods like DisableExtension that CannotBeDisabled is false. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:963: // Blacklisted extensions remain in the blacklisted list. What about terminated extensions? Those need to go in the locked list as well. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:968: } I don't actually think the "if"s here are necessary, Remove just won't do anything if the ID isn't there. Just: registry_->RemoveEnabled(id); registry_->RemoveDisabled(id); registry_->RemoveTerminated(id); // Important not to remove Blacklisted extensions! https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:985: NOTREACHED() << "Extension " << *it << " needs to be " Again; not possible, and this code is perfectly safe (given you're taking a reference to |extension|) to directly loop over the locked extensions and not need to extract the IDs. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:994: extension_prefs_->SetExtensionState(*it, Extension::ENABLED); Again: terminated extensions. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:298: // Extensions specified by |extension_ids| should become locked. Extensions You might want to mention that "locking" does imply unloading all extensions. "Locking" would easily be misinterpreted as preventing any changes whatsoever, which isn't want the feature is. Funnily enough it almost seems like "blocked" is a better term for this whole thing. You may in fact want to think of a more abstract term for this whole thing, IIRC: - the policy team was also looking at a feature to disable all extensions? - this functionality would go nicely with a --disable-extensions or something flag, or feature, which I'm not sure - maybe we already have? https://codereview.chromium.org/695133005/diff/100001/extensions/browser/exte... File extensions/browser/extension_registry.h (right): https://codereview.chromium.org/695133005/diff/100001/extensions/browser/exte... extensions/browser/extension_registry.h:36: NONE = 0, Did clang-format really do this? Damn :( oh well. It was much better the old way, but can't argue with clang-format.
https://codereview.chromium.org/695133005/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/695133005/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:62: extension_service->LockAllExtensions(); Can this ever be NULL? https://codereview.chromium.org/695133005/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:131: cache.ProfileIsSigninRequiredAtIndex(index)) { I read this as "if this profile is locked, then unlock its extensions". Why is the profile still locked if it's got a browser window opening? https://codereview.chromium.org/695133005/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:342: LockExtensions(profile_manager->GetProfileByPath(profile_path)); I would either add a comment about how this profile is guaranteed to exist, and we're not loading it, or try to pass the Profile pointer instead of the path. https://codereview.chromium.org/695133005/diff/100001/extensions/common/exten... File extensions/common/extension.h (right): https://codereview.chromium.org/695133005/diff/100001/extensions/common/exten... extensions/common/extension.h:543: REASON_PROFILE_LOCK, // Profile is being locked. nit: is being locked or has already been locked? https://codereview.chromium.org/695133005/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/695133005/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43567: + <int value="4096" label="INACTIVE_EPHEMERAL_APP"/> nit: Is this being used anywhere?
Thanks kalman@ and noms@ for the comments. Mike https://codereview.chromium.org/695133005/diff/100001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/695133005/diff/100001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:479: default: On 2014/11/12 18:22:59, kalman wrote: > Cleanup: could you remove this default case entirely, and change the 'break's > above to 'return's, then move that NOTREACHED and Shutdown(...) to after the > switch. Nicer compiler errors. Done. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:379: // that have been disabled due to blacklisting. On 2014/11/12 18:23:00, kalman wrote: > Update comment? Done. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:583: // Ignore attempts to reload a blacklisted or locked extension. Sometimes this On 2014/11/12 18:22:59, kalman wrote: > Also update this comment. Ah, I'd only done half! Thanks. Done. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:928: On 2014/11/12 18:23:00, kalman wrote: > Remove redundant new line. Done. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:937: for (ExtensionSet::const_iterator extension = extensions->begin(); On 2014/11/12 18:23:00, kalman wrote: > You could use the new : foreach syntax? Done. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:947: scoped_refptr<const Extension> extension = GetInstalledExtension(*id); On 2014/11/12 18:23:00, kalman wrote: > Unless something is very wrong, this shouldn't be possible. I don't think it's > necessary to store IDs separately and loop over the extensions, though. The > installed extension set is a copy, and will hold on to references to the > Extensions, so you can just directly loop over that. That saves a lot of code. Thanks! https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:956: extension.get()->location() == Manifest::EXTERNAL_COMPONENT) { On 2014/11/12 18:23:00, kalman wrote: > Hm, it's surprising that this is the first time we've needed to check this > (component + external component + policy) condition. Perhaps add a method like > CannotBeDisabled, and we can justify its addition by DCHECKing in methods like > DisableExtension that CannotBeDisabled is false. It seems like this DCHECK in DisableExtension wouldn't be wise, according to what I see here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext.... Created the refactored method anywho, and called it to ensure we handle restoration of terminated extensions properly. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:963: // Blacklisted extensions remain in the blacklisted list. On 2014/11/12 18:23:00, kalman wrote: > What about terminated extensions? Those need to go in the locked list as well. I had assumed that a terminated extension was A) A separated bucket like blacklist that things don't move out of very often and B) was something that won't start up, since it's terminated... no? Anywho - Done. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:968: } On 2014/11/12 18:23:00, kalman wrote: > I don't actually think the "if"s here are necessary, Remove just won't do > anything if the ID isn't there. Just: > > registry_->RemoveEnabled(id); > registry_->RemoveDisabled(id); > registry_->RemoveTerminated(id); > // Important not to remove Blacklisted extensions! Done. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:985: NOTREACHED() << "Extension " << *it << " needs to be " On 2014/11/12 18:23:00, kalman wrote: > Again; not possible, and this code is perfectly safe (given you're taking a > reference to |extension|) to directly loop over the locked extensions and not > need to extract the IDs. Really? I can't loop directory of registry->locked_extensions(), given I'm calling RemoveLocked() within the loop. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:994: extension_prefs_->SetExtensionState(*it, Extension::ENABLED); On 2014/11/12 18:23:00, kalman wrote: > Again: terminated extensions. Done. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:298: // Extensions specified by |extension_ids| should become locked. Extensions On 2014/11/12 18:23:00, kalman wrote: > You might want to mention that "locking" does imply unloading all extensions. > "Locking" would easily be misinterpreted as preventing any changes whatsoever, > which isn't want the feature is. > > Funnily enough it almost seems like "blocked" is a better term for this whole > thing. You may in fact want to think of a more abstract term for this whole > thing, IIRC: > - the policy team was also looking at a feature to disable all extensions? > - this functionality would go nicely with a --disable-extensions or something > flag, or feature, which I'm not sure - maybe we already have? Done. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/695133005/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:62: extension_service->LockAllExtensions(); On 2014/11/12 19:23:51, Monica Dinculescu wrote: > Can this ever be NULL? I don't think so, so long as ENABLE_EXTENSIONS is defined. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:131: cache.ProfileIsSigninRequiredAtIndex(index)) { On 2014/11/12 19:23:51, Monica Dinculescu wrote: > I read this as "if this profile is locked, then unlock its extensions". Why is > the profile still locked if it's got a browser window opening? We intentionally don't unset the bit within user_manager_screen_handler until after the browser window's open. Re-wrote the comment to clarify. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:342: LockExtensions(profile_manager->GetProfileByPath(profile_path)); On 2014/11/12 19:23:51, Monica Dinculescu wrote: > I would either add a comment about how this profile is guaranteed to exist, and > we're not loading it, or try to pass the Profile pointer instead of the path. Comment written. https://codereview.chromium.org/695133005/diff/100001/extensions/browser/exte... File extensions/browser/extension_registry.h (right): https://codereview.chromium.org/695133005/diff/100001/extensions/browser/exte... extensions/browser/extension_registry.h:36: NONE = 0, On 2014/11/12 18:23:01, kalman wrote: > Did clang-format really do this? Damn :( oh well. It was much better the old > way, but can't argue with clang-format. git cl format - ya it did :*( https://codereview.chromium.org/695133005/diff/100001/extensions/common/exten... File extensions/common/extension.h (right): https://codereview.chromium.org/695133005/diff/100001/extensions/common/exten... extensions/common/extension.h:543: REASON_PROFILE_LOCK, // Profile is being locked. On 2014/11/12 19:23:51, Monica Dinculescu wrote: > nit: is being locked or has already been locked? Re-written. https://codereview.chromium.org/695133005/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/695133005/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43567: + <int value="4096" label="INACTIVE_EPHEMERAL_APP"/> On 2014/11/12 19:23:51, Monica Dinculescu wrote: > nit: Is this being used anywhere? Gets logged automatically in c/b/extensions/installed_loader::RecordDisbleReasonHistogram
Ok, more comments. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:963: // Blacklisted extensions remain in the blacklisted list. On 2014/11/12 20:58:42, Mike Lerman wrote: > On 2014/11/12 18:23:00, kalman wrote: > > What about terminated extensions? Those need to go in the locked list as well. > > I had assumed that a terminated extension was A) A separated bucket like > blacklist that things don't move out of very often and B) was something that > won't start up, since it's terminated... no? > > Anywho - Done. You can restart a terminated extension by going to chrome://extensions and hitting "reload" on a terminated extension, and you can terminate an extension by killing in the process manager. https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:985: NOTREACHED() << "Extension " << *it << " needs to be " On 2014/11/12 20:58:42, Mike Lerman wrote: > On 2014/11/12 18:23:00, kalman wrote: > > Again; not possible, and this code is perfectly safe (given you're taking a > > reference to |extension|) to directly loop over the locked extensions and not > > need to extract the IDs. > > Really? I can't loop directory of registry->locked_extensions(), given I'm > calling RemoveLocked() within the loop. Oh right, good point, I guess you need to take a copy of locked extensions. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/695133005/diff/160001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:478: case UnloadedExtensionInfo::REASON_UNDEFINED: Ugh, this UNDEFINED value only exists in a single place for a single test. I think it would be better to just break with "impossible" and put the NOTREACHED() outside the switch (because if you really want you can pass a Reason into this function which isn't actually a valid reason). https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:932: void ExtensionService::BlockAllExtensions() { Oh, you might want to check that it's not already blocked and/or there are no extensions in the current block list? https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:937: ExtensionIdSet to_lock; You can delete this now. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:939: const std::string& id = extension.get()->id(); just extension->id(); https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:948: Come to think of it it's probably also wise/safer not to have an Extension in both the blacklisted and blocked list, just to be safe. Hey here's an idea: how about adding a 2nd version of GenerateInstallExtensionsSet which takes an IncludeMask, then only include non-blacklisted extensions in here in the first place. Then you don't have to worry about that at all. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:950: extension_prefs_->SetExtensionState(id, Extension::BLOCKED); Actually - why do you need to change prefs at all? Blocking is a global state, you shouldn't need to change the extension *prefs*. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:959: ExtensionIdSet to_unblock = registry_->blocked_extensions().GetIDs(); (see previous point about just copying the whole extension set) https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:962: for (ExtensionIdSet::iterator it = to_unblock.begin(); (and use : syntax here too) https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:972: if (extensions_being_terminated_.find(extension->id()) != Mm, maybe this isn't actually useful. I doubt this set actually includes anything in it anymore. Perhaps just re-enabling terminated extensions would be fine, the important part is more just blocking them in the first place. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:976: if (extension_prefs_->GetDisableReasons(*it)) { GetDisableReasons returns a bitmask, so it's a little confusing doing if (GetDisableReasons). prefs_->IsExtensionDisabled(..) might be more appropriate. However, see comment above. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:2246: return (extension->location() != Manifest::COMPONENT && Outer parens are unnecessary. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:2339: UpdateBlockedExtensions(blocked, unchanged); Looks like the "blocked" term is already used, but it actually means blacklisting in this case, so could you rename this method to UpdateBlacklistedExtensions? https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:301: // blocked. Comment should explain what this method is supposed to do. For example: // Puts all extensions in a blocked state: unloading every extension, // and preventing them from ever loading until UnblockAllExtensions is called. // This state is stored in preferences, so persists until Chrome restarts. // // Component, external component, and whitelisted policy installed extensions // are exempt from being blocked (see CanBlockExtension). https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:306: void UnblockAllBlockedExtensions(); I think just "UnblockAllExtensions" is fine. And you might want to update the comment to match the one above.
LGTM for histograms.
profiles lgtm % nit. A test would be great -- I know you're working on it, but leaving it in writing which makes it real :) https://codereview.chromium.org/695133005/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/695133005/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1008: cache.ProfileIsSigninRequiredAtIndex( nit: can cache.Profile...( go on the previous line, so that you only indent once? I hate staircases :(
https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/100001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:963: // Blacklisted extensions remain in the blacklisted list. On 2014/11/12 23:41:42, kalman wrote: > On 2014/11/12 20:58:42, Mike Lerman wrote: > > On 2014/11/12 18:23:00, kalman wrote: > > > What about terminated extensions? Those need to go in the locked list as > well. > > > > I had assumed that a terminated extension was A) A separated bucket like > > blacklist that things don't move out of very often and B) was something that > > won't start up, since it's terminated... no? > > > > Anywho - Done. > > You can restart a terminated extension by going to chrome://extensions and > hitting "reload" on a terminated extension, and you can terminate an extension > by killing in the process manager. Acknowledged. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/695133005/diff/160001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:478: case UnloadedExtensionInfo::REASON_UNDEFINED: On 2014/11/12 23:41:43, kalman wrote: > Ugh, this UNDEFINED value only exists in a single place for a single test. I > think it would be better to just break with "impossible" and put the > NOTREACHED() outside the switch (because if you really want you can pass a > Reason into this function which isn't actually a valid reason). Should we remove REASON_UNDEFINED from the enum and then change the test? If it's not something that should really exist and has no meaning, maybe that makes more sense. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:932: void ExtensionService::BlockAllExtensions() { On 2014/11/12 23:41:43, kalman wrote: > Oh, you might want to check that it's not already blocked and/or there are no > extensions in the current block list? There's no reason this method couldn't be called multiple times. It logically won't the way the profiles code is implemented to call this, and shouldn't have any effect with the safeguard that's added to AddExtension(), but I see no harm in leaving it possible to be called multiple times. If we add a DCHECK or if()-return , I see some future developer looking over this code, needing to call it multiple times and saying, "But WHY was this prevented? What edge-case was the author preventing?" https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:937: ExtensionIdSet to_lock; On 2014/11/12 23:41:43, kalman wrote: > You can delete this now. Bye bye! https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:939: const std::string& id = extension.get()->id(); On 2014/11/12 23:41:43, kalman wrote: > just extension->id(); Done. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:948: On 2014/11/12 23:41:43, kalman wrote: > Come to think of it it's probably also wise/safer not to have an Extension in > both the blacklisted and blocked list, just to be safe. > > Hey here's an idea: how about adding a 2nd version of > GenerateInstallExtensionsSet which takes an IncludeMask, then only include > non-blacklisted extensions in here in the first place. Then you don't have to > worry about that at all. I'm concerned about removing things from the blacklist - it seems very important that blacklisted things stay that way, and I'd hate for a bug here to somehow result in something escaping the blacklist. Also, if I remove it from the blacklist, I need a foolproof way to put it back there in UnblockAllBlockedExtensions(), and I can't find a way to determine that. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:950: extension_prefs_->SetExtensionState(id, Extension::BLOCKED); On 2014/11/12 23:41:43, kalman wrote: > Actually - why do you need to change prefs at all? Blocking is a global state, > you shouldn't need to change the extension *prefs*. The goal was the keep the ExtensionState sync-ed with the ExtensionRegistry - having a state of DISABLED but not being in the disabled_extensions() seems like a problem waiting to happen. So I need to set the state to something, and made a new one. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:959: ExtensionIdSet to_unblock = registry_->blocked_extensions().GetIDs(); On 2014/11/12 23:41:43, kalman wrote: > (see previous point about just copying the whole extension set) Which one? Is making a copy of the set better than just referring to the IDs? Also, using the IDs is consistent with the implementations of similar loops in UpdateBlockedExtensions and UpdateGreylistedExtensions. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:962: for (ExtensionIdSet::iterator it = to_unblock.begin(); On 2014/11/12 23:41:43, kalman wrote: > (and use : syntax here too) Done. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:972: if (extensions_being_terminated_.find(extension->id()) != On 2014/11/12 23:41:43, kalman wrote: > Mm, maybe this isn't actually useful. I doubt this set actually includes > anything in it anymore. Perhaps just re-enabling terminated extensions would be > fine, the important part is more just blocking them in the first place. Done. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:976: if (extension_prefs_->GetDisableReasons(*it)) { On 2014/11/12 23:41:43, kalman wrote: > GetDisableReasons returns a bitmask, so it's a little confusing doing if > (GetDisableReasons). prefs_->IsExtensionDisabled(..) might be more appropriate. > > However, see comment above. I agree about using IsExtensionDisabled, if we have the state. Pending comment above resolution :) https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:2246: return (extension->location() != Manifest::COMPONENT && On 2014/11/12 23:41:43, kalman wrote: > Outer parens are unnecessary. Done. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:2339: UpdateBlockedExtensions(blocked, unchanged); On 2014/11/12 23:41:43, kalman wrote: > Looks like the "blocked" term is already used, but it actually means > blacklisting in this case, so could you rename this method to > UpdateBlacklistedExtensions? Done. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:301: // blocked. On 2014/11/12 23:41:43, kalman wrote: > Comment should explain what this method is supposed to do. For example: > > // Puts all extensions in a blocked state: unloading every extension, > // and preventing them from ever loading until UnblockAllExtensions is called. > // This state is stored in preferences, so persists until Chrome restarts. > // > // Component, external component, and whitelisted policy installed extensions > // are exempt from being blocked (see CanBlockExtension). Done, although I've never been an Oxford Comma man. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:306: void UnblockAllBlockedExtensions(); On 2014/11/12 23:41:43, kalman wrote: > I think just "UnblockAllExtensions" is fine. And you might want to update the > comment to match the one above. Done. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/695133005/diff/160001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1008: cache.ProfileIsSigninRequiredAtIndex( On 2014/11/13 15:24:54, Monica Dinculescu wrote: > nit: can cache.Profile...( go on the previous line, so that you only indent > once? I hate staircases :( Done.
Responding to your comments before I have another look at the current code. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:948: On 2014/11/13 15:30:05, Mike Lerman wrote: > On 2014/11/12 23:41:43, kalman wrote: > > Come to think of it it's probably also wise/safer not to have an Extension in > > both the blacklisted and blocked list, just to be safe. > > > > Hey here's an idea: how about adding a 2nd version of > > GenerateInstallExtensionsSet which takes an IncludeMask, then only include > > non-blacklisted extensions in here in the first place. Then you don't have to > > worry about that at all. > > I'm concerned about removing things from the blacklist - it seems very important > that blacklisted things stay that way, and I'd hate for a bug here to somehow > result in something escaping the blacklist. > > Also, if I remove it from the blacklist, I need a foolproof way to put it back > there in UnblockAllBlockedExtensions(), and I can't find a way to determine > that. I'm not suggesting removing it from the blacklist. I'm suggesting that you exclude the blacklisted extensions from ever actually entering the installed extensions list by the method I mentioned above. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:950: extension_prefs_->SetExtensionState(id, Extension::BLOCKED); On 2014/11/13 15:30:04, Mike Lerman wrote: > On 2014/11/12 23:41:43, kalman wrote: > > Actually - why do you need to change prefs at all? Blocking is a global state, > > you shouldn't need to change the extension *prefs*. > > The goal was the keep the ExtensionState sync-ed with the ExtensionRegistry - > having a state of DISABLED but not being in the disabled_extensions() seems like > a problem waiting to happen. So I need to set the state to something, and made a > new one. I don't think so. For example we keep terminated extensions in their enabled/disabled prefs. It will simplify things considerably if you don't make any changes to the prefs. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:959: ExtensionIdSet to_unblock = registry_->blocked_extensions().GetIDs(); On 2014/11/13 15:30:04, Mike Lerman wrote: > On 2014/11/12 23:41:43, kalman wrote: > > (see previous point about just copying the whole extension set) > > Which one? > > Is making a copy of the set better than just referring to the IDs? Also, using > the IDs is consistent with the implementations of similar loops in > UpdateBlockedExtensions and UpdateGreylistedExtensions. Maybe they should copy the set as well. Needing 7 lines to deal with fetching the extension back out of the list again is avoidable work (my point being confusing to read rather than computationally efficient, though it probably is also more efficient).
https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:948: On 2014/11/13 18:09:47, kalman wrote: > On 2014/11/13 15:30:05, Mike Lerman wrote: > > On 2014/11/12 23:41:43, kalman wrote: > > > Come to think of it it's probably also wise/safer not to have an Extension > in > > > both the blacklisted and blocked list, just to be safe. > > > > > > Hey here's an idea: how about adding a 2nd version of > > > GenerateInstallExtensionsSet which takes an IncludeMask, then only include > > > non-blacklisted extensions in here in the first place. Then you don't have > to > > > worry about that at all. > > > > I'm concerned about removing things from the blacklist - it seems very > important > > that blacklisted things stay that way, and I'd hate for a bug here to somehow > > result in something escaping the blacklist. > > > > Also, if I remove it from the blacklist, I need a foolproof way to put it back > > there in UnblockAllBlockedExtensions(), and I can't find a way to determine > > that. > > I'm not suggesting removing it from the blacklist. I'm suggesting that you > exclude the blacklisted extensions from ever actually entering the installed > extensions list by the method I mentioned above. Done. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:950: extension_prefs_->SetExtensionState(id, Extension::BLOCKED); On 2014/11/13 18:09:47, kalman wrote: > On 2014/11/13 15:30:04, Mike Lerman wrote: > > On 2014/11/12 23:41:43, kalman wrote: > > > Actually - why do you need to change prefs at all? Blocking is a global > state, > > > you shouldn't need to change the extension *prefs*. > > > > The goal was the keep the ExtensionState sync-ed with the ExtensionRegistry - > > having a state of DISABLED but not being in the disabled_extensions() seems > like > > a problem waiting to happen. So I need to set the state to something, and made > a > > new one. > > I don't think so. For example we keep terminated extensions in their > enabled/disabled prefs. It will simplify things considerably if you don't make > any changes to the prefs. Done. https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:959: ExtensionIdSet to_unblock = registry_->blocked_extensions().GetIDs(); On 2014/11/13 18:09:47, kalman wrote: > On 2014/11/13 15:30:04, Mike Lerman wrote: > > On 2014/11/12 23:41:43, kalman wrote: > > > (see previous point about just copying the whole extension set) > > > > Which one? > > > > Is making a copy of the set better than just referring to the IDs? Also, using > > the IDs is consistent with the implementations of similar loops in > > UpdateBlockedExtensions and UpdateGreylistedExtensions. > > Maybe they should copy the set as well. Needing 7 lines to deal with fetching > the extension back out of the list again is avoidable work (my point being > confusing to read rather than computationally efficient, though it probably is > also more efficient). Copied as well. Done.
Hi Benjamin or Drew, Any further comments for this CL? Thanks, Mike On Thu, Nov 13, 2014, 4:42 PM null <mlerman@chromium.org> wrote: > > https://codereview.chromium.org/695133005/diff/160001/ > chrome/browser/extensions/extension_service.cc > File chrome/browser/extensions/extension_service.cc (right): > > https://codereview.chromium.org/695133005/diff/160001/ > chrome/browser/extensions/extension_service.cc#newcode948 > chrome/browser/extensions/extension_service.cc:948 > <https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio...> > : > On 2014/11/13 18:09:47, kalman wrote: > > On 2014/11/13 15:30:05, Mike Lerman wrote: > > > On 2014/11/12 23:41:43, kalman wrote: > > > > Come to think of it it's probably also wise/safer not to have an > Extension > > in > > > > both the blacklisted and blocked list, just to be safe. > > > > > > > > Hey here's an idea: how about adding a 2nd version of > > > > GenerateInstallExtensionsSet which takes an IncludeMask, then only > include > > > > non-blacklisted extensions in here in the first place. Then you > don't have > > to > > > > worry about that at all. > > > > > > I'm concerned about removing things from the blacklist - it seems > very > > important > > > that blacklisted things stay that way, and I'd hate for a bug here > to somehow > > > result in something escaping the blacklist. > > > > > > Also, if I remove it from the blacklist, I need a foolproof way to > put it back > > > there in UnblockAllBlockedExtensions(), and I can't find a way to > determine > > > that. > > > I'm not suggesting removing it from the blacklist. I'm suggesting that > you > > exclude the blacklisted extensions from ever actually entering the > installed > > extensions list by the method I mentioned above. > > Done. > > https://codereview.chromium.org/695133005/diff/160001/ > chrome/browser/extensions/extension_service.cc#newcode950 > chrome/browser/extensions/extension_service.cc:950 > <https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio...> > : > extension_prefs_->SetExtensionState(id, Extension::BLOCKED); > On 2014/11/13 18:09:47, kalman wrote: > > On 2014/11/13 15:30:04, Mike Lerman wrote: > > > On 2014/11/12 23:41:43, kalman wrote: > > > > Actually - why do you need to change prefs at all? Blocking is a > global > > state, > > > > you shouldn't need to change the extension *prefs*. > > > > > > The goal was the keep the ExtensionState sync-ed with the > ExtensionRegistry - > > > having a state of DISABLED but not being in the > disabled_extensions() seems > > like > > > a problem waiting to happen. So I need to set the state to > something, and made > > a > > > new one. > > > I don't think so. For example we keep terminated extensions in their > > enabled/disabled prefs. It will simplify things considerably if you > don't make > > any changes to the prefs. > > Done. > > https://codereview.chromium.org/695133005/diff/160001/ > chrome/browser/extensions/extension_service.cc#newcode959 > chrome/browser/extensions/extension_service.cc:959 > <https://codereview.chromium.org/695133005/diff/160001/chrome/browser/extensio...>: > ExtensionIdSet > to_unblock = registry_->blocked_extensions().GetIDs(); > On 2014/11/13 18:09:47, kalman wrote: > > On 2014/11/13 15:30:04, Mike Lerman wrote: > > > On 2014/11/12 23:41:43, kalman wrote: > > > > (see previous point about just copying the whole extension set) > > > > > > Which one? > > > > > > Is making a copy of the set better than just referring to the IDs? > Also, using > > > the IDs is consistent with the implementations of similar loops in > > > UpdateBlockedExtensions and UpdateGreylistedExtensions. > > > Maybe they should copy the set as well. Needing 7 lines to deal with > fetching > > the extension back out of the list again is avoidable work (my point > being > > confusing to read rather than computationally efficient, though it > probably is > > also more efficient). > > Copied as well. Done. > > https://codereview.chromium.org/695133005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm with a few nits/questions. Also not 100% convinced that force installed extensions should remain installed when profiles are locked - seems like the right approach, but concerned that you could still get UI leakage/notifications, windows opening, things showing up in the background app list, etc from these force-installed extensions. https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:465: return; Why did this change from break to return? https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:478: case UnloadedExtensionInfo::REASON_UNDEFINED: I think this line is mis-indented. Also, no default case - is that OK? https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:928: // Extensions that are not locked, components or forced by policy should be So, if I have a force-installed extension that is going to do things in the background (display notifications, etc) - that will still happen, even though the profile is locked?
https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:465: return; On 2014/11/17 16:09:24, Andrew T Wilson wrote: > Why did this change from break to return? Because I asked Mike to, see below. https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:478: case UnloadedExtensionInfo::REASON_UNDEFINED: On 2014/11/17 16:09:24, Andrew T Wilson wrote: > I think this line is mis-indented. Also, no default case - is that OK? No default case is good for compilation, however you're right that this function isn't quite correct. https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:479: NOTREACHED(); I think there is a bit of miscommunication here - sorry. I mean: 1. Fix Andrew's indentation comment. 2. Replace this case with "// Fall through to undefined case." https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:483: } Then here write: NOTREACHED() << "Undefined case " << reason; return ShutdownAssociatedBackgroundContents( base::ASCIIToUTF16(extension->id())); https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:928: // Extensions that are not locked, components or forced by policy should be On 2014/11/17 16:09:25, Andrew T Wilson wrote: > So, if I have a force-installed extension that is going to do things in the > background (display notifications, etc) - that will still happen, even though > the profile is locked? Yeah, good point. You could argue it either way, but I do think we need to keep many features of force-installed extensions active (like proxy settings, security key) - and like you say, others not - but it would unfortunately add complexity and maintenance burden (and potentially breaking changes anyway) to try to implement this. https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:932: block_extensions_ = true; Can you early-return from this function if it's already true? https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:939: for (const scoped_refptr<const Extension> extension : *extensions.get()) { 1. Make it a const scoped_refptr<const Extension>& (i.e. reference) 2. You need the .get() if you're dereferencing a scoped_refptr. https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:961: for (const scoped_refptr<const Extension> extension : *to_unblock.get()) { Also use a reference, no .get(). https://codereview.chromium.org/695133005/diff/260001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/695133005/diff/260001/extensions/browser/exte... extensions/browser/extension_prefs.h:446: bool IsExtensionBlocked(const std::string& id) const; I said in a previous comment there should be no need to store in prefs if an extension is blocked since blocked is a global not a per-extension state, and you said Done. Why do you still need this? https://codereview.chromium.org/695133005/diff/260001/extensions/common/exten... File extensions/common/extension.h (right): https://codereview.chromium.org/695133005/diff/260001/extensions/common/exten... extensions/common/extension.h:66: BLOCKED, We should keep this in order since it's stored in prefs. In fact could you add a big comment on this enum to only add values at the bottom?
https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:479: NOTREACHED(); On 2014/11/17 17:04:20, kalman wrote: > I think there is a bit of miscommunication here - sorry. I mean: > > 1. Fix Andrew's indentation comment. > 2. Replace this case with "// Fall through to undefined case." Done. https://codereview.chromium.org/695133005/diff/260001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:483: } On 2014/11/17 17:04:20, kalman wrote: > Then here write: > > NOTREACHED() << "Undefined case " << reason; > return ShutdownAssociatedBackgroundContents( > base::ASCIIToUTF16(extension->id())); Done. https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:932: block_extensions_ = true; On 2014/11/17 17:04:20, kalman wrote: > Can you early-return from this function if it's already true? Done. https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:939: for (const scoped_refptr<const Extension> extension : *extensions.get()) { On 2014/11/17 17:04:20, kalman wrote: > 1. Make it a const scoped_refptr<const Extension>& (i.e. reference) > 2. You need the .get() if you're dereferencing a scoped_refptr. Done. https://codereview.chromium.org/695133005/diff/260001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:961: for (const scoped_refptr<const Extension> extension : *to_unblock.get()) { On 2014/11/17 17:04:20, kalman wrote: > Also use a reference, no .get(). Done. https://codereview.chromium.org/695133005/diff/260001/extensions/browser/exte... File extensions/browser/extension_prefs.h (right): https://codereview.chromium.org/695133005/diff/260001/extensions/browser/exte... extensions/browser/extension_prefs.h:446: bool IsExtensionBlocked(const std::string& id) const; On 2014/11/17 17:04:20, kalman wrote: > I said in a previous comment there should be no need to store in prefs if an > extension is blocked since blocked is a global not a per-extension state, and > you said Done. Why do you still need this? Removed. https://codereview.chromium.org/695133005/diff/260001/extensions/common/exten... File extensions/common/extension.h (right): https://codereview.chromium.org/695133005/diff/260001/extensions/common/exten... extensions/common/extension.h:66: BLOCKED, On 2014/11/17 17:04:20, kalman wrote: > We should keep this in order since it's stored in prefs. In fact could you add a > big comment on this enum to only add values at the bottom? I'll remove this state since I'm also removing IsExtensionBlocked from extension_prefs, and blocked-ness is global rather than extension specific. Comment added for future selves. https://codereview.chromium.org/695133005/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/695133005/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:567: bool CanBlockExtension(const extensions::Extension* extension) const; Made this const so it can be called from IsExtensionEnabled(). Also, it should be const.
Patch looks fine, though I thought you had a test earlier? Maybe I'm remembering incorrectly. It would be great to have a test (extension_service_unittest.cc would be appropriate, if daunting - you can see the blacklisting tests in there, though). https://codereview.chromium.org/695133005/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/695133005/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:299: // preventing them from ever loading until UnlockAllExtensions is called. This Seeing the "lock" word here not "block" (which is what the functions use), also in the enum value. Should be consistent. https://codereview.chromium.org/695133005/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:303: // are exempt from beck Blocked (see CanBlockExtension). beck --> being https://codereview.chromium.org/695133005/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:567: bool CanBlockExtension(const extensions::Extension* extension) const; On 2014/11/19 14:54:34, Mike Lerman wrote: > Made this const so it can be called from IsExtensionEnabled(). Also, it should > be const. Thanks. There are a lot of functions that should be const on this interface, maybe I'll fix them up after your CL goes in. https://codereview.chromium.org/695133005/diff/280001/chrome/common/extension... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/695133005/diff/280001/chrome/common/extension... chrome/common/extensions/api/_permission_features.json:932: "location": "component", Why did you need to make this change? https://codereview.chromium.org/695133005/diff/280001/extensions/browser/exte... File extensions/browser/extension_registry.h (right): https://codereview.chromium.org/695133005/diff/280001/extensions/browser/exte... extensions/browser/extension_registry.h:70: // blocked extensions. Maybe you can simplify this comment to "Returns the set of all installed extensions, regardless of state (enabled, disabled, etc). Equivalent to GenerateInstalledExtensionSet(EVERYTHING).
Hi kalman, I've addressed your comments and added unit tests for the extension_service. I'm working on another test (which I'll land separately) that tests the integration from the Profiles code (and the User Manager) to the extensions service, but that work will also address other issues in those tests so I'd like to keep it separate. Thanks! https://codereview.chromium.org/695133005/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/695133005/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:299: // preventing them from ever loading until UnlockAllExtensions is called. This On 2014/11/19 20:20:41, kalman wrote: > Seeing the "lock" word here not "block" (which is what the functions use), also > in the enum value. Should be consistent. Changed to block. https://codereview.chromium.org/695133005/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:303: // are exempt from beck Blocked (see CanBlockExtension). On 2014/11/19 20:20:41, kalman wrote: > beck --> being Done. https://codereview.chromium.org/695133005/diff/280001/chrome/common/extension... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/695133005/diff/280001/chrome/common/extension... chrome/common/extensions/api/_permission_features.json:932: "location": "component", On 2014/11/19 20:20:42, kalman wrote: > Why did you need to make this change? After consultation with isherman@, who works on this API and Component. Since I changed the screenlock_private_apitest to always run as a component extension for Desktop, any future extensions which access this API (of which there are currently none) should have to be components. The Blocked-Extension rule would also require this - screen unlocking extensions need to be available while the Profile's locked, and if they're not components then they won't be loaded. https://codereview.chromium.org/695133005/diff/280001/extensions/browser/exte... File extensions/browser/extension_registry.h (right): https://codereview.chromium.org/695133005/diff/280001/extensions/browser/exte... extensions/browser/extension_registry.h:70: // blocked extensions. On 2014/11/19 20:20:42, kalman wrote: > Maybe you can simplify this comment to "Returns the set of all installed > extensions, regardless of state (enabled, disabled, etc). Equivalent to > GenerateInstalledExtensionSet(EVERYTHING). Done.
Cool! https://codereview.chromium.org/695133005/diff/280001/chrome/common/extension... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/695133005/diff/280001/chrome/common/extension... chrome/common/extensions/api/_permission_features.json:932: "location": "component", On 2014/11/20 17:18:33, Mike Lerman wrote: > On 2014/11/19 20:20:42, kalman wrote: > > Why did you need to make this change? > > After consultation with isherman@, who works on this API and Component. Since I > changed the screenlock_private_apitest to always run as a component extension > for Desktop, any future extensions which access this API (of which there are > currently none) should have to be components. The Blocked-Extension rule would > also require this - screen unlocking extensions need to be available while the > Profile's locked, and if they're not components then they won't be loaded. Alright, it just seemed orthogonal to your API. Though this compound screenlockPrivate rule is now a bit odd. It's available - on chromeos, for any component extension - on other platforms, for only specific component extensions I like the explicit whitelist, though if it's a component extension I don't really see the point. Seems like the whole rule could basically be "screenlockPrivate": { "channel": "stable", "extension_types": ["platform_app"], "location": "component" } https://codereview.chromium.org/695133005/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/695133005/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:3604: !blocked_extensions.Contains(good2)); It's better to separate the assertions rather than using &&, that way the failure message is more meaningful. You know exactly what condition failed, rather than it being one of two possible conditions. You could do this neatly with a helper function (i.e. protected function on ExtensionServiceTest) like this: testing::AssertionResult IsBlocked(const std::string& id) { if (registry()->enabled_extensions().Contains(id)) return testing:AssertionFailure() << id << " is still enabled"; if (!registry()->blocked_extensions().Contains(id)) return testing::AssertionFailure() << id << " is not blocked"; return testing::AssertionSuccess(); } ... EXPECT_TRUE(IsBlocked(good0)); EXPECT_TRUE(IsBlocked(good1)); ... You could even put some more/stronger assertions in that IsBlocked method. For example, rather than enabled_extensions() use GenerateInstallExtensionSet(EVERYTHING & ~BLOCKED) so that you get ever set other than blocked. https://codereview.chromium.org/695133005/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:3620: I think it's also important to test some more cases here: - a blacklisted extension, both one that is blacklisted before initialising (make sure it's never blocked nor unblocked) and one that is blacklisted before unblocking (likewise). - a disable extension, similar situation. - a terminated extension, similar situation. https://codereview.chromium.org/695133005/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:3630: // extensions. If possible could you also put policy extensions in here (if it's too hard then don't worry about it, the code is fairly straightforward to see that you *do* block it, but test coverage is nice)
Ilya, Are you okay with kalman@'s suggestion, re: https://codereview.chromium.org/695133005/diff/280001/chrome/common/extension... I have no issues with it, but you're more familiar with the direction around screenlock than I.
isherman@chromium.org changed reviewers: + isherman@chromium.org
https://codereview.chromium.org/695133005/diff/280001/chrome/common/extension... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/695133005/diff/280001/chrome/common/extension... chrome/common/extensions/api/_permission_features.json:932: "location": "component", On 2014/11/20 18:27:20, kalman wrote: > On 2014/11/20 17:18:33, Mike Lerman wrote: > > On 2014/11/19 20:20:42, kalman wrote: > > > Why did you need to make this change? > > > > After consultation with isherman@, who works on this API and Component. Since > I > > changed the screenlock_private_apitest to always run as a component extension > > for Desktop, any future extensions which access this API (of which there are > > currently none) should have to be components. The Blocked-Extension rule would > > also require this - screen unlocking extensions need to be available while the > > Profile's locked, and if they're not components then they won't be loaded. > > Alright, it just seemed orthogonal to your API. > > Though this compound screenlockPrivate rule is now a bit odd. It's available > - on chromeos, for any component extension > - on other platforms, for only specific component extensions > > I like the explicit whitelist, though if it's a component extension I don't > really see the point. Seems like the whole rule could basically be > > "screenlockPrivate": { > "channel": "stable", > "extension_types": ["platform_app"], > "location": "component" > } Agreed, that's what I had intended to nudge Mike towards. Sorry for any confusion!
Patchset #17 (id:320001) has been deleted
mlerman@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi kalman@, Added more unit tests, covering: enabled, disabled, forced-add-by-policy, blacklisted (before and after init()), theme, component extension, terminated each of which goes through the block-unblock cycle and verifies state before/during/after. Also one test for if we block before Init(). pkotwicz@, please review the change in themes. I'm adding a Profile Lock feature which closes all browsers and requires a password at the User Manager before they can re-open. Themes should not be cleared when we do this. Thanks! https://codereview.chromium.org/695133005/diff/300001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/695133005/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:3604: !blocked_extensions.Contains(good2)); On 2014/11/20 18:27:20, kalman wrote: > It's better to separate the assertions rather than using &&, that way the > failure message is more meaningful. You know exactly what condition failed, > rather than it being one of two possible conditions. > > You could do this neatly with a helper function (i.e. protected function on > ExtensionServiceTest) like this: > > testing::AssertionResult IsBlocked(const std::string& id) { > if (registry()->enabled_extensions().Contains(id)) > return testing:AssertionFailure() << id << " is still enabled"; > if (!registry()->blocked_extensions().Contains(id)) > return testing::AssertionFailure() << id << " is not blocked"; > return testing::AssertionSuccess(); > } > > ... > > EXPECT_TRUE(IsBlocked(good0)); > EXPECT_TRUE(IsBlocked(good1)); > ... > > You could even put some more/stronger assertions in that IsBlocked method. For > example, rather than enabled_extensions() use > GenerateInstallExtensionSet(EVERYTHING & ~BLOCKED) so that you get ever set > other than blocked. Done. https://codereview.chromium.org/695133005/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:3620: On 2014/11/20 18:27:20, kalman wrote: > I think it's also important to test some more cases here: > > - a blacklisted extension, both one that is blacklisted before initialising > (make sure it's never blocked nor unblocked) and one that is blacklisted before > unblocking (likewise). > > - a disable extension, similar situation. > > - a terminated extension, similar situation. Wrote a helper method to refactor the boiler plate involved here. Trying to help prevent this file from hitting the 8000 line mark ;) https://codereview.chromium.org/695133005/diff/300001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:3630: // extensions. On 2014/11/20 18:27:20, kalman wrote: > If possible could you also put policy extensions in here (if it's too hard then > don't worry about it, the code is fairly straightforward to see that you *do* > block it, but test coverage is nice) I presume by "policy extension" you mean an extension that was force-installed by policy, and that should be prohibited from blocking? If so - Done. https://codereview.chromium.org/695133005/diff/340001/chrome/browser/themes/t... File chrome/browser/themes/theme_service.cc (right): https://codereview.chromium.org/695133005/diff/340001/chrome/browser/themes/t... chrome/browser/themes/theme_service.cc:303: unloaded_details->reason != UnloadedExtensionInfo::REASON_LOCK_ALL && We don't want to change the profile's theme if we're blocking the extensions while the profile's locked - the theme will need to come right back after we unlock it.
lgtm
ping pkotwicz@ Let me know if you have any questions about this CL!
lgtm
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/695133005/360001
Message was sent while issue was closed.
Committed patchset #18 (id:360001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/6a37b6a4b4914a5e762f90b673d1711ab8df7770 Cr-Commit-Position: refs/heads/master@{#305897}
Message was sent while issue was closed.
binjin@chromium.org changed reviewers: + binjin@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/695133005/diff/360001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/695133005/diff/360001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:44127: - <int value="8192" label="EXTERNAL_EXTENSION"/> Is there a reason to remove this disable reason? Or it's just a mistake in rebasing.
Message was sent while issue was closed.
https://codereview.chromium.org/695133005/diff/360001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/695133005/diff/360001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:44127: - <int value="8192" label="EXTERNAL_EXTENSION"/> On 2014/11/27 12:44:26, binjin wrote: > Is there a reason to remove this disable reason? Or it's just a mistake in > rebasing. Apologies. Fixing in: https://codereview.chromium.org/763783002/ |
