Skip to content

Commit

Permalink
Extensions: Navigate to default chrome page on extension unload
Browse files Browse the repository at this point in the history
Currently, when an extension is unloaded, Chrome closes all tabs
associated with the extensions content. If the extension overrode a
Chrome page (namely NTP, bookmarks, history), Chrome would not close
that tab and instead refresh/navigate to the default Chrome page.

However, when there is only one tab in the browser; the tab contains
extension related content; and the tab is not a Chrome page override,
Chrome would close the tab, which thereby closes the browser window.

In such case, Chrome should not close the tab, but instead navigate to
the default NTP chrome page.

Bug: 794472
Change-Id: I5a3438817ad7f442fdd534efe481e35dd7b36a4a
Reviewed-on: https://chromium-review.googlesource.com/862183
Commit-Queue: catmullings <catmullings@chromium.org>
Reviewed-by: catmullings <catmullings@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530015}
  • Loading branch information
Catherine Mullings authored and Commit Bot committed Jan 18, 2018
1 parent 1a7bde5 commit 4d546d6
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,16 @@ class BrowserActionInteractiveTest : public ExtensionApiTest {
}

// Open an extension popup via the chrome.browserAction.openPopup API.
void OpenPopupViaAPI() {
void OpenPopupViaAPI(bool will_reply) {
// Setup the notification observer to wait for the popup to finish loading.
content::WindowedNotificationObserver frame_observer(
content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
content::NotificationService::AllSources());
ExtensionTestMessageListener listener("ready", will_reply);
// Show first popup in first window and expect it to have loaded.
ASSERT_TRUE(RunExtensionSubtest("browser_action/open_popup",
"open_popup_succeeds.html")) << message_;
EXPECT_TRUE(listener.WaitUntilSatisfied());
frame_observer.Wait();
EnsurePopupActive();
}
Expand Down Expand Up @@ -205,7 +207,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, TestOpenPopup) {
// Setup extension message listener to wait for javascript to finish running.
ExtensionTestMessageListener listener("ready", true);
{
OpenPopupViaAPI();
OpenPopupViaAPI(true);
EXPECT_TRUE(browserActionBar.HasPopup());
browserActionBar.HidePopup();
}
Expand Down Expand Up @@ -331,7 +333,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
if (!ShouldRunPopupTest())
return;

OpenPopupViaAPI();
OpenPopupViaAPI(false);
ExtensionService* service = extensions::ExtensionSystem::Get(
browser()->profile())->extension_service();
ASSERT_FALSE(
Expand All @@ -348,7 +350,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, FocusLossClosesPopup1) {
if (!ShouldRunPopupTest())
return;
OpenPopupViaAPI();
OpenPopupViaAPI(false);
ClosePopupViaFocusLoss();
}

Expand Down Expand Up @@ -376,7 +378,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, TabSwitchClosesPopup) {
ASSERT_EQ(2, browser()->tab_strip_model()->count());
EXPECT_EQ(browser()->tab_strip_model()->GetWebContentsAt(1),
browser()->tab_strip_model()->GetActiveWebContents());
OpenPopupViaAPI();
OpenPopupViaAPI(false);

content::WindowedNotificationObserver observer(
extensions::NOTIFICATION_EXTENSION_HOST_DESTROYED,
Expand All @@ -394,7 +396,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
return;

// First, we open a popup.
OpenPopupViaAPI();
OpenPopupViaAPI(false);
BrowserActionTestUtil browser_action_test_util(browser());
EXPECT_TRUE(browser_action_test_util.HasPopup());

Expand Down Expand Up @@ -451,7 +453,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
if (!ShouldRunPopupTest())
return;

OpenPopupViaAPI();
OpenPopupViaAPI(false);
BrowserActionTestUtil test_util(browser());
const gfx::NativeView view = test_util.GetPopupNativeView();
EXPECT_NE(static_cast<gfx::NativeView>(NULL), view);
Expand Down
70 changes: 70 additions & 0 deletions chrome/browser/extensions/extension_web_ui_browsertest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright 2018 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/extensions/extension_browsertest.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/common/url_constants.h"
#include "chrome/test/base/ui_test_utils.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test_utils.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/test_extension_registry_observer.h"
#include "extensions/common/extension.h"
#include "net/test/embedded_test_server/embedded_test_server.h"

using ExtensionWebUIBrowserTest = ExtensionBrowserTest;

namespace extensions {

IN_PROC_BROWSER_TEST_F(ExtensionWebUIBrowserTest,
NavigateToDefaultChromePageOnExtensionUnload) {
TabStripModel* tab_strip_model = browser()->tab_strip_model();

const Extension* extension =
LoadExtension(test_data_dir_.AppendASCII("api_test")
.AppendASCII("override")
.AppendASCII("history"));
ASSERT_TRUE(extension);
std::string extension_id = extension->id();

// Open the chrome://history/ page in the current tab.
GURL extension_url(chrome::kChromeUIHistoryURL);
ui_test_utils::NavigateToURL(browser(), extension_url);
content::WaitForLoadStop(tab_strip_model->GetActiveWebContents());

// Check that the chrome://history/ page contains the extension's content.
std::string location;
EXPECT_TRUE(content::ExecuteScriptAndExtractString(
browser()->tab_strip_model()->GetActiveWebContents(),
"domAutomationController.send(location.href);\n", &location));
EXPECT_EQ(extension_id, GURL(location).host()); // Extension has control.

ASSERT_EQ(1, tab_strip_model->count());

// Uninstall the extension.
ExtensionService* service =
ExtensionSystem::Get(browser()->profile())->extension_service();
ExtensionRegistry* registry = ExtensionRegistry::Get(browser()->profile());
TestExtensionRegistryObserver registry_observer(registry);
service->UnloadExtension(extension->id(), UnloadedExtensionReason::UNINSTALL);
registry_observer.WaitForExtensionUnloaded();

// Check that the opened chrome://history/ page contains the default chrome
// history page content.
ASSERT_EQ(1, tab_strip_model->count());
content::WaitForLoadStop(tab_strip_model->GetActiveWebContents());
EXPECT_EQ(
chrome::kChromeUIHistoryURL,
tab_strip_model->GetActiveWebContents()->GetLastCommittedURL().spec());

EXPECT_TRUE(content::ExecuteScriptAndExtractString(
tab_strip_model->GetActiveWebContents(),
"domAutomationController.send(location.href);\n", &location));
// Default history page has control.
EXPECT_EQ(chrome::kChromeUIHistoryURL, GURL(location).spec());
}

} // namespace extensions
16 changes: 15 additions & 1 deletion chrome/browser/ui/browser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2103,7 +2103,21 @@ void Browser::OnExtensionUnloaded(content::BrowserContext* browser_context,
web_contents->GetURL().host_piece() == extension->id()) ||
(extensions::TabHelper::FromWebContents(web_contents)
->extension_app() == extension)) {
tab_strip_model_->CloseWebContentsAt(i, TabStripModel::CLOSE_NONE);
if (tab_strip_model_->count() > 1) {
tab_strip_model_->CloseWebContentsAt(i, TabStripModel::CLOSE_NONE);
} else {
// If there is only 1 tab remaining, do not close it and instead
// navigate to the default NTP. Note that if there is an installed
// extension that overrides the NTP page, that extension's content
// will override the NTP contents.
GURL url(chrome::kChromeUINewTabURL);
web_contents->GetController().LoadURL(
url,
content::Referrer::SanitizeForRequest(
url,
content::Referrer(url, blink::kWebReferrerPolicyDefault)),
ui::PAGE_TRANSITION_RELOAD, std::string());
}
} else {
chrome::UnmuteIfMutedByExtension(web_contents, extension->id());
}
Expand Down
47 changes: 44 additions & 3 deletions chrome/browser/ui/browser_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "chrome/browser/devtools/devtools_window_testing.h"
#include "chrome/browser/extensions/extension_browsertest.h"
#include "chrome/browser/extensions/extension_service.h"
#include "chrome/browser/extensions/extension_tab_util.h"
#include "chrome/browser/extensions/extension_util.h"
#include "chrome/browser/extensions/tab_helper.h"
#include "chrome/browser/first_run/first_run.h"
Expand Down Expand Up @@ -106,6 +107,7 @@
#include "content/public/test/test_navigation_observer.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_system.h"
#include "extensions/browser/test_extension_registry_observer.h"
#include "extensions/browser/uninstall_reason.h"
#include "extensions/common/constants.h"
#include "extensions/common/extension.h"
Expand Down Expand Up @@ -1118,6 +1120,45 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, MAYBE_TabClosingWhenRemovingExtension) {
ASSERT_EQ(1, browser()->tab_strip_model()->count());
}

