Ensure event pages are unloaded when onSuspend calls an API function

Extensions and apps which call API functions with callbacks such as
chrome.storage.local.set() and chrome.storage.local.get() in the
onSuspend event handler prevents the event page from ever being
unloaded.

The API function finishes between ExtensionProcessManager::OnSuspendAck()
and ExtensionProcessManager::CloseLazyBackgroundPageNow(). The destructor
of UIThreadExtensionFunction results in DecrementLazyKeepaliveCount()
being called in between these two functions, which increments
close_sequence_id. This causes CloseLazyBackgroundPageNow() to drop the
delayed close.

API functions which finish before OnSuspendAck() will not cause this
problem.

Fix:
DecrementLazyKeepAliveCount() should check the is_closing flag before
incrementing close_sequence_id.

BUG=296834
TEST=browser_tests (AppEventPageTest*:LazyBackgroundPageApiTest*)
See bug for how to reproduce and verify issue.

Review URL: https://codereview.chromium.org/24712003

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

chrome/browser/apps/event_page_browsertest.cc

@@ -0,0 +1,54 @@
+// Copyright 2013 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/apps/app_browsertest_util.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/extension_test_message_listener.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/test_utils.h"
+
+using extensions::Extension;
+using extensions::PlatformAppBrowserTest;
+
+namespace {
+
+class AppEventPageTest : public PlatformAppBrowserTest {
+ protected:
+  void TestUnloadEventPage(const char* app_path) {
+    // Load and launch the app.
+    ExtensionTestMessageListener launched_listener("launched", false);
+    const Extension* extension = LoadAndLaunchPlatformApp(app_path);
+    ASSERT_TRUE(extension);
+    ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
+
+    content::WindowedNotificationObserver event_page_suspended(
+        chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
+        content::NotificationService::AllSources());
+
+    // Close the app window.
+    EXPECT_EQ(1U, GetShellWindowCount());
+    apps::ShellWindow* shell_window = GetFirstShellWindow();
+    ASSERT_TRUE(shell_window);
+    CloseShellWindow(shell_window);
+
+    // Verify that the event page is destroyed.
+    event_page_suspended.Wait();
+  }
+};
+
+}  // namespace
+
+// Tests that an app's event page will eventually be unloaded. The onSuspend
+// event handler of this app does not make any API calls.
+IN_PROC_BROWSER_TEST_F(AppEventPageTest, OnSuspendNoApiUse) {
+  TestUnloadEventPage("event_page/suspend_simple");
+}
+
+// Tests that an app's event page will eventually be unloaded. The onSuspend
+// event handler of this app calls a chrome.storage API function.
+// See: http://crbug.com/296834
+IN_PROC_BROWSER_TEST_F(AppEventPageTest, OnSuspendUseStorageApi) {
+  TestUnloadEventPage("event_page/suspend_storage_api");
+}

chrome/browser/extensions/extension_process_manager.cc

