Reload force-installed extensions on crash/force-close

chrome/browser/background/background_contents_service.cc

Index: chrome/browser/background/background_contents_service.cc
diff --git a/chrome/browser/background/background_contents_service.cc b/chrome/browser/background/background_contents_service.cc
index 1e81d648bb7e383048e299743f62cf688b09ae99..b3b24da95af3f9e703885e37bc6e3cecb6aed9b2 100644
--- a/chrome/browser/background/background_contents_service.cc
+++ b/chrome/browser/background/background_contents_service.cc
@@ -22,6 +22,7 @@
 #include "chrome/browser/extensions/image_loader.h"
 #include "chrome/browser/notifications/desktop_notification_service.h"
 #include "chrome/browser/notifications/notification.h"
+#include "chrome/browser/notifications/notification_delegate.h"
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/profiles/profile.h"
@@ -54,19 +55,40 @@ using extensions::UnloadedExtensionInfo;
 
 namespace {
 
-const char kNotificationPrefix[] = "app.background.crashed.";
+const char kCrashNotificationPrefix[] = "app.background.crashed.";
+const char kMisbehaveNotificationPrefix[] = "app.background.misbehaved.";
 
-void CloseBalloon(const std::string& id) {
-  g_browser_process->notification_ui_manager()->CancelById(id);
+// Number of recent crashes required to trigger an extension-misbehave popup

[anitawoodruff, 2013/08/27 09:05:36]

* app/extension - will fix

[anitawoodruff, 2013/08/27 09:13:03]

Done.

+// for force-installed extensions/apps.
+const unsigned int kForceInstalledExtensionCrashNotificationThreshold = 5;

[bartfab (slow), 2013/08/27 09:21:29]

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.

[anitawoodruff, 2013/08/27 11:57:39]

Done - renamed to kMisbehaveCrashCountThreshold.

+
+void CloseBalloon(const std::string& prefixed_extension_id) {
+  g_browser_process->notification_ui_manager()->
+      CancelById(prefixed_extension_id);
 }
 
-void ScheduleCloseBalloon(const std::string& extension_id) {
+// Closes the balloon created with the specified id, which should be of the form

[bartfab (slow), 2013/08/27 09:21:29]

If you rename |prefixed_extension_id|, this comment can be simplified.

[anitawoodruff, 2013/08/27 11:57:39]

Done.

+// notification prefix + extension/app-id.
+void ScheduleCloseBalloon(const std::string& prefixed_extension_id) {
   if (!base::MessageLoop::current())  // For unit_tests
     return;
   base::MessageLoop::current()->PostTask(
-      FROM_HERE, base::Bind(&CloseBalloon, kNotificationPrefix + extension_id));
+      FROM_HERE, base::Bind(&CloseBalloon, prefixed_extension_id));
+}
+
+// Closes the crash notification balloon for the app/extension with this id.
+void ScheduleCloseCrashBalloon(const std::string& extension_id) {
+  ScheduleCloseBalloon(kCrashNotificationPrefix + extension_id);
 }
 
+// Closes all notification balloons relating to the app/extension with this id.
+void ScheduleCloseBalloons(const std::string& extension_id) {
+  ScheduleCloseBalloon(kMisbehaveNotificationPrefix + extension_id);
+  ScheduleCloseBalloon(kCrashNotificationPrefix + extension_id);
+}
+
+// Delegate for the 'Extension/app has crashed' popup balloon. Restarts the
+// extension/app when the balloon is clicked.
 class CrashNotificationDelegate : public NotificationDelegate {
  public:
   CrashNotificationDelegate(Profile* profile,
@@ -107,15 +129,15 @@ class CrashNotificationDelegate : public NotificationDelegate {
           ReloadExtension(copied_extension_id);
     }
 
-    // Closing the balloon here should be OK, but it causes a crash on Mac
-    // http://crbug.com/78167
-    ScheduleCloseBalloon(copied_extension_id);
+    // Closing the 'Extension has crashed' balloon here should be OK, but it

[anitawoodruff, 2013/08/27 09:05:36]

*Extension/App

[anitawoodruff, 2013/08/27 09:13:03]

Done.

+    // causes a crash on Mac (http://crbug.com/78167).
+    ScheduleCloseCrashBalloon(copied_extension_id);
   }
 
   virtual bool HasClickedListener() OVERRIDE { return true; }
 
   virtual std::string id() const OVERRIDE {
-    return kNotificationPrefix + extension_id_;
+    return kCrashNotificationPrefix + extension_id_;
   }
 
   virtual content::RenderViewHost* GetRenderViewHost() const OVERRIDE {
@@ -133,12 +155,48 @@ class CrashNotificationDelegate : public NotificationDelegate {
   DISALLOW_COPY_AND_ASSIGN(CrashNotificationDelegate);
 };
 
+// Empty delegate for the 'Extension/app is misbehaving' popup balloon. which is
+// triggered if a force-installed app/extension gets stuck in a crash/reload
+// cycle. Doesn't do anything on click because force-installed apps/extensions
+// get restarted automatically.
+class MisbehaveNotificationDelegate : public NotificationDelegate {
+ public:
+  explicit MisbehaveNotificationDelegate(const Extension* extension)
+      : extension_id_(extension->id()) {
+  }
+
+  virtual void Display() OVERRIDE {}
+
+  virtual void Error() OVERRIDE {}
+
+  virtual void Close(bool by_user) OVERRIDE {}
+
+  virtual void Click() OVERRIDE {}
+
+  virtual bool HasClickedListener() OVERRIDE { return true; }
+
+  virtual std::string id() const OVERRIDE {
+    return kMisbehaveNotificationPrefix + extension_id_;
+  }
+
+  virtual content::RenderViewHost* GetRenderViewHost() const OVERRIDE {
+    return NULL;
+  }
+
+ private:
+  virtual ~MisbehaveNotificationDelegate() {}
+
+  std::string extension_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(MisbehaveNotificationDelegate);
+};
+
 #if defined(ENABLE_NOTIFICATIONS)
 void NotificationImageReady(
     const std::string extension_name,
     const string16 message,
     const GURL extension_url,
-    scoped_refptr<CrashNotificationDelegate> delegate,
+    scoped_refptr<NotificationDelegate> delegate,
     Profile* profile,
     const gfx::Image& icon) {
   gfx::Image notification_icon(icon);
@@ -157,19 +215,39 @@ void NotificationImageReady(
 }
 #endif
 
-void ShowBalloon(const Extension* extension, Profile* profile) {
-#if defined(ENABLE_NOTIFICATIONS)
-  string16 message = l10n_util::GetStringFUTF16(
-      extension->is_app() ? IDS_BACKGROUND_CRASHED_APP_BALLOON_MESSAGE :
-                            IDS_BACKGROUND_CRASHED_EXTENSION_BALLOON_MESSAGE,
-      UTF8ToUTF16(extension->name()));
+void ReloadExtension(const std::string extension_id, Profile* profile) {
+  extensions::ExtensionSystem* extensionSystem =
+      extensions::ExtensionSystem::Get(profile);
+  if (extensionSystem && extensionSystem->extension_service())
+    extensionSystem->extension_service()->ReloadExtension(extension_id);
+}
 
+// Show a popup notification balloon with a crash message for a given app/
+// extension. |isForced| is true if the app/extension was force-installed, in
+// which case we show an 'App/extension misbehaving' message instead of a
+// crash message (since it will get restarted automatically).
+void ShowBalloon(const Extension* extension, Profile* profile, bool isForced) {

[bartfab (slow), 2013/08/27 09:21:29]

Nit: You used |forceInstalled| in a similar context further down. I think this
would be a better name here as well.

[anitawoodruff, 2013/08/27 11:57:39]

Done - just realised I shouldn't be using camelCase though, so it should be
|force_installed| in both places, right? and 'const' I guess?

[bartfab (slow), 2013/08/27 15:37:44]

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.

[anitawoodruff, 2013/08/27 17:34:29]

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?

[bartfab (slow), 2013/08/27 19:21:30]

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.

+#if defined(ENABLE_NOTIFICATIONS)
+  string16 message;
+  scoped_refptr<NotificationDelegate> delegate;
+  if (isForced) {
+    message = l10n_util::GetStringFUTF16(
+        extension->is_app() ?
+            IDS_BACKGROUND_MISBEHAVING_APP_BALLOON_MESSAGE :
+            IDS_BACKGROUND_MISBEHAVING_EXTENSION_BALLOON_MESSAGE,
+        UTF8ToUTF16(extension->name()));
+    delegate = new MisbehaveNotificationDelegate(extension);
+  } else {
+    message = l10n_util::GetStringFUTF16(
+        extension->is_app() ? IDS_BACKGROUND_CRASHED_APP_BALLOON_MESSAGE :
+                              IDS_BACKGROUND_CRASHED_EXTENSION_BALLOON_MESSAGE,
+        UTF8ToUTF16(extension->name()));
+    delegate = new CrashNotificationDelegate(profile, extension);
+  }
   extension_misc::ExtensionIcons size(extension_misc::EXTENSION_ICON_MEDIUM);
   extensions::ExtensionResource resource =
       extensions::IconsInfo::GetIconResource(
           extension, size, ExtensionIconSet::MATCH_SMALLER);
-  scoped_refptr<CrashNotificationDelegate> delegate =
-    new CrashNotificationDelegate(profile, extension);
   // We can't just load the image in the Observe method below because, despite
   // what this method is called, it may call the callback synchronously.
   // However, it's possible that the extension went away during the interim,
@@ -205,6 +283,9 @@ void ShowBalloon(const Extension* extension, Profile* profile) {
 const char kUrlKey[] = "url";
 const char kFrameNameKey[] = "name";
 
+int BackgroundContentsService::restart_delay_millis_ = 3000;

[bartfab (slow), 2013/08/27 09:21:29]

Nit: It is customary to follow this with "  // 3 seconds." (prefixed by two
spaces).

[anitawoodruff, 2013/08/27 11:57:39]

Done.

+int BackgroundContentsService::crash_window_millis_ = 1000;

[bartfab (slow), 2013/08/27 09:21:29]

Nit: It is customary to follow this with "  // 1 second." (prefixed by two
spaces).

[anitawoodruff, 2013/08/27 11:57:39]

Done.

+
 BackgroundContentsService::BackgroundContentsService(
     Profile* profile, const CommandLine* command_line)
     : prefs_(NULL) {
@@ -225,6 +306,15 @@ BackgroundContentsService::~BackgroundContentsService() {
   DCHECK(contents_map_.empty());
 }
 
+// static
+void BackgroundContentsService::
+    SetCrashDelaysForForceInstalledAppsAndExtensionsForTesting(
+        const int restart_delay_millis,
+        const int crash_window_millis) {
+  restart_delay_millis_ = restart_delay_millis;
+  crash_window_millis_ = crash_window_millis;
+}
+
 std::vector<BackgroundContents*>
 BackgroundContentsService::GetBackgroundContents() const
 {
@@ -349,9 +439,8 @@ void BackgroundContentsService::Observe(
                                  UTF8ToUTF16(extension->id()));
         }
       }
-
       // Remove any "This extension has crashed" balloons.
-      ScheduleCloseBalloon(extension->id());
+      ScheduleCloseCrashBalloon(extension->id());
       SendChangeNotification(profile);
       break;
     }
@@ -379,10 +468,19 @@ void BackgroundContentsService::Observe(
       // When an extension crashes, EXTENSION_PROCESS_TERMINATED is followed by
       // an EXTENSION_UNLOADED notification. This UNLOADED signal causes all the
       // notifications for this extension to be cancelled by
-      // DesktopNotificationService. For this reason, instead of showing the
-      // balloon right now, we schedule it to show a little later.
-      base::MessageLoop::current()->PostTask(
-          FROM_HERE, base::Bind(&ShowBalloon, extension, profile));
+      // DesktopNotificationService. For this reason, we post the crash handling
+      // code as a task here so that it is not executed before this event.
+      bool forceInstalled =

[bartfab (slow), 2013/08/27 09:21:29]

Nit: const

[anitawoodruff, 2013/08/27 11:57:39]

Done.

+          extension->location() ==
+              extensions::Manifest::EXTERNAL_POLICY_DOWNLOAD;
+      if (!forceInstalled) {
+        // Notify user extension has crashed.
+        base::MessageLoop::current()->PostTask(
+            FROM_HERE, base::Bind(&ShowBalloon, extension, profile, false));
+      } else {
+        // Restart the extension; notify user if crash recurs frequently.
+        RestartForceInstalledExtensionOnCrash(extension, profile);
+      }
       break;
     }
     case chrome::NOTIFICATION_EXTENSION_UNLOADED:
@@ -419,9 +517,12 @@ void BackgroundContentsService::Observe(
       break;
 
     case chrome::NOTIFICATION_EXTENSION_UNINSTALLED: {
+      const std::string& extension_id =
+          content::Details<const Extension>(details).ptr()->id();
       // Remove any "This extension has crashed" balloons.
-      ScheduleCloseBalloon(
-          content::Details<const Extension>(details).ptr()->id());
+      ScheduleCloseBalloons(extension_id);
+      misbehaving_extensions_.erase(extension_id);
+      extension_crashlog_map_.erase(extension_id);
       break;
     }
 
@@ -431,6 +532,41 @@ void BackgroundContentsService::Observe(
   }
 }
 
+void BackgroundContentsService::RestartForceInstalledExtensionOnCrash(
+    const Extension* extension, Profile* profile) {

[anitawoodruff, 2013/08/27 09:05:36]

Is this the right place to put the extracted method?

[bartfab (slow), 2013/08/27 15:37:44]

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.

[anitawoodruff, 2013/08/27 17:34:29]

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.

+  const std::string& extension_id = extension->id();
+  const bool alreadyNotified = misbehaving_extensions_.find(extension_id) !=
+      misbehaving_extensions_.end();
+  std::queue<base::TimeTicks>& crashes = extension_crashlog_map_[extension_id];
+  const base::TimeDelta duration = base::TimeDelta::FromMilliseconds(
+      kForceInstalledExtensionCrashNotificationThreshold *
+          (restart_delay_millis_ + crash_window_millis_));
+  // Show a notification if the threshold number of crashes has occurred within
+  // a given duration.
+  const bool shouldNotify =

[bartfab (slow), 2013/08/27 09:21:29]

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.

[anitawoodruff, 2013/08/27 11:57:39]

Done.

+      !alreadyNotified &&
+      crashes.size() ==
+          kForceInstalledExtensionCrashNotificationThreshold - 1 &&
+      base::TimeTicks::Now() - crashes.front() < duration;
+  if (shouldNotify) {
+    base::MessageLoop::current()->PostTask(FROM_HERE,
+        base::Bind(&ShowBalloon, extension, profile, true));
+    misbehaving_extensions_.insert(extension_id);
+    extension_crashlog_map_.erase(extension_id);
+  } else if (!alreadyNotified) {
+    while (!crashes.empty() &&
+           base::TimeTicks::Now() - crashes.front() > duration) {
+      crashes.pop(); // Remove old timestamps.
+    }
+    crashes.push(base::TimeTicks::Now());
+    if (crashes.size() == kForceInstalledExtensionCrashNotificationThreshold)
+      crashes.pop();
+  }
+  base::MessageLoop::current()->PostDelayedTask(FROM_HERE,
+      base::Bind(&ReloadExtension, extension_id, profile),
+      base::TimeDelta::FromMilliseconds(restart_delay_millis_));
+}
+
 // Loads all background contents whose urls have been stored in prefs.
 void BackgroundContentsService::LoadBackgroundContentsFromPrefs(
     Profile* profile) {
@@ -653,7 +789,7 @@ void BackgroundContentsService::BackgroundContentsOpened(
   contents_map_[details->application_id].contents = details->contents;
   contents_map_[details->application_id].frame_name = details->frame_name;
 
-  ScheduleCloseBalloon(UTF16ToASCII(details->application_id));
+  ScheduleCloseCrashBalloon(UTF16ToASCII(details->application_id));
 }
 
 // Used by test code and debug checks to verify whether a given