Allow extensions to unload while notifications are active.

NotificationDelegates kept a reference counted pointer to the
ApiFunction, thus preventing the extension from unloading until
the delegate was destroyed.  This allows notifications to inform
the delegate when the ApiFunction (and its RenderViewHost) are
no longer needed, after all the image downloads have completed.

BUG=177563

Review URL: https://chromiumcodereview.appspot.com/13872023

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@196601 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/api/notifications/notifications_api.cc

@@ -76,12 +76,19 @@ class NotificationsApiDelegate : public NotificationDelegate {
   }
 
   virtual content::RenderViewHost* GetRenderViewHost() const OVERRIDE {
-    // We're holding a reference to api_function_, so we know it'll be valid as
-    // long as we are, and api_function_ (as a UIThreadExtensionFunction)
-    // will zero out its copy of render_view_host when the RVH goes away.
+    // We're holding a reference to api_function_, so we know it'll be valid
+    // until ReleaseRVH is called, and api_function_ (as a
+    // UIThreadExtensionFunction) will zero out its copy of render_view_host
+    // when the RVH goes away.
+    if (!api_function_)
+      return NULL;
     return api_function_->render_view_host();
   }
 
+  virtual void ReleaseRenderViewHost() OVERRIDE {
+    api_function_ = NULL;
+  }
+
  private:
   virtual ~NotificationsApiDelegate() {}
 

chrome/browser/extensions/notifications_apitest.cc

@@ -11,6 +11,10 @@
 
 #if defined(ENABLE_MESSAGE_CENTER)
 
+#include "chrome/browser/extensions/extension_process_manager.h"
+#include "chrome/browser/extensions/lazy_background_page_test_util.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/extension.h"
 #include "ui/message_center/message_center_switches.h"
 #include "ui/message_center/message_center_util.h"
 
@@ -32,6 +36,26 @@ class DisabledRichWebkitNotificationTest : public ExtensionApiTest {
   }
 };
 
+class NotificationIdleTest : public RichWebkitNotificationTest {
+ protected:
+  virtual void SetUpCommandLine(CommandLine* command_line) {
+    RichWebkitNotificationTest::SetUpCommandLine(command_line);
+
+    command_line->AppendSwitchASCII(switches::kEventPageIdleTime, "1");
+    command_line->AppendSwitchASCII(switches::kEventPageSuspendingTime, "1");
+  }
+
+  const extensions::Extension* LoadExtensionAndWait(
+      const std::string& test_name) {
+    LazyBackgroundObserver page_complete;
+    base::FilePath extdir = test_data_dir_.AppendASCII(test_name);
+    const extensions::Extension* extension = LoadExtension(extdir);
+    if (extension)
+      page_complete.Wait();
+    return extension;
+  }
+};
+
 #endif
 
 // TODO(kbr): remove: http://crbug.com/222296
@@ -90,3 +114,16 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, NotificationsHasPermission) {
       << message_;
 #endif
 }
+
+#if defined(ENABLE_MESSAGE_CENTER)
+IN_PROC_BROWSER_TEST_F(NotificationIdleTest, NotificationsAllowUnload) {
+  const extensions::Extension* extension =
+      LoadExtensionAndWait("notifications/api/unload");
+  ASSERT_TRUE(extension) << message_;
+
+  // Lazy Background Page has been shut down.
+  ExtensionProcessManager* pm =
+      extensions::ExtensionSystem::Get(profile())->process_manager();
+  EXPECT_FALSE(pm->GetBackgroundHostForExtension(last_loaded_extension_id_));
+}
+#endif

chrome/browser/notifications/message_center_notification_manager.cc