@@ -435,7 +435,12 @@ int ExtensionProcessManager::DecrementLazyKeepaliveCount(
 
   int& count = background_page_data_[extension->id()].lazy_keepalive_count;
   DCHECK_GT(count, 0);
-  if (--count == 0) {
+
+  // If we reach a zero keepalive count when the lazy background page is about
+  // to be closed, incrementing close_sequence_id will cancel the close
+  // sequence and cause the background page to linger. So check is_closing
+  // before initiating another close sequence.
+  if (--count == 0 && !background_page_data_[extension->id()].is_closing) {
     base::MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&ExtensionProcessManager::OnLazyBackgroundPageIdle,

chrome/browser/extensions/lazy_background_page_apitest.cc

@@ -503,5 +503,12 @@ IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, UpdateExtensionsPage) {
   EXPECT_TRUE(is_inactive);
 }
 
+// Tests that the lazy background page will be unloaded if the onSuspend event
+// handler calls an API function such as chrome.storage.local.set().
+// See: http://crbug.com/296834
+IN_PROC_BROWSER_TEST_F(LazyBackgroundPageApiTest, OnSuspendUseStorageApi) {
+  EXPECT_TRUE(LoadExtensionAndWait("on_suspend"));
+}
+
 // TODO: background page with timer.
 // TODO: background page that interacts with popup.

chrome/chrome_tests.gypi

@@ -940,6 +940,7 @@
         'browser/apps/app_browsertest_util.cc',
         'browser/apps/app_browsertest_util.h',
         'browser/apps/app_url_redirector_browsertest.cc',
+        'browser/apps/event_page_browsertest.cc',
         'browser/apps/speech_recognition_browsertest.cc',
         'browser/apps/web_view_browsertest.cc',
         'browser/apps/window_controls_browsertest.cc',

chrome/test/data/extensions/api_test/lazy_background_page/on_suspend/background.js

@@ -0,0 +1,10 @@
+// Copyright 2013 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.
+
+chrome.runtime.onSuspend.addListener(function() {
+  var now = new Date();
+  chrome.storage.local.set({"last_save": now.toLocaleString()}, function() {
+    console.log("Finished writing last_save: " + now.toLocaleString());
+  });
+});

chrome/test/data/extensions/api_test/lazy_background_page/on_suspend/manifest.json

@@ -0,0 +1,10 @@
+{
+  "name": "Lazy BG onSuspend Test",
+  "version": "1.0",
+  "manifest_version": 2,
+  "permissions": ["storage"],
+  "background": {
+    "scripts": ["background.js"],
+    "persistent": false
+  }
+}

chrome/test/data/extensions/platform_apps/event_page/suspend_simple/index.html

@@ -0,0 +1,13 @@
+<!--
+ * Copyright 2013 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.
+-->
+<!DOCTYPE html>
+<html>
+<head>
+</head>
+<body>
+  <script src="index.js"></script>
+</body>
+</html>

chrome/test/data/extensions/platform_apps/event_page/suspend_simple/index.js

@@ -0,0 +1,7 @@
+// Copyright 2013 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.
+
+onload = function() {
+  chrome.test.sendMessage('launched');
+};

chrome/test/data/extensions/platform_apps/event_page/suspend_simple/main.js

@@ -0,0 +1,25 @@
+// Copyright 2013 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.
+
+chrome.app.runtime.onLaunched.addListener(function() {
+
+  var screenWidth = screen.availWidth;
+  var screenHeight = screen.availHeight;
+  var width = 500;
+  var height = 300;
+
+  chrome.app.window.create('index.html', {
+    bounds: {
+      width: width,
+      height: height,
+      left: Math.round((screenWidth-width)/2),
+      top: Math.round((screenHeight-height)/2)
+    }
+  });
+});
+
+chrome.runtime.onSuspend.addListener(function() {
+  var now = new Date();
+  console.log("The current time is: " + now.toLocaleString());
+});

chrome/test/data/extensions/platform_apps/event_page/suspend_simple/manifest.json

@@ -0,0 +1,10 @@
+{
+  "name": "Event Page Test - Simple",
+  "version": "1.0",
+  "manifest_version": 2,
+  "app": {
+    "background": {
+      "scripts": ["main.js"]
+    }
+  }
+}

chrome/test/data/extensions/platform_apps/event_page/suspend_storage_api/index.html

@@ -0,0 +1,13 @@
+<!--
+ * Copyright 2013 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.
+-->
+<!DOCTYPE html>
+<html>
+<head>
+</head>
+<body>
+  <script src="index.js"></script>
+</body>
+</html>

chrome/test/data/extensions/platform_apps/event_page/suspend_storage_api/index.js

@@ -0,0 +1,7 @@
+// Copyright 2013 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.
+
+onload = function() {
+  chrome.test.sendMessage('launched');
+};

chrome/test/data/extensions/platform_apps/event_page/suspend_storage_api/main.js

@@ -0,0 +1,27 @@
+// Copyright 2013 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.
+
+chrome.app.runtime.onLaunched.addListener(function() {
+
+  var screenWidth = screen.availWidth;
+  var screenHeight = screen.availHeight;
+  var width = 500;
+  var height = 300;
+
+  chrome.app.window.create('index.html', {
+    bounds: {
+      width: width,
+      height: height,
+      left: Math.round((screenWidth-width)/2),
+      top: Math.round((screenHeight-height)/2)
+    }
+  });
+});
+
+chrome.runtime.onSuspend.addListener(function() {
+  var now = new Date();
+  chrome.storage.local.set({"last_save": now.toLocaleString()}, function() {
+    console.log("Finished writing last_save: " + now.toLocaleString());
+  });
+});

chrome/test/data/extensions/platform_apps/event_page/suspend_storage_api/manifest.json

@@ -0,0 +1,11 @@
+{
+  "name": "Event Page Test - Storage API",
+  "version": "1.0",
+  "manifest_version": 2,
+  "permissions": ["storage"],
+  "app": {
+    "background": {
+      "scripts": ["main.js"]
+    }
+  }
+}