Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23427003:
Reload force-installed extensions on crash/force-close (Closed)
|
Created:
7 years, 3 months ago by anitawoodruff Modified:
7 years, 3 months ago Reviewers:
bartfab (slow), asargent_no_longer_on_chrome, Andrew T Wilson (Slow), not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, Joao da Silva Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionReload force-installed extensions on crash/force-close
Previously, users could disable force-installed extensions by killing the
process via the Chrome Task Manager or other means. This fix prevents that
by reloading force-installed extensions if they crash.
Also added an 'extension misbehaving' popup to inform the user if we get
stuck in a crash/reload loop, so they know to inform their administrator.
BUG=175701
TEST=Updated browser_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220586
Patch Set 1 #
Total comments: 70
Patch Set 2 : Responding to Bartosz' review (take 2) #
Total comments: 33
Patch Set 3 : Refer to 'apps/extensions' instead of 'extensions' everywhere #Patch Set 4 : Safety checks: post delayed task with weak ptr & null-check extension before reload #Patch Set 5 : Responding to Bartosz' comments on PS2. #
Total comments: 39
Patch Set 6 : Responding to Bartosz' comments on PS5 #
Total comments: 4
Patch Set 7 : Fixing nits from PS6 #
Total comments: 3
Patch Set 8 : Use isValidProfile check instead of WeakPtr method #
Total comments: 4
Patch Set 9 : Fixing nits #Patch Set 10 : Removing 'OVERRIDE' notation from .cc file which was causing some tests to fail #Patch Set 11 : good2.crx was committed manually, this should now work! #
Messages
Total messages: 31 (0 generated)
Adding Bartosz as reviewer & cc-ing Joao.
https://codereview.chromium.org/23427003/diff/1/chrome/app/generated_resource... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/23427003/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:274: + <message name="IDS_BACKGROUND_MISBEHAVING_APP_BALLOON_MESSAGE" desc="The contents of the balloon that is displayed when a background force-installed app crashes multiple times"> Nit: Your commit message talks about extensions only. It seems like it should be saying "app/extension" istead. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... File chrome/browser/background/background_contents_service.cc (left): https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:110: // Closing the balloon here should be OK, but it causes a crash on Mac Nit: "the balloon" is no longer unambiguous. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:60: /** No. of recent crashes required to trigger an extension-misbehave popup **/ Nit 1: Do not use abbreviations: No. => Number Nit 2: Note that this applies to force-installed apps/extensions only. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:61: const unsigned int kNCrashesShouldNotify = 5; Nit: This constant does not follow the usual Chrome constant naming. It should more be something like kForceInstalledExtensionCrashNotificationThreshold. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:67: void ScheduleCloseBalloon(const std::string& id) { Nit 1: "extension_id" identifies an extension; easy. What does "id" identify? Nit 2: Add a comment that explains what these three methods do and how they differ. The differences are subtle and hard to spot. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:149: class MisbehaveNotificationDelegate : public NotificationDelegate { Nit 1: #include "chrome/browser/notifications/notification_delegate.h" Nit 2: Add comments that note what CrashNotificationDelegate and MisbehaveNotificationDelegate are for. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:212: void ShowBalloon(const Extension* extension, Profile* profile, bool isForced) { Nit: Add a comment explaining the parameters. "isForced" sounds like the balloon is being forced :). https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:231: scoped_refptr<NotificationDelegate> delegate = NULL; Nit 1: You could set the delegate in the if-block above. Nit 2: scoped_refptr initializes itself to NULL automatically. No need to do it explicitly. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:271: /** Delay (in seconds) before restarting a forced extension that crashed **/ Nit 1: s/forced/force-installed/ Nit 2: Move the comment to the header file instead. Nit 3: We use //, not /** **/ in Chrome. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:275: /** Seconds since a crash-reload during which we listen for further crashes **/ This comment is not quite correct. With the default settings, we wait for 5 crashes to happen in a 5 * (3 + 1) = 20 second window. 5 * 3 = 15 seconds are just waiting for restarts to happen. But the crashes can occur anywhere within the remaining 5 seconds, not just one crash per 1-second window. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:457: // an EXTENSION_UNLOADED notification. We post the crash handling code as The previous comment had more information on why popping up the balloon straight away is not the right thing to do. Please re-add that part. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:467: // Restart the extension; notify user if crash recurs frequently. I think this code is complex enough to warrant putting it into a separate method. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:468: const std::string& id = extension->id(); Nit: Why not call the variable extension_id? https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:474: if (!alreadyNotified && crashes.size() == (kNCrashesShouldNotify - 1) && Please put the calculation on its own line and explain what it does. It is too complex to follow like this IMHO. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:489: base::MessageLoop::current()->PostDelayedTask(FROM_HERE, This task will run 3s later. What if |extension| or |profile| (both pointers) becomes invalid before that, e.g. because the extension is uninstalled during the 3s window? * For |profile|, you may be OK. You only use it in ExtensionSystem::Get(profile). If that method can handle a nonexistent profile (you should read its code to make sure this is the case), you are good. * For |extension|, accessing |extension->id()| in the delayed task is dangerous. You could access extension->id() here now and pass the resulting |std::string| to the delayed task. Make sure though that extension_service()->ReloadExtension() can handle an extension id that may no longer exist. * What will ReloadExtension() do if the extension has been uninstalled since - will it reinstall it? That would be bad. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:765: ScheduleCloseBalloons(UTF16ToASCII(details->application_id)); Will this not close the "misbehaving app" balloon when the app has reloaded? IIUC, the balloon was meant to stay up, even if the app keeps crashing and reloading. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:56: static void SetCrashDelaysForTesting(const base::TimeDelta& restart_delay, Nit 1: Add comment that explain what this does. Nit 2: The name may be a bit too generic. Since these delays apply to force-installed extensions only, it would be best to mention that in the method name. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:177: static base::TimeDelta restart_delay_; Static initializers are not allowed in Chrome. You can have static PODs but not complex classes. You will have to make the statics PODs (say ints holding the number of milliseconds or base::TimeDelta::ToInternalValue()s). https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:200: // Map associating extension IDs with their most recent crash timestamps. Nit: Add a note that this is used for force-installed apps/extensions only. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:203: std::map<std::string, std::queue<base::TimeTicks> > extension_crashlog_map_; Nit 1: #include <queue> Nit 2: #include "base/time.h" https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:206: // 'Extension is misbehaving' notifications. Nit 1: Instead of "sent notifications," better say "shown ballons". Nit 2: Add a note that this applies to force-installed apps/extensions only. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:207: std::set<std::string> misbehaving_extensions_; Nit: #include <set> https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... File chrome/browser/policy/policy_browsertest.cc (left): https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:1395: Nit: Why remove the blank line? https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:458: // This class is a customised version of WindowedNotificationObserver to allow Nit: s/WindowedNotificationObserver/content::WindowedNotificationObserver/ https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:459: // us to register and wait until one of two simple events has occurred. See Nit: "simple events" is nomenclature I introduced in WindowedNotificationObserver only. It is not what you would normally use in Chrome. The comment should more be something along the lines of: This is a customized version of content::WindowedNotificationObserver that waits until either of the two provided notification types is obseved. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:463: // Set up to wait for one of two conditions. The condition is met when a Nit: As above, "condition" is not the right term here. "Notification" is. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:467: Nit: Remove this blank line. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:470: // Wait until the specified condition is met. If the condition is already met Nit: As above, "notification" instead of "condition". https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:475: // NotificationObserver: Nit: s/NotificationObserver/content::NotificationObserver/ https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:502: return; Nit: Indent 2 spaces, not 4. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:1473: // Wait for all the extension render view hosts that exist to finish loading. Nit: Technically, this comment is correct. But it does not explain why you are waiting for these RVHs to finish loading. Better use something like: // Wait until any background pages belonging to force-installed extensions have been loaded. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:1494: // Test policy-installed extensions are reloaded when killed. Nit: Add a blank line above.
Hey Bartosz, Sorry, afraid all my resyncing in between has rendered it incapable of showing side-by-side diffs with patch set 1. Is there anything we can do about this? (It would be quite helpful to see them side by side because the test has started failing and I don't know why!). Also, chrome still isn't working on master.. so I think it's time to go home now. See you in the morning! https://codereview.chromium.org/23427003/diff/1/chrome/app/generated_resource... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/23427003/diff/1/chrome/app/generated_resource... chrome/app/generated_resources.grd:274: + <message name="IDS_BACKGROUND_MISBEHAVING_APP_BALLOON_MESSAGE" desc="The contents of the balloon that is displayed when a background force-installed app crashes multiple times"> On 2013/08/26 13:26:53, bartfab wrote: > Nit: Your commit message talks about extensions only. It seems like it should be > saying "app/extension" istead. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... File chrome/browser/background/background_contents_service.cc (left): https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:110: // Closing the balloon here should be OK, but it causes a crash on Mac On 2013/08/26 13:26:53, bartfab wrote: > Nit: "the balloon" is no longer unambiguous. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:60: /** No. of recent crashes required to trigger an extension-misbehave popup **/ On 2013/08/26 13:26:53, bartfab wrote: > Nit 1: Do not use abbreviations: No. => Number > Nit 2: Note that this applies to force-installed apps/extensions only. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:61: const unsigned int kNCrashesShouldNotify = 5; On 2013/08/26 13:26:53, bartfab wrote: > Nit: This constant does not follow the usual Chrome constant naming. It should > more be something like kForceInstalledExtensionCrashNotificationThreshold. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:67: void ScheduleCloseBalloon(const std::string& id) { On 2013/08/26 13:26:53, bartfab wrote: > Nit 1: "extension_id" identifies an extension; easy. What does "id" identify? > Done - is prefixed_extension_id ok? Or should I make up a new concept such as balloon_id ? > Nit 2: Add a comment that explains what these three methods do and how they > differ. The differences are subtle and hard to spot. Done. Is this clear enough now? https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:149: class MisbehaveNotificationDelegate : public NotificationDelegate { On 2013/08/26 13:26:53, bartfab wrote: > Nit 1: #include "chrome/browser/notifications/notification_delegate.h" > > Nit 2: Add comments that note what CrashNotificationDelegate and > MisbehaveNotificationDelegate are for. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:212: void ShowBalloon(const Extension* extension, Profile* profile, bool isForced) { On 2013/08/26 13:26:53, bartfab wrote: > Nit: Add a comment explaining the parameters. "isForced" sounds like the balloon > is being forced :). Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:231: scoped_refptr<NotificationDelegate> delegate = NULL; On 2013/08/26 13:26:53, bartfab wrote: > Nit 1: You could set the delegate in the if-block above. > Nit 2: scoped_refptr initializes itself to NULL automatically. No need to do it > explicitly. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:271: /** Delay (in seconds) before restarting a forced extension that crashed **/ On 2013/08/26 13:26:53, bartfab wrote: > Nit 1: s/forced/force-installed/ > Nit 2: Move the comment to the header file instead. > Nit 3: We use //, not /** **/ in Chrome. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:275: /** Seconds since a crash-reload during which we listen for further crashes **/ On 2013/08/26 13:26:53, bartfab wrote: > This comment is not quite correct. With the default settings, we wait for 5 > crashes to happen in a 5 * (3 + 1) = 20 second window. 5 * 3 = 15 seconds are > just waiting for restarts to happen. But the crashes can occur anywhere within > the remaining 5 seconds, not just one crash per 1-second window. I know it's not, but I can't for the life of me think how to phrase that! Is it ok to have a 5 line explanation for one static variable? Even now I don't think it's clear. Can you think of a snappy summary? https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:457: // an EXTENSION_UNLOADED notification. We post the crash handling code as On 2013/08/26 13:26:53, bartfab wrote: > The previous comment had more information on why popping up the balloon straight > away is not the right thing to do. Please re-add that part. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:467: // Restart the extension; notify user if crash recurs frequently. On 2013/08/26 13:26:53, bartfab wrote: > I think this code is complex enough to warrant putting it into a separate > method. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:468: const std::string& id = extension->id(); On 2013/08/26 13:26:53, bartfab wrote: > Nit: Why not call the variable extension_id? Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:474: if (!alreadyNotified && crashes.size() == (kNCrashesShouldNotify - 1) && On 2013/08/26 13:26:53, bartfab wrote: > Please put the calculation on its own line and explain what it does. It is too > complex to follow like this IMHO. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:489: base::MessageLoop::current()->PostDelayedTask(FROM_HERE, On 2013/08/26 13:26:53, bartfab wrote: > This task will run 3s later. What if |extension| or |profile| (both pointers) > becomes invalid before that, e.g. because the extension is uninstalled during > the 3s window? > > * For |profile|, you may be OK. You only use it in > ExtensionSystem::Get(profile). If that method can handle a nonexistent profile > (you should read its code to make sure this is the case), you are good. > I traced it through to BrowserContextKeyedBaseFactory::GetBrowserContextToUse(context), and there is an AssertBrowserContextWasntDestroyed(context) call, within a #ifndef NDEBUG, which calls NOTREACHED if it doesn't find a context .. so I think this might be an issue. Suggestions? > * For |extension|, accessing |extension->id()| in the delayed task is dangerous. > You could access extension->id() here now and pass the resulting |std::string| > to the delayed task. Make sure though that > extension_service()->ReloadExtension() can handle an extension id that may no > longer exist. > I'm not sure it does handle non-existent ones.. How do I check if it still exists? Then I could do that in my ReloadExtension method before calling extension_service()->ReloadExtension(). > * What will ReloadExtension() do if the extension has been uninstalled since - > will it reinstall it? That would be bad. err I'm trying to understand the code here, I'm not sure but I think there is a danger it might do just that - it checks 'if what we're loading was already installed', and if not does extensions::UnpackedInstaller::Create(this)->Load(path); <-- will this reinstall an extension? (Does 'path' mean anything for force-installed extensions? It gets cached on UnloadExtension, and doesn't get cleared on UninstallExtension, as far as I can tell, but maybe it only means something for local unpacked extensions?) https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:765: ScheduleCloseBalloons(UTF16ToASCII(details->application_id)); On 2013/08/26 13:26:53, bartfab wrote: > Will this not close the "misbehaving app" balloon when the app has reloaded? > IIUC, the balloon was meant to stay up, even if the app keeps crashing and > reloading. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:56: static void SetCrashDelaysForTesting(const base::TimeDelta& restart_delay, On 2013/08/26 13:26:53, bartfab wrote: > Nit 1: Add comment that explain what this does. > > Nit 2: The name may be a bit too generic. Since these delays apply to > force-installed extensions only, it would be best to mention that in the method > name. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:177: static base::TimeDelta restart_delay_; On 2013/08/26 13:26:53, bartfab wrote: > Static initializers are not allowed in Chrome. You can have static PODs but not > complex classes. You will have to make the statics PODs (say ints holding the > number of milliseconds or base::TimeDelta::ToInternalValue()s). Done. Made them ints, and only convert to TimeDelta at the last minute. Should I still pass the ints by reference (using int&) to the SetRestartDelays method? https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:200: // Map associating extension IDs with their most recent crash timestamps. On 2013/08/26 13:26:53, bartfab wrote: > Nit: Add a note that this is used for force-installed apps/extensions only. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:203: std::map<std::string, std::queue<base::TimeTicks> > extension_crashlog_map_; On 2013/08/26 13:26:53, bartfab wrote: > Nit 1: #include <queue> > Nit 2: #include "base/time.h" Done Nit 2. <queue> I'd already included on line 9. B-) https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:206: // 'Extension is misbehaving' notifications. On 2013/08/26 13:26:53, bartfab wrote: > Nit 1: Instead of "sent notifications," better say "shown ballons". > Nit 2: Add a note that this applies to force-installed apps/extensions only. Done. I've changed the wording of comments to say apps/extensions instead of extensions everywhere, but should I rename all these variables too? It's gonna get a bit verbose.. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:207: std::set<std::string> misbehaving_extensions_; On 2013/08/26 13:26:53, bartfab wrote: > Nit: #include <set> Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... File chrome/browser/policy/policy_browsertest.cc (left): https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:1395: On 2013/08/26 13:26:53, bartfab wrote: > Nit: Why remove the blank line? Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:458: // This class is a customised version of WindowedNotificationObserver to allow On 2013/08/26 13:26:53, bartfab wrote: > Nit: s/WindowedNotificationObserver/content::WindowedNotificationObserver/ Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:459: // us to register and wait until one of two simple events has occurred. See On 2013/08/26 13:26:53, bartfab wrote: > Nit: "simple events" is nomenclature I introduced in > WindowedNotificationObserver only. It is not what you would normally use in > Chrome. The comment should more be something along the lines of: > > This is a customized version of content::WindowedNotificationObserver that waits > until either of the two provided notification types is obseved. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:463: // Set up to wait for one of two conditions. The condition is met when a On 2013/08/26 13:26:53, bartfab wrote: > Nit: As above, "condition" is not the right term here. "Notification" is. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:467: On 2013/08/26 13:26:53, bartfab wrote: > Nit: Remove this blank line. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:470: // Wait until the specified condition is met. If the condition is already met On 2013/08/26 13:26:53, bartfab wrote: > Nit: As above, "notification" instead of "condition". Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:475: // NotificationObserver: On 2013/08/26 13:26:53, bartfab wrote: > Nit: s/NotificationObserver/content::NotificationObserver/ Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:502: return; On 2013/08/26 13:26:53, bartfab wrote: > Nit: Indent 2 spaces, not 4. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:1473: // Wait for all the extension render view hosts that exist to finish loading. On 2013/08/26 13:26:53, bartfab wrote: > Nit: Technically, this comment is correct. But it does not explain why you are > waiting for these RVHs to finish loading. Better use something like: > > // Wait until any background pages belonging to force-installed extensions have > been loaded. Done. https://codereview.chromium.org/23427003/diff/1/chrome/browser/policy/policy_... chrome/browser/policy/policy_browsertest.cc:1494: // Test policy-installed extensions are reloaded when killed. On 2013/08/26 13:26:53, bartfab wrote: > Nit: Add a blank line above. Done.
Hey Bartosz, Philip helped me get Chrome working again (there was just some file in the out/ directory I had to delete). And now the test passes again, wahey! :) https://codereview.chromium.org/23427003/diff/7001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/23427003/diff/7001/chrome/app/generated_resou... chrome/app/generated_resources.grd:275: Strings currently under review. https://codereview.chromium.org/23427003/diff/7001/chrome/browser/background/... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/7001/chrome/browser/background/... chrome/browser/background/background_contents_service.cc:310: Longer than 80 chars, will split.
The side-by-side incremental diff from PS1 now works, so I've deleted the old PS2. Hooray! https://codereview.chromium.org/23427003/diff/20001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/23427003/diff/20001/chrome/app/generated_reso... chrome/app/generated_resources.grd:275: + Managed app <ph name="APP_NAME">$1<ex>Background App</ex></ph> installed by enterprise policy is misbehaving. Please contact your administrator. strings under review
Just noticed a couple more comments that need amending, which I'll fix in the next patch set, plus a couple of minor questions. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:61: // Number of recent crashes required to trigger an extension-misbehave popup * app/extension - will fix https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:132: // Closing the 'Extension has crashed' balloon here should be OK, but it *Extension/App https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:536: const Extension* extension, Profile* profile) { Is this the right place to put the extracted method? https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.h:61: // crashes before showing a notification that the app is misbehaving. *'app/extension', not 'app'- will fix https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.h:195: // of crashes. This is the verbose description moved from the .cc file that I'm still not sure is clear.. suggestions?
https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:61: // Number of recent crashes required to trigger an extension-misbehave popup On 2013/08/27 09:05:36, anitawoodruff wrote: > * app/extension - will fix Done. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:132: // Closing the 'Extension has crashed' balloon here should be OK, but it On 2013/08/27 09:05:36, anitawoodruff wrote: > *Extension/App Done. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.h:61: // crashes before showing a notification that the app is misbehaving. On 2013/08/27 09:05:36, anitawoodruff wrote: > *'app/extension', not 'app'- will fix Done.
https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:61: // Number of recent crashes required to trigger an extension-misbehave popup On 2013/08/27 09:05:36, anitawoodruff wrote: > * app/extension - will fix Done. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:132: // Closing the 'Extension has crashed' balloon here should be OK, but it On 2013/08/27 09:05:36, anitawoodruff wrote: > *Extension/App Done. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.h:61: // crashes before showing a notification that the app is misbehaving. On 2013/08/27 09:05:36, anitawoodruff wrote: > *'app/extension', not 'app'- will fix Done.
https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:67: void ScheduleCloseBalloon(const std::string& id) { On 2013/08/26 21:45:23, anitawoodruff wrote: > On 2013/08/26 13:26:53, bartfab wrote: > > Nit 1: "extension_id" identifies an extension; easy. What does "id" identify? > > > Done - is prefixed_extension_id ok? Or should I make up a new concept such as > balloon_id ? > > > Nit 2: Add a comment that explains what these three methods do and how they > > differ. The differences are subtle and hard to spot. > > Done. Is this clear enough now? |prefixed_extension_id| is an accurate description of how this ID was generated but not what it is used for. |notification_id| or |balloon_id| would be much better. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:275: /** Seconds since a crash-reload during which we listen for further crashes **/ On 2013/08/26 21:45:23, anitawoodruff wrote: > On 2013/08/26 13:26:53, bartfab wrote: > > This comment is not quite correct. With the default settings, we wait for 5 > > crashes to happen in a 5 * (3 + 1) = 20 second window. 5 * 3 = 15 seconds are > > just waiting for restarts to happen. But the crashes can occur anywhere within > > the remaining 5 seconds, not just one crash per 1-second window. > > I know it's not, but I can't for the life of me think how to phrase that! Is it > ok to have a 5 line explanation for one static variable? Even now I don't think > it's clear. Can you think of a snappy summary? It is OK to have a long comment if a variable is unclear otherwise. In general, Chrome has few and terse comments but that does not mean you are not allowed to explain things well when needed. As with everything, it is a judgment call. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:489: base::MessageLoop::current()->PostDelayedTask(FROM_HERE, On 2013/08/26 21:45:23, anitawoodruff wrote: > On 2013/08/26 13:26:53, bartfab wrote: > > This task will run 3s later. What if |extension| or |profile| (both pointers) > > becomes invalid before that, e.g. because the extension is uninstalled during > > the 3s window? > > > > * For |profile|, you may be OK. You only use it in > > ExtensionSystem::Get(profile). If that method can handle a nonexistent profile > > (you should read its code to make sure this is the case), you are good. > > > I traced it through to > BrowserContextKeyedBaseFactory::GetBrowserContextToUse(context), and there is an > AssertBrowserContextWasntDestroyed(context) call, within a #ifndef NDEBUG, which > calls NOTREACHED if it doesn't find a context .. so I think this might be an > issue. Suggestions? > > > * For |extension|, accessing |extension->id()| in the delayed task is > dangerous. > > You could access extension->id() here now and pass the resulting |std::string| > > to the delayed task. Make sure though that > > extension_service()->ReloadExtension() can handle an extension id that may no > > longer exist. > > > I'm not sure it does handle non-existent ones.. How do I check if it still > exists? Then I could do that in my ReloadExtension method before calling > extension_service()->ReloadExtension(). > > > * What will ReloadExtension() do if the extension has been uninstalled since - > > will it reinstall it? That would be bad. > > err I'm trying to understand the code here, I'm not sure but I think there is a > danger it might do just that - it checks 'if what we're loading was already > installed', and if not does > extensions::UnpackedInstaller::Create(this)->Load(path); <-- will this > reinstall an extension? (Does 'path' mean anything for force-installed > extensions? It gets cached on UnloadExtension, and doesn't get cleared on > UninstallExtension, as far as I can tell, but maybe it only means something for > local unpacked extensions?) Sounds like both issues may arise - we may end up installing a deinstalled extension and we may end up accessing an invalid profile. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:177: static base::TimeDelta restart_delay_; On 2013/08/26 21:45:23, anitawoodruff wrote: > On 2013/08/26 13:26:53, bartfab wrote: > > Static initializers are not allowed in Chrome. You can have static PODs but > not > > complex classes. You will have to make the statics PODs (say ints holding the > > number of milliseconds or base::TimeDelta::ToInternalValue()s). > > Done. Made them ints, and only convert to TimeDelta at the last minute. Should I > still pass the ints by reference (using int&) to the SetRestartDelays method? > For PODs, passing by reference is not necessary. https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.h:206: // 'Extension is misbehaving' notifications. On 2013/08/26 21:45:23, anitawoodruff wrote: > On 2013/08/26 13:26:53, bartfab wrote: > > Nit 1: Instead of "sent notifications," better say "shown ballons". > > Nit 2: Add a note that this applies to force-installed apps/extensions only. > > Done. > I've changed the wording of comments to say apps/extensions instead of > extensions everywhere, but should I rename all these variables too? It's gonna > get a bit verbose.. The variable naming is OK. Comments are for verbose stuff. Variable names should be as-crisp-as-possible. I think you have the right balance here now. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:63: const unsigned int kForceInstalledExtensionCrashNotificationThreshold = 5; I know I have been asking for more verbosity but feel free to use your own judgment and/or push back when you think naming gets too long and confusing. In this case, I think something shorter would be ok, say kMisbehaveCrashCount. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:70: // Closes the balloon created with the specified id, which should be of the form If you rename |prefixed_extension_id|, this comment can be simplified. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:229: void ShowBalloon(const Extension* extension, Profile* profile, bool isForced) { Nit: You used |forceInstalled| in a similar context further down. I think this would be a better name here as well. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:286: int BackgroundContentsService::restart_delay_millis_ = 3000; Nit: It is customary to follow this with " // 3 seconds." (prefixed by two spaces). https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:287: int BackgroundContentsService::crash_window_millis_ = 1000; Nit: It is customary to follow this with " // 1 second." (prefixed by two spaces). https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:473: bool forceInstalled = Nit: const https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:546: const bool shouldNotify = How about this kind of structure instead: if (!alreadyNotified) { const bool notify = crashes.size() == kForceInstalledExtensionCrashNotificationThreshold - 1 && base::TimeTicks::Now() - crashes.front() < duration; if (notify) { ... } else { ... } } That way, it is clear that all of the processing is being skipped if the notification was shown already. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.h:63: const int restart_delay_millis, const int crash_window_millis); Nit: We tend to use "_in_ms" instead of "_millis". https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.h:191: // When a force-installed extension/app crashes, we check if it's in a crash/ Nit: Some comments say "app/extension," others say "extension/app" (in this file and others).
https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/1/chrome/browser/background/bac... chrome/browser/background/background_contents_service.cc:67: void ScheduleCloseBalloon(const std::string& id) { On 2013/08/27 09:21:29, bartfab wrote: > On 2013/08/26 21:45:23, anitawoodruff wrote: > > On 2013/08/26 13:26:53, bartfab wrote: > > > Nit 1: "extension_id" identifies an extension; easy. What does "id" > identify? > > > > > Done - is prefixed_extension_id ok? Or should I make up a new concept such as > > balloon_id ? > > > > > Nit 2: Add a comment that explains what these three methods do and how they > > > differ. The differences are subtle and hard to spot. > > > > Done. Is this clear enough now? > > |prefixed_extension_id| is an accurate description of how this ID was generated > but not what it is used for. |notification_id| or |balloon_id| would be much > better. Done.
There are still a few comments in patch set 2 that have not been addressed. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.h:195: // of crashes. On 2013/08/27 09:05:36, anitawoodruff wrote: > This is the verbose description moved from the .cc file that I'm still not sure > is clear.. suggestions? I think it is clear.
Sorry; I missed these comments before. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:63: const unsigned int kForceInstalledExtensionCrashNotificationThreshold = 5; On 2013/08/27 09:21:29, bartfab wrote: > I know I have been asking for more verbosity but feel free to use your own > judgment and/or push back when you think naming gets too long and confusing. In > this case, I think something shorter would be ok, say kMisbehaveCrashCount. Done - renamed to kMisbehaveCrashCountThreshold. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:70: // Closes the balloon created with the specified id, which should be of the form On 2013/08/27 09:21:29, bartfab wrote: > If you rename |prefixed_extension_id|, this comment can be simplified. Done. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:229: void ShowBalloon(const Extension* extension, Profile* profile, bool isForced) { On 2013/08/27 09:21:29, bartfab wrote: > Nit: You used |forceInstalled| in a similar context further down. I think this > would be a better name here as well. Done - just realised I shouldn't be using camelCase though, so it should be |force_installed| in both places, right? and 'const' I guess? https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:286: int BackgroundContentsService::restart_delay_millis_ = 3000; On 2013/08/27 09:21:29, bartfab wrote: > Nit: It is customary to follow this with " // 3 seconds." (prefixed by two > spaces). Done. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:287: int BackgroundContentsService::crash_window_millis_ = 1000; On 2013/08/27 09:21:29, bartfab wrote: > Nit: It is customary to follow this with " // 1 second." (prefixed by two > spaces). Done. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:473: bool forceInstalled = On 2013/08/27 09:21:29, bartfab wrote: > Nit: const Done. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:546: const bool shouldNotify = On 2013/08/27 09:21:29, bartfab wrote: > How about this kind of structure instead: > > if (!alreadyNotified) { > const bool notify = crashes.size() == > kForceInstalledExtensionCrashNotificationThreshold - 1 && > base::TimeTicks::Now() - crashes.front() < duration; > if (notify) { > ... > } else { > ... > } > } > > That way, it is clear that all of the processing is being skipped if the > notification was shown already. Done. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.h:63: const int restart_delay_millis, const int crash_window_millis); On 2013/08/27 09:21:29, bartfab wrote: > Nit: We tend to use "_in_ms" instead of "_millis". Done. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.h:191: // When a force-installed extension/app crashes, we check if it's in a crash/ On 2013/08/27 09:21:29, bartfab wrote: > Nit: Some comments say "app/extension," others say "extension/app" (in this file > and others). Done.
https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:229: void ShowBalloon(const Extension* extension, Profile* profile, bool isForced) { On 2013/08/27 11:57:39, anitawoodruff wrote: > On 2013/08/27 09:21:29, bartfab wrote: > > Nit: You used |forceInstalled| in a similar context further down. I think this > > would be a better name here as well. > > Done - just realised I shouldn't be using camelCase though, so it should be > |force_installed| in both places, right? and 'const' I guess? Aye. Something about that variable name looked fishy but I could not quite figure it out. Camel caps, duh. Yes, you should not be using camel caps in members and arguments. We tend not to use const for arguments. It would actually be useful (preventing accidental assignment to the argument) but since we do not do it anywhere else in the code, I would leave out the const here too. https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:536: const Extension* extension, Profile* profile) { On 2013/08/27 09:05:36, anitawoodruff wrote: > Is this the right place to put the extracted method? It will work here and you can leave it here. If you wanted to make your code more palatable to the reader, you could put it *before* the method that calls it - this is the most natural order. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (left): https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:352: Nit: Any reason to remove this line? https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:422: // Remove any "This extension has crashed" balloons. Nit: Update comment. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:61: // Number of recent crashes of a force-installed extensions/app that will Nit: s:extension/app:app/extension: https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:157: // Empty delegate for the 'app/extension is misbehaving' popup balloon. which is Nit: s/./,/ https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:278: int BackgroundContentsService::restart_delay_millis_ = 3000; // 3 seconds. Nit: s/millis/in_ms/ https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:279: int BackgroundContentsService::crash_window_millis_ = 1000; // 1 second. Nit: s/millis/in_ms/ https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:464: // code as a task here so that it is not executed before this event. Actually, this comment applies to the !force_installed case only. Could you move it inside the if-block? https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:526: void BackgroundContentsService::RestartForceInstalledExtensionOnCrash( The method order in the .cc file does not seem to match that in the .h file. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:529: const bool alreadyNotified = misbehaving_extensions_.find(extension_id) != s/alreadyNotified/already_notified/ https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:532: const base::TimeDelta duration = Nit: |duration| is fairly generic. How about |crash_loop_window|? Or maybe I a just being too verbose. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:538: const bool shouldNotify = s/shouldNotify/should_notify/ https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:549: crashes.pop(); // Remove old timestamps. Nit: Put the comment above the source line. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:570: // Force-installed app/extension has been removed by policy. Maybe better: Policy has changed. The app/extension is no longer force-installed. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:63: const int restart_delay_in_ms, const int crash_window_in_ms); Nit: The const is very much correct here but not customary for PODs in Chrome code. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:186: const extensions::Extension* extension, Profile* profile); Here, the const definitely should stay as we are dealing with a pointer, not a POD. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:189: void ReloadExtension(const std::string extension_id, Profile* profile); Nit: Pass the string by const reference. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:192: static int restart_delay_millis_; Nit: s/millis/in_ms/ https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:197: // |crash_window_millis_| by the threshold crash count and adding on the Nit 1: s/millis/in_ms/ Nit 2: The prose is fine. If you prefer, you could quote the formula instead. Whatever you find more legible: |kMisbehaveCrashCountThreshold * (restart_delay_in_ms + crash_window_in_ms)| https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:199: static int crash_window_millis_; Nit: s/millis/in_ms/ https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:231: base::WeakPtrFactory<BackgroundContentsService> weak_factory_; Nit: #include "base/memory/weak_ptr.h"
https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:229: void ShowBalloon(const Extension* extension, Profile* profile, bool isForced) { On 2013/08/27 15:37:44, bartfab wrote: > On 2013/08/27 11:57:39, anitawoodruff wrote: > > On 2013/08/27 09:21:29, bartfab wrote: > > > Nit: You used |forceInstalled| in a similar context further down. I think > this > > > would be a better name here as well. > > > > Done - just realised I shouldn't be using camelCase though, so it should be > > |force_installed| in both places, right? and 'const' I guess? > > Aye. Something about that variable name looked fishy but I could not quite > figure it out. Camel caps, duh. Yes, you should not be using camel caps in > members and arguments. > > We tend not to use const for arguments. It would actually be useful (preventing > accidental assignment to the argument) but since we do not do it anywhere else > in the code, I would leave out the const here too. Done. So don't use 'const' for args which aren't pointers/refs, but do use 'const' when declaring a local POD within a function that doesn't change? (e.g. const bool already_notified = ...) or is it unnecessary there too, if I only use it on the next line? https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:536: const Extension* extension, Profile* profile) { On 2013/08/27 15:37:44, bartfab wrote: > On 2013/08/27 09:05:36, anitawoodruff wrote: > > Is this the right place to put the extracted method? > > It will work here and you can leave it here. If you wanted to make your code > more palatable to the reader, you could put it *before* the method that calls it > - this is the most natural order. But, Observe (which calls this) is the first method in the class, so all the (class) methods it calls are defined below it - it wouldn't be consistent if I moved just these 2 above it without reordering the other methods. Since it doesn't seem to have much order already I shall leave as is. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (left): https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:352: On 2013/08/27 15:37:44, bartfab wrote: > Nit: Any reason to remove this line? Replaced. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:422: // Remove any "This extension has crashed" balloons. On 2013/08/27 15:37:44, bartfab wrote: > Nit: Update comment. Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:61: // Number of recent crashes of a force-installed extensions/app that will On 2013/08/27 15:37:44, bartfab wrote: > Nit: s:extension/app:app/extension: Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:157: // Empty delegate for the 'app/extension is misbehaving' popup balloon. which is On 2013/08/27 15:37:44, bartfab wrote: > Nit: s/./,/ Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:278: int BackgroundContentsService::restart_delay_millis_ = 3000; // 3 seconds. On 2013/08/27 15:37:44, bartfab wrote: > Nit: s/millis/in_ms/ Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:279: int BackgroundContentsService::crash_window_millis_ = 1000; // 1 second. On 2013/08/27 15:37:44, bartfab wrote: > Nit: s/millis/in_ms/ Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:464: // code as a task here so that it is not executed before this event. On 2013/08/27 15:37:44, bartfab wrote: > Actually, this comment applies to the !force_installed case only. Could you move > it inside the if-block? Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:526: void BackgroundContentsService::RestartForceInstalledExtensionOnCrash( On 2013/08/27 15:37:44, bartfab wrote: > The method order in the .cc file does not seem to match that in the .h file. Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:529: const bool alreadyNotified = misbehaving_extensions_.find(extension_id) != On 2013/08/27 15:37:44, bartfab wrote: > s/alreadyNotified/already_notified/ Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:532: const base::TimeDelta duration = On 2013/08/27 15:37:44, bartfab wrote: > Nit: |duration| is fairly generic. How about |crash_loop_window|? Or maybe I a > just being too verbose. Changed to 'recent_time_window'. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:538: const bool shouldNotify = On 2013/08/27 15:37:44, bartfab wrote: > s/shouldNotify/should_notify/ Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:549: crashes.pop(); // Remove old timestamps. On 2013/08/27 15:37:44, bartfab wrote: > Nit: Put the comment above the source line. Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:570: // Force-installed app/extension has been removed by policy. On 2013/08/27 15:37:44, bartfab wrote: > Maybe better: > > Policy has changed. The app/extension is no longer force-installed. Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:63: const int restart_delay_in_ms, const int crash_window_in_ms); On 2013/08/27 15:37:44, bartfab wrote: > Nit: The const is very much correct here but not customary for PODs in Chrome > code. Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:189: void ReloadExtension(const std::string extension_id, Profile* profile); On 2013/08/27 15:37:44, bartfab wrote: > Nit: Pass the string by const reference. Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:192: static int restart_delay_millis_; On 2013/08/27 15:37:44, bartfab wrote: > Nit: s/millis/in_ms/ Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:197: // |crash_window_millis_| by the threshold crash count and adding on the On 2013/08/27 15:37:44, bartfab wrote: > Nit 1: s/millis/in_ms/ > > Nit 2: The prose is fine. If you prefer, you could quote the formula instead. > Whatever you find more legible: > > |kMisbehaveCrashCountThreshold * (restart_delay_in_ms + crash_window_in_ms)| Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:199: static int crash_window_millis_; On 2013/08/27 15:37:44, bartfab wrote: > Nit: s/millis/in_ms/ Done. https://codereview.chromium.org/23427003/diff/45001/chrome/browser/background... chrome/browser/background/background_contents_service.h:231: base::WeakPtrFactory<BackgroundContentsService> weak_factory_; On 2013/08/27 15:37:44, bartfab wrote: > Nit: #include "base/memory/weak_ptr.h" Done.
Adding Antony & Drew as reviewers. Drew: please review chrome/browser/background/background_contents_service.cc https://codereview.chromium.org/23427003/patch/10001/56002 chrome/browser/background/background_contents_service.h https://codereview.chromium.org/23427003/patch/10001/56003 chrome/browser/policy/policy_browsertest.cc https://codereview.chromium.org/23427003/patch/10001/56004 Antony: Hi! I'm adding you because you are an owner of the chrome/browser/extensions folder, where I have modified one of the extensions we use for testing: chrome/test/data/extensions/good2.crx All we did was give it a static html background page, so that it starts a background process we can crash, in order to test the extension gets reloaded. Thanks both, look forward to hearing from you. Anita ps. Just a heads up, Friday is the last day of my internship. Hoping to get this landed before then, but if there are any outstanding issues, Bartosz will take over.
lgtm https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/20001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:229: void ShowBalloon(const Extension* extension, Profile* profile, bool isForced) { On 2013/08/27 17:34:29, anitawoodruff wrote: > On 2013/08/27 15:37:44, bartfab wrote: > > On 2013/08/27 11:57:39, anitawoodruff wrote: > > > On 2013/08/27 09:21:29, bartfab wrote: > > > > Nit: You used |forceInstalled| in a similar context further down. I think > > this > > > > would be a better name here as well. > > > > > > Done - just realised I shouldn't be using camelCase though, so it should be > > > |force_installed| in both places, right? and 'const' I guess? > > > > Aye. Something about that variable name looked fishy but I could not quite > > figure it out. Camel caps, duh. Yes, you should not be using camel caps in > > members and arguments. > > > > We tend not to use const for arguments. It would actually be useful > (preventing > > accidental assignment to the argument) but since we do not do it anywhere else > > in the code, I would leave out the const here too. > > Done. So don't use 'const' for args which aren't pointers/refs, but do use > 'const' when declaring a local POD within a function that doesn't change? (e.g. > const bool already_notified = ...) or is it unnecessary there too, if I only use > it on the next line? Our usage is not entirely consistent but your summary matches what most people do: no const for POD arguments; const for all constant local variables, POD or not. https://codereview.chromium.org/23427003/diff/10001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/10001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:514: // Remove any "app/extension has crashed" balloons. Nit: This also removes any "app/extension is misbehaving" balloons. A comment that covers both would be better, e.g.: // Remove any balloons shown for this app/extension. https://codereview.chromium.org/23427003/diff/10001/chrome/browser/background... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/10001/chrome/browser/background... chrome/browser/background/background_contents_service.h:197: // given time window. The duration of that window is given by Nit: Add a colon at the end of the sentence.
https://codereview.chromium.org/23427003/diff/10001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/10001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:514: // Remove any "app/extension has crashed" balloons. On 2013/08/27 19:21:31, bartfab wrote: > Nit: This also removes any "app/extension is misbehaving" balloons. A comment > that covers both would be better, e.g.: > > // Remove any balloons shown for this app/extension. Done. https://codereview.chromium.org/23427003/diff/10001/chrome/browser/background... File chrome/browser/background/background_contents_service.h (right): https://codereview.chromium.org/23427003/diff/10001/chrome/browser/background... chrome/browser/background/background_contents_service.h:197: // given time window. The duration of that window is given by On 2013/08/27 19:21:31, bartfab wrote: > Nit: Add a colon at the end of the sentence. Done.
Mostly looks good, with just that one comment about getting rid of WeakPtrFactory. We have a test to test that force-installed extensions are force-reloaded, but do we have a test to make sure we notify the user about misbehaving extensions? That could be in a separate CL. https://codereview.chromium.org/23427003/diff/55001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/55001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:560: weak_factory_.GetWeakPtr(), extension_id, profile), Does ReloadExtension() actually need to be a member function (does it actually access any members of BackgroundContentsService, or does it just use the passed Profile/extension_id? Because if we don't need it to be a member function, then we can get rid of the WeakPtrFactory.
https://codereview.chromium.org/23427003/diff/55001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/55001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:560: weak_factory_.GetWeakPtr(), extension_id, profile), On 2013/08/28 17:37:35, Andrew T Wilson wrote: > Does ReloadExtension() actually need to be a member function (does it actually > access any members of BackgroundContentsService, or does it just use the passed > Profile/extension_id? > > Because if we don't need it to be a member function, then we can get rid of the > WeakPtrFactory. If ReloadExtension() is not a member function, it may run at a point where |profile| is no longer valid. If it is a member of |this|, |profile| is guaranteed to still be valid when it runs because the profile is destroyed before |this|.
On 2013/08/28 17:37:34, Andrew T Wilson wrote: > We have a test to test that force-installed extensions are force-reloaded, but > do we have a test to make sure we notify the user about misbehaving extensions? > That could be in a separate CL. > Hi Drew, thanks for the review. I've spoken to Bartosz about this and he thinks this would be difficult to test, because we have no easy way to hook in to test the UI for popup notifications across different systems. There might be an argument for unit testing the logic of the decision of whether to show one. However, the built-in delays and wait times complicate things -for testing we would like to set the times down to 0, but then we'd effectively just be checking if we show a popup when our count exceeds 5.. not so useful. Anita
On 2013/08/29 10:16:02, anitawoodruff wrote: > On 2013/08/28 17:37:34, Andrew T Wilson wrote: > > We have a test to test that force-installed extensions are force-reloaded, but > > do we have a test to make sure we notify the user about misbehaving > extensions? > > That could be in a separate CL. > > > > Hi Drew, thanks for the review. I've spoken to Bartosz about this and he thinks > this would be difficult to test, because we have no easy way to hook in to test > the UI for popup notifications across different systems. > > There might be an argument for unit testing the logic of the decision of whether > to show one. However, the built-in delays and wait times complicate things -for > testing we would like to set the times down to 0, but then we'd effectively just > be checking if we show a popup when our count exceeds 5.. not so useful. > > Anita My concern is that there is non-trivial code in there managing the queue of "recent items" with absolutely no test coverage. If it were just a simple count (i.e. always display a dialog after 5 crashes) then I might be OK with no test coverage, but you've built something more complex with a sliding window. Agreed that you can't hook in to test the UI popup, but some unit-level tests would be useful here.
On 2013/08/28 17:43:00, bartfab wrote: > https://codereview.chromium.org/23427003/diff/55001/chrome/browser/background... > File chrome/browser/background/background_contents_service.cc (right): > > https://codereview.chromium.org/23427003/diff/55001/chrome/browser/background... > chrome/browser/background/background_contents_service.cc:560: > weak_factory_.GetWeakPtr(), extension_id, profile), > On 2013/08/28 17:37:35, Andrew T Wilson wrote: > > Does ReloadExtension() actually need to be a member function (does it actually > > access any members of BackgroundContentsService, or does it just use the > passed > > Profile/extension_id? > > > > Because if we don't need it to be a member function, then we can get rid of > the > > WeakPtrFactory. > > If ReloadExtension() is not a member function, it may run at a point where > |profile| is no longer valid. If it is a member of |this|, |profile| is > guaranteed to still be valid when it runs because the profile is destroyed > before |this|. Hmm, that's kind of an indirect reason to base this functionality within this object. I'm OK with this, but you should document that this is why you're doing it as part of this class.
+ Kalman for review as another owner of the chrome/test/data/extensions/ directory, since no word from Antony thus far. Hi Kalman - adding you because I have modified one of the extensions we use for testing: chrome/test/data/extensions/good2.crx All we did was give it a static html background page, so that it starts a background process we can crash, in order to test the extension gets reloaded. It's my last day tomorrow (my internship is ending) so a fast review would be appreciated. It would be nice to get this landed before I leave :) However, if there are any outstanding problems then Bartosz will take this over in my absence. Cheers Anita
https://codereview.chromium.org/23427003/diff/55001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/55001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:560: weak_factory_.GetWeakPtr(), extension_id, profile), On 2013/08/28 17:37:35, Andrew T Wilson wrote: > Does ReloadExtension() actually need to be a member function (does it actually > access any members of BackgroundContentsService, or does it just use the passed > Profile/extension_id? > > Because if we don't need it to be a member function, then we can get rid of the > WeakPtrFactory. On Bartosz' suggestion, I have now avoided using a weak pointer altogether by instead using a profile manager utility method to check if the profile is still valid in the ReloadExtension method. (See PS8).
still lgtm https://codereview.chromium.org/23427003/diff/73001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/73001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:265: !g_browser_process->profile_manager()->IsValidProfile(profile)) Nit 1: un-indent by two spaces. Nit 2: Curly braces. https://codereview.chromium.org/23427003/diff/73001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:273: // Policy has changed. The app/extension is no longer force-installed. Nit: This will return |false| if the app/extension was uninstalled or has since been restarted successfully by someone else (the user). Could you update the comment?
lgtm, though we removed that owners file now.
Drew, I take your point about unit tests being a good idea. I spent some time with Bartosz today figuring out how to do this with an injected tick clock, and made significant progress. Hopefully I'll be in a position to put up an initial CL tomorrow. However I am not sure it will be ready to land by the end of tomorrow, so I will do this in a separate CL, so that Bartosz can take over if necessary. I'll leave it up to you if you want to wait until that CL is complete before LGTM-ing this one. Anita https://codereview.chromium.org/23427003/diff/73001/chrome/browser/background... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/23427003/diff/73001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:265: !g_browser_process->profile_manager()->IsValidProfile(profile)) On 2013/08/29 12:06:24, bartfab wrote: > Nit 1: un-indent by two spaces. > Nit 2: Curly braces. Done. https://codereview.chromium.org/23427003/diff/73001/chrome/browser/background... chrome/browser/background/background_contents_service.cc:273: // Policy has changed. The app/extension is no longer force-installed. On 2013/08/29 12:06:24, bartfab wrote: > Nit: This will return |false| if the app/extension was uninstalled or has since > been restarted successfully by someone else (the user). Could you update the > comment? Done.
lgtm
I am dcommitting the extension update separately because the CQ cannot handle binary files. See CL 23482005.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anitawoodruff@chromium.org/23427003/89001
Message was sent while issue was closed.
Change committed as 220586 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