@@ -258,14 +258,20 @@ void MessageCenterNotificationManager::OnNotificationDisplayed(
 // ImageDownloads
 
 MessageCenterNotificationManager::ImageDownloads::ImageDownloads(
-    message_center::MessageCenter* message_center)
-    : message_center_(message_center) {
+    message_center::MessageCenter* message_center,
+    ImageDownloadsObserver* observer)
+    : message_center_(message_center),
+      pending_downloads_(0),
+      observer_(observer) {
 }
 
 MessageCenterNotificationManager::ImageDownloads::~ImageDownloads() { }
 
 void MessageCenterNotificationManager::ImageDownloads::StartDownloads(
     const Notification& notification) {
+  // In case all downloads are synchronous, assume a pending download.
+  AddPendingDownload();
+
   // Notification primary icon.
   StartDownloadWithImage(
       notification,
@@ -299,6 +305,9 @@ void MessageCenterNotificationManager::ImageDownloads::StartDownloads(
       base::Bind(&message_center::MessageCenter::SetNotificationButtonIcon,
                  base::Unretained(message_center_),
                  notification.notification_id(), 1));
+
+  // This should tell the observer we're done if everything was synchronous.
+  PendingDownloadCompleted();
 }
 
 void MessageCenterNotificationManager::ImageDownloads::StartDownloadWithImage(
@@ -330,6 +339,8 @@ void MessageCenterNotificationManager::ImageDownloads::StartDownloadWithImage(
     return;
   }
 
+  AddPendingDownload();
+
   contents->DownloadImage(
       url,
       false,
@@ -359,12 +370,27 @@ void MessageCenterNotificationManager::ImageDownloads::DownloadComplete(
     const GURL& image_url,
     int requested_size,
     const std::vector<SkBitmap>& bitmaps) {
+  PendingDownloadCompleted();
+
   if (bitmaps.empty())
     return;
   gfx::Image image = gfx::Image::CreateFrom1xBitmap(bitmaps[0]);
   callback.Run(image);
 }
 
+// Private methods.
+
+void MessageCenterNotificationManager::ImageDownloads::AddPendingDownload() {
+  ++pending_downloads_;
+}
+
+void
+MessageCenterNotificationManager::ImageDownloads::PendingDownloadCompleted() {
+  DCHECK(pending_downloads_ > 0);
+  if (--pending_downloads_ == 0 && observer_)
+    observer_->OnDownloadsCompleted();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // ProfileNotification
 
@@ -374,7 +400,7 @@ MessageCenterNotificationManager::ProfileNotification::ProfileNotification(
     message_center::MessageCenter* message_center)
     : profile_(profile),
       notification_(notification),
-      downloads_(new ImageDownloads(message_center)) {
+      downloads_(new ImageDownloads(message_center, this)) {
   DCHECK(profile);
 }
 
@@ -385,6 +411,12 @@ void MessageCenterNotificationManager::ProfileNotification::StartDownloads() {
   downloads_->StartDownloads(notification_);
 }
 
+void
+MessageCenterNotificationManager::ProfileNotification::OnDownloadsCompleted() {
+  notification_.DoneRendering();
+}
+
+
 std::string
     MessageCenterNotificationManager::ProfileNotification::GetExtensionId() {
   const ExtensionURLInfo url(notification().origin_url());

chrome/browser/notifications/message_center_notification_manager.h

@@ -63,11 +63,18 @@ class MessageCenterNotificationManager
       const std::string& notification_id) OVERRIDE;
 
  private:
+  class ImageDownloadsObserver {
+   public:
+    virtual void OnDownloadsCompleted() = 0;
+  };
+
   typedef base::Callback<void(const gfx::Image&)> SetImageCallback;
   class ImageDownloads
       : public base::SupportsWeakPtr<ImageDownloads> {
    public:
-    explicit ImageDownloads(message_center::MessageCenter* message_center);
+    ImageDownloads(
+        message_center::MessageCenter* message_center,
+        ImageDownloadsObserver* observer);
     virtual ~ImageDownloads();
 
     void StartDownloads(const Notification& notification);
@@ -88,8 +95,21 @@ class MessageCenterNotificationManager
                           int requested_size,
                           const std::vector<SkBitmap>& bitmaps);
    private:
+    // Used to keep track of the number of pending downloads.  Once this
+    // reaches zero, we can tell the delegate that we don't need the
+    // RenderViewHost anymore.
+    void AddPendingDownload();
+    void PendingDownloadCompleted();
+
     // Weak reference to global message center.
     message_center::MessageCenter* message_center_;
+
+    // Count of downloads that remain.
+    size_t pending_downloads_;
+
+    // Weak.
+    ImageDownloadsObserver* observer_;
+
     DISALLOW_COPY_AND_ASSIGN(ImageDownloads);
   };
 
@@ -105,7 +125,7 @@ class MessageCenterNotificationManager
   // TODO(dimich): Consider merging all 4 types (Notification,
   // QueuedNotification, ProfileNotification and NotificationList::Notification)
   // into a single class.
-  class ProfileNotification {
+  class ProfileNotification : public ImageDownloadsObserver {
    public:
     ProfileNotification(Profile* profile,
                         const Notification& notification,
@@ -114,6 +134,9 @@ class MessageCenterNotificationManager
 
     void StartDownloads();
 
+    // Overridden from ImageDownloadsObserver.
+    virtual void OnDownloadsCompleted() OVERRIDE;
+
     Profile* profile() const { return profile_; }
     const Notification& notification() const { return notification_; }
 

chrome/browser/notifications/notification.h

@@ -111,6 +111,7 @@ class Notification {
   void Click() const { delegate()->Click(); }
   void ButtonClick(int index) const { delegate()->ButtonClick(index); }
   void Close(bool by_user) const { delegate()->Close(by_user); }
+  void DoneRendering() { delegate()->ReleaseRenderViewHost(); }
 
   std::string notification_id() const { return delegate()->id(); }
 

chrome/browser/notifications/notification_delegate.cc

@@ -6,3 +6,6 @@
 
 void NotificationDelegate::ButtonClick(int button_index) {
 }
+
+void NotificationDelegate::ReleaseRenderViewHost() {
+}

chrome/browser/notifications/notification_delegate.h

@@ -44,6 +44,9 @@ class NotificationDelegate
   // Returns the RenderViewHost that generated the notification, or NULL.
   virtual content::RenderViewHost* GetRenderViewHost() const = 0;
 
+  // Lets the delegate know that no more rendering will be necessary.
+  virtual void ReleaseRenderViewHost();
+
  protected:
   virtual ~NotificationDelegate() {}
 

chrome/test/data/extensions/api_test/notifications/api/unload/background.js

@@ -0,0 +1,31 @@
+// 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.
+
+const notifications = chrome.notifications;
+
+var idString = "foo";
+
+var testBasicEvents = function() {
+  var incidents = 0;
+
+  var onCreateCallback = function(id) {
+    chrome.test.assertTrue(id.length > 0);
+    chrome.test.assertEq(idString, id);
+    chrome.test.succeed();
+  }
+
+  var red_dot = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUA" +
+      "AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO" +
+      "9TXL0Y4OHwAAAABJRU5ErkJggg==";
+
+  var options = {
+    type: "basic",
+    iconUrl: red_dot,
+    title: "Attention!",
+    message: "Check out Cirque du Soleil"
+  };
+  notifications.create(idString, options, onCreateCallback);
+};
+
+chrome.test.runTests([ testBasicEvents ]);

chrome/test/data/extensions/api_test/notifications/api/unload/manifest.json

@@ -0,0 +1,13 @@
+{
+  "name": "chrome.notifications",
+  "version": "0.1",
+  "description": "chrome.notifications API events",
+  "app": {
+    "background": {
+      "scripts": ["background.js"]
+    }
+  },
+  "permissions": [
+    "notifications"
+  ]
+}