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..7c2da182c592bf14062ca9ead814c15cf6ba4245 100644
--- a/chrome/browser/background/background_contents_service.cc
+++ b/chrome/browser/background/background_contents_service.cc
@@ -54,17 +54,30 @@ using extensions::UnloadedExtensionInfo;
 
 namespace {
 
-const char kNotificationPrefix[] = "app.background.crashed.";
+const char kCrashNotificationPrefix[] = "app.background.crashed.";
+const char kMisbehaveNotificationPrefix[] = "app.background.misbehaved.";
+
+/** No. of recent crashes required to trigger an extension-misbehave popup **/

[bartfab (slow), 2013/08/26 13:26:53]

Nit 1: Do not use abbreviations: No. => Number
Nit 2: Note that this applies to force-installed apps/extensions only.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+const unsigned int kNCrashesShouldNotify = 5;

[bartfab (slow), 2013/08/26 13:26:53]

Nit: This constant does not follow the usual Chrome constant naming. It should
more be something like kForceInstalledExtensionCrashNotificationThreshold.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

 
 void CloseBalloon(const std::string& id) {
   g_browser_process->notification_ui_manager()->CancelById(id);
 }
 
-void ScheduleCloseBalloon(const std::string& extension_id) {
+void ScheduleCloseBalloon(const std::string& id) {

[bartfab (slow), 2013/08/26 13:26:53]

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.

[anitawoodruff, 2013/08/26 21:45:23]

Done - is prefixed_extension_id ok? Or should I make up a new concept such as
balloon_id ?
Done. Is this clear enough now?

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

|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.

[anitawoodruff, 2013/08/27 10:00:51]

Done.

   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, id));
+}
+
+void ScheduleCloseCrashBalloon(const std::string& extension_id) {
+  ScheduleCloseBalloon(kCrashNotificationPrefix + extension_id);
+}
+
+void ScheduleCloseBalloons(const std::string& extension_id) {
+  ScheduleCloseBalloon(kMisbehaveNotificationPrefix + extension_id);
+  ScheduleCloseBalloon(kCrashNotificationPrefix + extension_id);
 }
 
 class CrashNotificationDelegate : public NotificationDelegate {
@@ -109,13 +122,13 @@ class CrashNotificationDelegate : public NotificationDelegate {
 
     // Closing the balloon here should be OK, but it causes a crash on Mac

[bartfab (slow), 2013/08/26 13:26:53]

Nit: "the balloon" is no longer unambiguous.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

     // http://crbug.com/78167
-    ScheduleCloseBalloon(copied_extension_id);
+    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 +146,44 @@ class CrashNotificationDelegate : public NotificationDelegate {
   DISALLOW_COPY_AND_ASSIGN(CrashNotificationDelegate);
 };
 
+class MisbehaveNotificationDelegate : public NotificationDelegate {

[bartfab (slow), 2013/08/26 13:26:53]

Nit 1: #include "chrome/browser/notifications/notification_delegate.h"

Nit 2: Add comments that note what CrashNotificationDelegate and
MisbehaveNotificationDelegate are for.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+ 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 +202,37 @@ 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 Extension* extension, Profile* profile) {
+  extensions::ExtensionSystem* extensionSystem =
+      extensions::ExtensionSystem::Get(profile);
+  if (extensionSystem && extensionSystem->extension_service())
+    extensionSystem->extension_service()->ReloadExtension(extension->id());
+}
 
+void ShowBalloon(const Extension* extension, Profile* profile, bool isForced) {

[bartfab (slow), 2013/08/26 13:26:53]

Nit: Add a comment explaining the parameters. "isForced" sounds like the balloon
is being forced :).

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+#if defined(ENABLE_NOTIFICATIONS)
+  string16 message;
+  if (isForced) {
+    message = l10n_util::GetStringFUTF16(
+        extension->is_app() ?
+            IDS_BACKGROUND_MISBEHAVING_APP_BALLOON_MESSAGE :
+            IDS_BACKGROUND_MISBEHAVING_EXTENSION_BALLOON_MESSAGE,
+        UTF8ToUTF16(extension->name()));
+  } else {
+    message = l10n_util::GetStringFUTF16(
+        extension->is_app() ? IDS_BACKGROUND_CRASHED_APP_BALLOON_MESSAGE :
+                              IDS_BACKGROUND_CRASHED_EXTENSION_BALLOON_MESSAGE,
+        UTF8ToUTF16(extension->name()));
+  }
   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);
+  scoped_refptr<NotificationDelegate> delegate = NULL;

[bartfab (slow), 2013/08/26 13:26:53]

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.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+  if (isForced)
+    delegate = new MisbehaveNotificationDelegate(extension);
+  else
+    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 +268,14 @@ void ShowBalloon(const Extension* extension, Profile* profile) {
 const char kUrlKey[] = "url";
 const char kFrameNameKey[] = "name";
 
+/** Delay (in seconds) before restarting a forced extension that crashed **/

[bartfab (slow), 2013/08/26 13:26:53]

Nit 1: s/forced/force-installed/
Nit 2: Move the comment to the header file instead.
Nit 3: We use //, not /** **/ in Chrome.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+base::TimeDelta BackgroundContentsService::restart_delay_ =
+    base::TimeDelta::FromSeconds(3);
+
+/** Seconds since a crash-reload during which we listen for further crashes **/

[bartfab (slow), 2013/08/26 13:26:53]

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.

[anitawoodruff, 2013/08/26 21:45:23]

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?

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

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.

+base::TimeDelta BackgroundContentsService::crash_window_ =
+    base::TimeDelta::FromSeconds(1);
+
 BackgroundContentsService::BackgroundContentsService(
     Profile* profile, const CommandLine* command_line)
     : prefs_(NULL) {
@@ -225,6 +296,13 @@ BackgroundContentsService::~BackgroundContentsService() {
   DCHECK(contents_map_.empty());
 }
 
+// static
+void BackgroundContentsService::SetCrashDelaysForTesting(
+    const base::TimeDelta& restart_delay, const base::TimeDelta& crash_window) {
+  restart_delay_ = restart_delay;
+  crash_window_ = crash_window;
+}
+
 std::vector<BackgroundContents*>
 BackgroundContentsService::GetBackgroundContents() const
 {
@@ -349,9 +427,8 @@ void BackgroundContentsService::Observe(
                                  UTF8ToUTF16(extension->id()));
         }
       }
-
       // Remove any "This extension has crashed" balloons.
-      ScheduleCloseBalloon(extension->id());
+      ScheduleCloseCrashBalloon(extension->id());
       SendChangeNotification(profile);
       break;
     }
@@ -377,12 +454,41 @@ void BackgroundContentsService::Observe(
         break;
 
       // 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));
+      // an EXTENSION_UNLOADED notification. We post the crash handling code as

[bartfab (slow), 2013/08/26 13:26:53]

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.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+      // a task here so it is not executed before the EXTENSION_UNLOADED event.
+      bool forceInstalled =
+          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.

[bartfab (slow), 2013/08/26 13:26:53]

I think this code is complex enough to warrant putting it into a separate
method.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+        const std::string& id = extension->id();

[bartfab (slow), 2013/08/26 13:26:53]

Nit: Why not call the variable extension_id?

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+        const bool alreadyNotified =
+            misbehaving_extensions_.find(id) != misbehaving_extensions_.end();
+        std::queue<base::TimeTicks>& crashes = extension_crashlog_map_[id];
+        const base::TimeDelta duration =
+            kNCrashesShouldNotify * (restart_delay_ + crash_window_);
+        if (!alreadyNotified && crashes.size() == (kNCrashesShouldNotify - 1) &&

[bartfab (slow), 2013/08/26 13:26:53]

Please put the calculation on its own line and explain what it does. It is too
complex to follow like this IMHO.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

+            base::TimeTicks::Now() - crashes.front() < duration) {
+          base::MessageLoop::current()->PostTask(FROM_HERE,
+              base::Bind(&ShowBalloon, extension, profile, true));
+          misbehaving_extensions_.insert(id);
+          extension_crashlog_map_.erase(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() == kNCrashesShouldNotify)
+            crashes.pop();
+        }
+        base::MessageLoop::current()->PostDelayedTask(FROM_HERE,

[bartfab (slow), 2013/08/26 13:26:53]

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. 

[anitawoodruff, 2013/08/26 21:45:23]

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?
 
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().
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?)

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

Sounds like both issues may arise - we may end up installing a deinstalled
extension and we may end up accessing an invalid profile.

+            base::Bind(&ReloadExtension, extension, profile), restart_delay_);
+      }
       break;
     }
     case chrome::NOTIFICATION_EXTENSION_UNLOADED:
@@ -419,9 +525,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;
     }
 
@@ -653,7 +762,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));
+  ScheduleCloseBalloons(UTF16ToASCII(details->application_id));

[bartfab (slow), 2013/08/26 13:26:53]

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.

[anitawoodruff, 2013/08/26 21:45:23]

Done.

 }
 
 // Used by test code and debug checks to verify whether a given