Reload force-installed extensions on crash/force-close

chrome/browser/background/background_contents_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/background/background_contents_service.h"
 
 #include "apps/app_load_service.h"
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/background/background_contents_service_factory.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #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"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/background_info.h"
 #include "chrome/common/extensions/extension.h"
@@ skipping 12 matching lines @@
 #include "ui/gfx/image/image.h"
 
 using content::SiteInstance;
 using content::WebContents;
 using extensions::BackgroundInfo;
 using extensions::Extension;
 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,
                             const Extension* extension)
       : profile_(profile),
         is_hosted_app_(extension->is_hosted_app()),
         is_platform_app_(extension->is_platform_app()),
         extension_id_(extension->id()) {
   }
 
@@ skipping 20 matching lines @@
         service->LoadBackgroundContentsForExtension(profile_,
                                                     copied_extension_id);
     } else if (is_platform_app_) {
       apps::AppLoadService::Get(profile_)->
           RestartApplication(copied_extension_id);
     } else {
       extensions::ExtensionSystem::Get(profile_)->extension_service()->
           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 {
     return NULL;
   }
 
  private:
   virtual ~CrashNotificationDelegate() {}
 
   Profile* profile_;
   bool is_hosted_app_;
   bool is_platform_app_;
   std::string extension_id_;
 
   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);
   if (icon.IsEmpty()) {
     ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
     notification_icon = rb.GetImageNamed(IDR_EXTENSION_DEFAULT_ICON);
   }
   string16 title;  // no notification title
   DesktopNotificationService::AddIconNotification(extension_url,
                                                   title,
                                                   message,
                                                   notification_icon,
                                                   string16(),
                                                   delegate.get(),
                                                   profile);
 }
 #endif
 
-void ShowBalloon(const Extension* extension, Profile* profile) {
+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 = l10n_util::GetStringFUTF16(
-      extension->is_app() ? IDS_BACKGROUND_CRASHED_APP_BALLOON_MESSAGE :
-                            IDS_BACKGROUND_CRASHED_EXTENSION_BALLOON_MESSAGE,
-      UTF8ToUTF16(extension->name()));
-
+  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,
   // so we'll bind all the pertinent data here.
   extensions::ImageLoader::Get(profile)->LoadImageAsync(
       extension,
       resource,
       gfx::Size(size, size),
       base::Bind(
           &NotificationImageReady,
@@ skipping 15 matching lines @@
 //
 // kRegisteredBackgroundContents:
 //    DictionaryValue {
 //       <appid_1>: { "url": <url1>, "name": <frame_name> },
 //       <appid_2>: { "url": <url2>, "name": <frame_name> },
 //         ... etc ...
 //    }
 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) {
   // Don't load/store preferences if the proper switch is not enabled, or if
   // the parent profile is incognito.
   if (!profile->IsOffTheRecord() &&
       !command_line->HasSwitch(switches::kDisableRestoreBackgroundContents))
     prefs_ = profile->GetPrefs();
 
   // Listen for events to tell us when to load/unload persisted background
   // contents.
   StartObserving(profile);
 }
 
 BackgroundContentsService::~BackgroundContentsService() {
   // BackgroundContents should be shutdown before we go away, as otherwise
   // our browser process refcount will be off.
   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
 {
   std::vector<BackgroundContents*> contents;
   for (BackgroundContentsMap::const_iterator it = contents_map_.begin();
        it != contents_map_.end(); ++it)
     contents.push_back(it->second.contents);
   return contents;
 }
 
@@ skipping 104 matching lines @@
         if (service && service->is_ready()) {
           // Now load the manifest-specified background page. If service isn't
           // ready, then the background page will be loaded from the
           // EXTENSIONS_READY callback.
           LoadBackgroundContents(profile,
                                  BackgroundInfo::GetBackgroundURL(extension),
                                  ASCIIToUTF16("background"),
                                  UTF8ToUTF16(extension->id()));
         }
       }
-
       // Remove any "This extension has crashed" balloons.
-      ScheduleCloseBalloon(extension->id());
+      ScheduleCloseCrashBalloon(extension->id());
       SendChangeNotification(profile);
       break;
     }
     case chrome::NOTIFICATION_EXTENSION_PROCESS_TERMINATED:
     case chrome::NOTIFICATION_BACKGROUND_CONTENTS_TERMINATED: {
       Profile* profile = content::Source<Profile>(source).ptr();
       const Extension* extension = NULL;
       if (type == chrome::NOTIFICATION_BACKGROUND_CONTENTS_TERMINATED) {
         BackgroundContents* bg =
             content::Details<BackgroundContents>(details).ptr();
         std::string extension_id = UTF16ToASCII(
             BackgroundContentsServiceFactory::GetForProfile(profile)->
                 GetParentApplicationId(bg));
         extension =
           extensions::ExtensionSystem::Get(profile)->extension_service()->
               GetExtensionById(extension_id, false);
       } else {
         extensions::ExtensionHost* extension_host =
             content::Details<extensions::ExtensionHost>(details).ptr();
         extension = extension_host->extension();
       }
       if (!extension)
         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));
+      // 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:
       switch (content::Details<UnloadedExtensionInfo>(details)->reason) {
         case extension_misc::UNLOAD_REASON_DISABLE:    // Fall through.
         case extension_misc::UNLOAD_REASON_TERMINATE:  // Fall through.
         case extension_misc::UNLOAD_REASON_UNINSTALL:  // Fall through.
         case extension_misc::UNLOAD_REASON_BLACKLIST:
           ShutdownAssociatedBackgroundContents(
               ASCIIToUTF16(content::Details<UnloadedExtensionInfo>(details)->
@@ skipping 16 matching lines @@
         default:
           NOTREACHED();
           ShutdownAssociatedBackgroundContents(
               ASCIIToUTF16(content::Details<UnloadedExtensionInfo>(details)->
                   extension->id()));
           break;
       }
       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;
     }
 
     default:
       NOTREACHED();
       break;
   }
 }
 
+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) {
   if (!prefs_)
     return;
   const DictionaryValue* contents =
       prefs_->GetDictionary(prefs::kRegisteredBackgroundContents);
   if (!contents)
     return;
   ExtensionService* extensions_service =
@@ skipping 202 matching lines @@
 }
 
 void BackgroundContentsService::BackgroundContentsOpened(
     BackgroundContentsOpenedDetails* details) {
   // Add the passed object to our list. Should not already be tracked.
   DCHECK(!IsTracked(details->contents));
   DCHECK(!details->application_id.empty());
   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
 // BackgroundContents is being tracked by this instance.
 bool BackgroundContentsService::IsTracked(
     BackgroundContents* background_contents) const {
   return !GetParentApplicationId(background_contents).empty();
 }
 
 void BackgroundContentsService::BackgroundContentsShutdown(
@@ skipping 27 matching lines @@
     bool user_gesture,
     bool* was_blocked) {
   Browser* browser = chrome::FindLastActiveWithProfile(
       Profile::FromBrowserContext(new_contents->GetBrowserContext()),
       chrome::GetActiveDesktop());
   if (browser) {
     chrome::AddWebContents(browser, NULL, new_contents, disposition,
                            initial_pos, user_gesture, was_blocked);
   }
 }