// Tests that when an extension is unloaded, if only one tab is opened
// containing extenions-related content, then the tab is kept open and is
// directed to the default NTP.
IN_PROC_BROWSER_TEST_F(BrowserTest, NavigateToDefaultNTPPageOnExtensionUnload) {
ASSERT_TRUE(embedded_test_server()->Start());
TabStripModel* tab_strip_model = browser()->tab_strip_model();

const Extension* extension =
LoadExtension(test_data_dir_.AppendASCII("options_page/"));
ASSERT_TRUE(extension);

GURL extension_url = extension->GetResourceURL("options.html");
ui_test_utils::NavigateToURL(browser(), extension_url);
content::WaitForLoadStop(tab_strip_model->GetActiveWebContents());

ASSERT_EQ(1, browser()->tab_strip_model()->count());
EXPECT_EQ(
extension_url.spec(),
tab_strip_model->GetActiveWebContents()->GetLastCommittedURL().spec());

// Uninstall the extension.
ExtensionService* service =
extensions::ExtensionSystem::Get(browser()->profile())
->extension_service();
extensions::ExtensionRegistry* registry =
extensions::ExtensionRegistry::Get(browser()->profile());
extensions::TestExtensionRegistryObserver registry_observer(registry);
service->UnloadExtension(extension->id(),
extensions::UnloadedExtensionReason::UNINSTALL);
registry_observer.WaitForExtensionUnloaded();
content::WaitForLoadStop(tab_strip_model->GetActiveWebContents());

// There should only be one tab now, with the NTP loaded.
ASSERT_EQ(1, tab_strip_model->count());
EXPECT_EQ(
chrome::kChromeUINewTabURL,
tab_strip_model->GetActiveWebContents()->GetLastCommittedURL().spec());
}

