Hidden windows should not keep Chrome alive.
For each app window, there is a ChromeAppDelegate which
holds a ScopedKeepAlive. This CL resets the ScopedKeepAlive
while the app window is hidden.
BUG=450710
Review URL: https://codereview.chromium.org/875273003
Cr-Commit-Position: refs/heads/master@{#316413}
(cherry picked from commit 39d75d5b5f11525e384745fd313d1d77ed7d0a88)
TBR=benwells@chromium.org
Review URL: https://codereview.chromium.org/927203002
Cr-Commit-Position: refs/branch-heads/2272@{#304}
Cr-Branched-From: 827a380cfdb31aa54c8d56e63ce2c3fd8c3ba4d4-refs/heads/master@{#310958}
diff --git a/chrome/browser/apps/app_window_interactive_uitest.cc b/chrome/browser/apps/app_window_interactive_uitest.cc
index 1212c206..30247d7 100644
--- a/chrome/browser/apps/app_window_interactive_uitest.cc
+++ b/chrome/browser/apps/app_window_interactive_uitest.cc
@@ -3,6 +3,9 @@
// found in the LICENSE file.
#include "chrome/browser/apps/app_browsertest_util.h"
+#include "chrome/browser/lifetime/application_lifetime.h"
+#include "chrome/browser/ui/browser_iterator.h"
+#include "chrome/browser/ui/browser_window.h"
#include "chrome/test/base/interactive_test_utils.h"
#include "extensions/browser/app_window/native_app_window.h"
#include "extensions/test/extension_test_message_listener.h"
@@ -457,3 +460,78 @@
IN_PROC_BROWSER_TEST_F(AppWindowInteractiveTest, TestDrawAttention) {
ASSERT_TRUE(RunAppWindowInteractiveTest("testDrawAttention")) << message_;
}
+
+// Only Linux and Windows use keep-alive to determine when to shut down.
+#if defined(OS_LINUX) || defined(OS_WIN)
+
+// In general, hidden windows should not keep Chrome alive. The exception is
+// when windows are created hidden, we allow the app some time to show the
+// the window.
+class AppWindowHiddenKeepAliveTest : public extensions::PlatformAppBrowserTest {
+ protected:
+ AppWindowHiddenKeepAliveTest() {}
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(AppWindowHiddenKeepAliveTest);
+};
+
+// A window that becomes hidden should not keep Chrome alive.
+IN_PROC_BROWSER_TEST_F(AppWindowHiddenKeepAliveTest, ShownThenHidden) {
+ LoadAndLaunchPlatformApp("minimal", "Launched");
+ for (chrome::BrowserIterator it; !it.done(); it.Next())
+ it->window()->Close();
+
+ EXPECT_TRUE(chrome::WillKeepAlive());
+ GetFirstAppWindow()->Hide();
+ EXPECT_FALSE(chrome::WillKeepAlive());
+}
+
+// A window that is hidden but re-shown should still keep Chrome alive.
+IN_PROC_BROWSER_TEST_F(AppWindowHiddenKeepAliveTest, ShownThenHiddenThenShown) {
+ LoadAndLaunchPlatformApp("minimal", "Launched");
+ AppWindow* app_window = GetFirstAppWindow();
+ app_window->Hide();
+ app_window->Show(AppWindow::SHOW_ACTIVE);
+
+ EXPECT_TRUE(chrome::WillKeepAlive());
+ for (chrome::BrowserIterator it; !it.done(); it.Next())
+ it->window()->Close();
+ EXPECT_TRUE(chrome::WillKeepAlive());
+ app_window->GetBaseWindow()->Close();
+}
+
+// A window that is created hidden and stays hidden should not keep Chrome
+// alive.
+IN_PROC_BROWSER_TEST_F(AppWindowHiddenKeepAliveTest, StaysHidden) {
+ LoadAndLaunchPlatformApp("hidden", "Launched");
+ AppWindow* app_window = GetFirstAppWindow();
+ EXPECT_TRUE(app_window->is_hidden());
+
+ for (chrome::BrowserIterator it; !it.done(); it.Next())
+ it->window()->Close();
+ // This will time out if the command above does not terminate Chrome.
+ content::RunMessageLoop();
+}
+
+// A window that is created hidden but shown soon after should keep Chrome
+// alive.
+IN_PROC_BROWSER_TEST_F(AppWindowHiddenKeepAliveTest, HiddenThenShown) {
+ ExtensionTestMessageListener launched_listener("Launched", true);
+ LoadAndLaunchPlatformApp("hidden_then_shown", &launched_listener);
+ AppWindow* app_window = GetFirstAppWindow();
+ EXPECT_TRUE(app_window->is_hidden());
+
+ // Close all browser windows.
+ for (chrome::BrowserIterator it; !it.done(); it.Next())
+ it->window()->Close();
+
+ // The app window will show after 3 seconds.
+ ExtensionTestMessageListener shown_listener("Shown", false);
+ launched_listener.Reply("");
+ EXPECT_TRUE(shown_listener.WaitUntilSatisfied());
+ EXPECT_FALSE(app_window->is_hidden());
+ EXPECT_TRUE(chrome::WillKeepAlive());
+ app_window->GetBaseWindow()->Close();
+}
+
+#endif
diff --git a/chrome/browser/ui/apps/chrome_app_delegate.cc b/chrome/browser/ui/apps/chrome_app_delegate.cc
index 4fbff22b..96ac0bf 100644
--- a/chrome/browser/ui/apps/chrome_app_delegate.cc
+++ b/chrome/browser/ui/apps/chrome_app_delegate.cc
@@ -25,6 +25,7 @@
#include "chrome/common/extensions/chrome_extension_messages.h"
#include "components/ui/zoom/zoom_controller.h"
#include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
#include "content/public/browser/notification_service.h"
#include "content/public/browser/render_view_host.h"
#include "content/public/browser/web_contents.h"
@@ -46,6 +47,9 @@
namespace {
+// Time to wait for an app window to show before allowing Chrome to quit.
+int kAppWindowFirstShowTimeoutSeconds = 10;
+
bool disable_external_open_for_testing_ = false;
// Opens a URL with Chromium (not external browser) with the right profile.
@@ -110,6 +114,16 @@
} // namespace
+// static
+void ChromeAppDelegate::RelinquishKeepAliveAfterTimeout(
+ const base::WeakPtr<ChromeAppDelegate>& chrome_app_delegate) {
+ // Resetting the ScopedKeepAlive may cause nested destruction of the
+ // ChromeAppDelegate which also resets the ScopedKeepAlive. To avoid this,
+ // move the ScopedKeepAlive out to here and let it fall out of scope.
+ if (chrome_app_delegate.get() && chrome_app_delegate->is_hidden_)
+ scoped_ptr<ScopedKeepAlive>(chrome_app_delegate->keep_alive_.Pass());
+}
+
class ChromeAppDelegate::NewWindowContentsDelegate
: public content::WebContentsDelegate {
public:
@@ -150,8 +164,11 @@
}
ChromeAppDelegate::ChromeAppDelegate(scoped_ptr<ScopedKeepAlive> keep_alive)
- : keep_alive_(keep_alive.Pass()),
- new_window_contents_delegate_(new NewWindowContentsDelegate()) {
+ : has_been_shown_(false),
+ is_hidden_(true),
+ keep_alive_(keep_alive.Pass()),
+ new_window_contents_delegate_(new NewWindowContentsDelegate()),
+ weak_factory_(this) {
registrar_.Add(this,
chrome::NOTIFICATION_APP_TERMINATING,
content::NotificationService::AllSources());
@@ -285,6 +302,28 @@
terminating_callback_ = callback;
}
+void ChromeAppDelegate::OnHide() {
+ is_hidden_ = true;
+ if (has_been_shown_) {
+ keep_alive_.reset();
+ return;
+ }
+
+ // Hold on to the keep alive for some time to give the app a chance to show
+ // the window.
+ content::BrowserThread::PostDelayedTask(
+ content::BrowserThread::UI, FROM_HERE,
+ base::Bind(&ChromeAppDelegate::RelinquishKeepAliveAfterTimeout,
+ weak_factory_.GetWeakPtr()),
+ base::TimeDelta::FromSeconds(kAppWindowFirstShowTimeoutSeconds));
+}
+
+void ChromeAppDelegate::OnShow() {
+ has_been_shown_ = true;
+ is_hidden_ = false;
+ keep_alive_.reset(new ScopedKeepAlive);
+}
+
void ChromeAppDelegate::Observe(int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) {
diff --git a/chrome/browser/ui/apps/chrome_app_delegate.h b/chrome/browser/ui/apps/chrome_app_delegate.h
index e08e613..8501e64 100644
--- a/chrome/browser/ui/apps/chrome_app_delegate.h
+++ b/chrome/browser/ui/apps/chrome_app_delegate.h
@@ -7,6 +7,7 @@
#include "base/callback.h"
#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "extensions/browser/app_window/app_delegate.h"
@@ -31,6 +32,9 @@
static void DisableExternalOpenForTesting();
private:
+ static void RelinquishKeepAliveAfterTimeout(
+ const base::WeakPtr<ChromeAppDelegate>& chrome_app_delegate);
+
class NewWindowContentsDelegate;
// extensions::AppDelegate:
@@ -66,16 +70,21 @@
bool blocked) override;
bool IsWebContentsVisible(content::WebContents* web_contents) override;
void SetTerminatingCallback(const base::Closure& callback) override;
+ void OnHide() override;
+ void OnShow() override;
// content::NotificationObserver:
void Observe(int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) override;
+ bool has_been_shown_;
+ bool is_hidden_;
scoped_ptr<ScopedKeepAlive> keep_alive_;
scoped_ptr<NewWindowContentsDelegate> new_window_contents_delegate_;
base::Closure terminating_callback_;
content::NotificationRegistrar registrar_;
+ base::WeakPtrFactory<ChromeAppDelegate> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(ChromeAppDelegate);
};
diff --git a/chrome/test/data/extensions/platform_apps/hidden/test.js b/chrome/test/data/extensions/platform_apps/hidden/test.js
index c110227..15a1b5e7 100644
--- a/chrome/test/data/extensions/platform_apps/hidden/test.js
+++ b/chrome/test/data/extensions/platform_apps/hidden/test.js
@@ -3,7 +3,7 @@
// found in the LICENSE file.
chrome.app.runtime.onLaunched.addListener(function() {
- chrome.app.window.create('main.html', {
+ chrome.app.window.create('empty.html', {
hidden: true,
}, function () {
chrome.test.sendMessage('Launched');
diff --git a/chrome/test/data/extensions/platform_apps/hidden_then_shown/empty.html b/chrome/test/data/extensions/platform_apps/hidden_then_shown/empty.html
new file mode 100644
index 0000000..aabcd1b
--- /dev/null
+++ b/chrome/test/data/extensions/platform_apps/hidden_then_shown/empty.html
@@ -0,0 +1 @@
+<!-- This file intentionally left blank. -->
diff --git a/chrome/test/data/extensions/platform_apps/hidden_then_shown/manifest.json b/chrome/test/data/extensions/platform_apps/hidden_then_shown/manifest.json
new file mode 100644
index 0000000..59a8a94
--- /dev/null
+++ b/chrome/test/data/extensions/platform_apps/hidden_then_shown/manifest.json
@@ -0,0 +1,10 @@
+{
+ "name": "Platform App Hidden Then Shown",
+ "version": "1",
+ "manifest_version": 2,
+ "app": {
+ "background": {
+ "scripts": ["test.js"]
+ }
+ }
+}
diff --git a/chrome/test/data/extensions/platform_apps/hidden_then_shown/test.js b/chrome/test/data/extensions/platform_apps/hidden_then_shown/test.js
new file mode 100644
index 0000000..d1a7ff13
--- /dev/null
+++ b/chrome/test/data/extensions/platform_apps/hidden_then_shown/test.js
@@ -0,0 +1,16 @@
+// Copyright 2014 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() {
+ chrome.app.window.create('empty.html', {
+ hidden: true,
+ }, function (win) {
+ chrome.test.sendMessage('Launched', function () {
+ setTimeout(function () {
+ win.show();
+ chrome.test.sendMessage('Shown');
+ }, 3000);
+ });
+ });
+});
diff --git a/extensions/browser/app_window/app_delegate.h b/extensions/browser/app_window/app_delegate.h
index 0e4477e..fd78d106 100644
--- a/extensions/browser/app_window/app_delegate.h
+++ b/extensions/browser/app_window/app_delegate.h
@@ -76,6 +76,10 @@
// |callback| will be called when the process is about to terminate.
virtual void SetTerminatingCallback(const base::Closure& callback) = 0;
+
+ // Called when the app is hidden or shown.
+ virtual void OnHide() = 0;
+ virtual void OnShow() = 0;
};
} // namespace extensions
diff --git a/extensions/browser/app_window/app_window.cc b/extensions/browser/app_window/app_window.cc
index 7fdaeffb..83bd201 100644
--- a/extensions/browser/app_window/app_window.cc
+++ b/extensions/browser/app_window/app_window.cc
@@ -658,6 +658,7 @@
}
void AppWindow::Show(ShowType show_type) {
+ app_delegate_->OnShow();
bool was_hidden = is_hidden_ || !has_been_shown_;
is_hidden_ = false;
@@ -694,6 +695,7 @@
show_on_first_paint_ = false;
GetBaseWindow()->Hide();
AppWindowRegistry::Get(browser_context_)->AppWindowHidden(this);
+ app_delegate_->OnHide();
}
void AppWindow::SetAlwaysOnTop(bool always_on_top) {
diff --git a/extensions/shell/browser/shell_app_delegate.h b/extensions/shell/browser/shell_app_delegate.h
index 8c9be2e..b458155 100644
--- a/extensions/shell/browser/shell_app_delegate.h
+++ b/extensions/shell/browser/shell_app_delegate.h
@@ -50,6 +50,8 @@
bool blocked) override;
bool IsWebContentsVisible(content::WebContents* web_contents) override;
void SetTerminatingCallback(const base::Closure& callback) override;
+ void OnHide() override {}
+ void OnShow() override {}
// content::WebContentsObserver:
void RenderViewCreated(content::RenderViewHost* render_view_host) override;