// Open with --app-id=<id>, and see that an application tab opens by default.
IN_PROC_BROWSER_TEST_F(BrowserTest, AppIdSwitch) {
ASSERT_TRUE(embedded_test_server()->Start());
Expand Down Expand Up @@ -2372,7 +2413,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, GetSizeForNewRenderView) {
web_contents->GetContainerBounds().size();
RenderViewSizeObserver observer(web_contents, browser()->window());

// Navigate to a non-NTP page, without resizing WebContentsView.
// Navigate to a non-NTP, without resizing WebContentsView.
ui_test_utils::NavigateToURL(browser(),
embedded_test_server()->GetURL("/title1.html"));
ASSERT_EQ(BookmarkBar::HIDDEN, browser()->bookmark_bar_state());
Expand Down Expand Up @@ -2410,7 +2451,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, GetSizeForNewRenderView) {
EXPECT_EQ(wcv_commit_size0, web_contents->GetContainerBounds().size());
#endif

// Navigate to another non-NTP page, without resizing WebContentsView.
// Navigate to another non-NTP, without resizing WebContentsView.
ui_test_utils::NavigateToURL(browser(),
https_test_server.GetURL("/title2.html"));
ASSERT_EQ(BookmarkBar::HIDDEN, browser()->bookmark_bar_state());
Expand All @@ -2426,7 +2467,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, GetSizeForNewRenderView) {
web_contents->GetRenderWidgetHostView()->GetViewBounds().size());
EXPECT_EQ(wcv_commit_size1, web_contents->GetContainerBounds().size());

// Navigate from NTP to a non-NTP page, resizing WebContentsView while
// Navigate from NTP to a non-NTP, resizing WebContentsView while
// navigation entry is pending.
ui_test_utils::NavigateToURL(browser(), GURL("chrome://newtab"));
gfx::Size wcv_resize_insets(1, 1);
Expand Down
1 change: 1 addition & 0 deletions chrome/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,7 @@ test("browser_tests") {
"../browser/extensions/extension_unload_browsertest.cc",
"../browser/extensions/extension_url_rewrite_browsertest.cc",
"../browser/extensions/extension_view_host_factory_browsertest.cc",
"../browser/extensions/extension_web_ui_browsertest.cc",
"../browser/extensions/extension_websocket_apitest.cc",
"../browser/extensions/extension_webui_apitest.cc",
"../browser/extensions/extension_with_management_policy_apitest.cc",
Expand Down

0 comments on commit 4d546d6

Please sign in to comment.