Skip to content

Commit

Permalink
Revert "Suppress beforeunload and in-progress download prompts for fo…
Browse files Browse the repository at this point in the history 
…rced restarts."

This reverts commit 2c98cab.

Reason for revert: Parent change causing crashes.

Original change's description:
> Suppress beforeunload and in-progress download prompts for forced restarts.
>
> This change introduces RelaunchIgnoreUnloadHandlers and uses it in
> RelaunchNotificationController when the deadline for a forced relaunch
> is reached. This new function is to AttemptRelaunch as
> ExitIgnoreUnloadHandlers is to AttemptExit.
>
> This also introduces browser_shutdown::ShouldIgnoreUnloadHandlers(),
> which is a convenience for code that needs to know whether or not an
> in-progress shutdown should ignore unload handlers.
>
> BUG=958865
>
> Change-Id: I91a9832d745e95b7436b176d78f5e39ed14addd2
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1914380
> Commit-Queue: Greg Thompson <grt@chromium.org>
> Auto-Submit: Greg Thompson <grt@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#715901}

TBR=treib@chromium.org,grt@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

(cherry picked from commit b053b57)

Bug: 958865,1025638,1025634
Change-Id: I0584904da38d150481e8450aacaff0f909a53d12
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1919356
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#716137}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1922209
Cr-Commit-Position: refs/branch-heads/3970@{#6}
Cr-Branched-From: 55f4451-refs/heads/master@{#716057}
  • Loading branch information
@GregTho
GregTho committed Nov 18, 2019
1 parent ef8b79d commit 76bd135
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 149 deletions.
74 changes: 22 additions & 52 deletions chrome/browser/lifetime/application_lifetime.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "base/process/process.h"
#include "base/process/process_handle.h"
#include "base/task/post_task.h"
#include "base/util/type_safety/strong_alias.h"
#include "build/build_config.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/browser_process_platform_part.h"
Expand Down Expand Up @@ -121,38 +120,6 @@ bool SetLocaleForNextStart(PrefService* local_state) {
bool g_send_stop_request_to_session_manager = false;
#endif

#if !defined(OS_ANDROID)
using IgnoreUnloadHandlers =
util::StrongAlias<class IgnoreUnloadHandlersTag, bool>;

void AttemptRestartInternal(IgnoreUnloadHandlers ignore_unload_handlers) {
// TODO(beng): Can this use ProfileManager::GetLoadedProfiles instead?
for (auto* browser : *BrowserList::GetInstance())
content::BrowserContext::SaveSessionState(browser->profile());

PrefService* pref_service = g_browser_process->local_state();
pref_service->SetBoolean(prefs::kWasRestarted, true);

#if defined(OS_CHROMEOS)
chromeos::BootTimesRecorder::Get()->set_restart_requested();

DCHECK(!g_send_stop_request_to_session_manager);
// Make sure we don't send stop request to the session manager.
g_send_stop_request_to_session_manager = false;
// Run exit process in clean stack.
base::PostTask(FROM_HERE, {content::BrowserThread::UI},
base::BindOnce(&ExitIgnoreUnloadHandlers));
#else
// Set the flag to restore state after the restart.
pref_service->SetBoolean(prefs::kRestartLastSessionOnShutdown, true);
if (ignore_unload_handlers)
ExitIgnoreUnloadHandlers();
else
AttemptExit();
#endif
}
#endif // !defined(OS_ANDROID)

} // namespace

#if !defined(OS_ANDROID)
Expand Down Expand Up @@ -257,29 +224,38 @@ void AttemptUserExit() {
// The Android implementation is in application_lifetime_android.cc
#if !defined(OS_ANDROID)
void AttemptRestart() {
AttemptRestartInternal(IgnoreUnloadHandlers(false));
}
#endif // !defined(OS_ANDROID)
// TODO(beng): Can this use ProfileManager::GetLoadedProfiles instead?
for (auto* browser : *BrowserList::GetInstance())
content::BrowserContext::SaveSessionState(browser->profile());

PrefService* pref_service = g_browser_process->local_state();
pref_service->SetBoolean(prefs::kWasRestarted, true);

void AttemptRelaunch() {
#if defined(OS_CHROMEOS)
chromeos::PowerManagerClient::Get()->RequestRestart(
power_manager::REQUEST_RESTART_OTHER, "Chrome relaunch");
// If running the Chrome OS build, but we're not on the device, fall through.
chromeos::BootTimesRecorder::Get()->set_restart_requested();

DCHECK(!g_send_stop_request_to_session_manager);
// Make sure we don't send stop request to the session manager.
g_send_stop_request_to_session_manager = false;
// Run exit process in clean stack.
base::PostTask(FROM_HERE, {content::BrowserThread::UI},
base::BindOnce(&ExitIgnoreUnloadHandlers));
#else
// Set the flag to restore state after the restart.
pref_service->SetBoolean(prefs::kRestartLastSessionOnShutdown, true);
AttemptExit();
#endif
AttemptRestart();
}
#endif // !defined(OS_ANDROID)

#if !defined(OS_ANDROID)
void RelaunchIgnoreUnloadHandlers() {
void AttemptRelaunch() {
#if defined(OS_CHROMEOS)
chromeos::PowerManagerClient::Get()->RequestRestart(
power_manager::REQUEST_RESTART_OTHER, "Chrome relaunch");
// If running the Chrome OS build, but we're not on the device, fall through.
#endif
AttemptRestartInternal(IgnoreUnloadHandlers(true));
AttemptRestart();
}
#endif

void AttemptExit() {
#if defined(OS_CHROMEOS)
Expand All @@ -305,20 +281,14 @@ void ExitIgnoreUnloadHandlers() {
// We always mark exit cleanly.
MarkAsCleanShutdown();

#if defined(OS_CHROMEOS)
// On ChromeOS ExitIgnoreUnloadHandlers() is used to handle SIGTERM.
// In this case, AreAllBrowsersCloseable()
// can be false in following cases. a) power-off b) signout from
// screen locker.
browser_shutdown::OnShutdownStarting(
AreAllBrowsersCloseable() ? browser_shutdown::ShutdownType::kBrowserExit
: browser_shutdown::ShutdownType::kEndSession);
#else // defined(OS_CHROMEOS)
// For desktop browsers, always perform a silent exit.
browser_shutdown::OnShutdownStarting(
browser_shutdown::ShutdownType::kSilentExit);
#endif // defined(OS_CHROMEOS)
#endif // !defined(OS_ANDROID)
#endif
AttemptExitInternal(true);
}

Expand Down
15 changes: 3 additions & 12 deletions chrome/browser/lifetime/application_lifetime.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,11 @@ void AttemptUserExit();
// manager re-launch the browser with restore last session flag.
void AttemptRestart();

// Starts a user initiated relaunch process. On platforms other than Chrome OS,
// this is equivalent to AttemptRestart. On Chrome OS, this relaunches the
// entire OS, instead of just relaunching the browser.
// Starts a user initiated relaunch process. On platforms other than
// chromeos, this is equivalent to AttemptRestart. On ChromeOS, this relaunches
// the entire OS, instead of just relaunching the browser.
void AttemptRelaunch();

#if !defined(OS_ANDROID)
// Starts an administrator-initiated relaunch process. On platforms other than
// Chrome OS, this relaunches the browser and restores the user's session. On
// Chrome OS, this restarts the entire OS. This differs from AttemptRelaunch in
// that all user prompts (e.g., beforeunload handlers and confirmation to abort
// in-progress downloads) are bypassed.
void RelaunchIgnoreUnloadHandlers();
#endif

// Attempt to exit by closing all browsers. This is equivalent to
// CloseAllBrowsers() on platforms where the application exits
// when no more windows are remaining. On other platforms (the Mac),
Expand Down
84 changes: 26 additions & 58 deletions chrome/browser/lifetime/application_lifetime_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,14 @@
#include "chrome/browser/lifetime/application_lifetime.h"

#include "base/command_line.h"
#include "base/task/post_task.h"
#include "base/test/mock_callback.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/chrome_browser_main.h"
#include "chrome/browser/chrome_browser_main_extra_parts.h"
#include "chrome/browser/first_run/scoped_relaunch_chrome_browser_override.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/common/pref_names.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test_utils.h"
#include "content/public/test/test_utils.h"
#include "testing/gtest/include/gtest/gtest.h"

Expand All @@ -33,20 +26,22 @@ MATCHER_P(HasSwitch, switch_name, "") {

} // namespace

// Test Fixture for testing chrome::AttemptRestart method.
//
// The SetupCommandLine method takes care of initiating the browser with the
// incognito or guest switch. This switch is accessed via GetParam() method and
// the switches are passed during the testcase declaration which invokes the
// same test once per switch.
//
// The SetUpInProcessBrowserTestFixture method, creates a mock callback which
// gets hooked when chrome tries to Relaunch the browser. The hooking is thanks
// to the upgrade_util::ScopedRelaunchChromeBrowserOverride. Our mock callback
// runs the MATCHER_P.
//
// It is template parameterized on the type of switches::kIncognito and
// switches::kGuest which is const char*
/**
* Test Fixture for testing chrome::AttemptRestart method.
*
* The SetupCommandLine method takes care of initiating the browser with the
* incognito or guest switch. This switch is accessed via GetParam() method and
* the switches are passed during the testcase declaration which invokes the
* same test once per switch.
*
* The SetUpInProcessBrowserTestFixture method, creates a mock callback which
* gets hooked when chrome tries to Relaunch the browser. The hooking is thanks
* to the upgrade_util::ScopedRelaunchChromeBrowserOverride. Our mock callback
* runs the MATCHER_P.
*
* It is template parameterized on the type of switches::kIncognito and
* switches::kGuest which is const char*
*/
class AttemptRestartTest : public InProcessBrowserTest,
public testing::WithParamInterface<const char*> {
protected:
Expand All @@ -59,15 +54,21 @@ class AttemptRestartTest : public InProcessBrowserTest,

void SetUpInProcessBrowserTestFixture() override {
// Expect a browser relaunch late in browser shutdown.
EXPECT_CALL(mock_relaunch_callback_,
mock_relaunch_callback_ = std::make_unique<::testing::StrictMock<
base::MockCallback<upgrade_util::RelaunchChromeBrowserCallback>>>();
EXPECT_CALL(*mock_relaunch_callback_,
Run(testing::Not(HasSwitch(GetParam()))));
relaunch_chrome_override_ =
std::make_unique<upgrade_util::ScopedRelaunchChromeBrowserOverride>(
mock_relaunch_callback_->Get());
}

private:
base::MockCallback<upgrade_util::RelaunchChromeBrowserCallback>
std::unique_ptr<
base::MockCallback<upgrade_util::RelaunchChromeBrowserCallback>>
mock_relaunch_callback_;
upgrade_util::ScopedRelaunchChromeBrowserOverride relaunch_chrome_override_{
mock_relaunch_callback_.Get()};
std::unique_ptr<upgrade_util::ScopedRelaunchChromeBrowserOverride>
relaunch_chrome_override_;
};

INSTANTIATE_TEST_CASE_P(,
Expand All @@ -81,36 +82,3 @@ IN_PROC_BROWSER_TEST_P(AttemptRestartTest, AttemptRestartWithOTRProfiles) {
// of the last session. Now, we will restart always to regular mode.
chrome::AttemptRestart();
}

// Ensures that a relaunch actually happens without interaction when a
// beforeunload handler is in place when RelaunchIgnoreUnloadHandlers is called.
class RelaunchIgnoreUnloadHandlersTest : public InProcessBrowserTest {
protected:
RelaunchIgnoreUnloadHandlersTest() = default;

// InProcessBrowserTest:
void SetUpInProcessBrowserTestFixture() override {
// Expect a browser relaunch late in browser shutdown.
EXPECT_CALL(mock_relaunch_callback_, Run(::testing::_));
}

private:
base::MockCallback<upgrade_util::RelaunchChromeBrowserCallback>
mock_relaunch_callback_;
upgrade_util::ScopedRelaunchChromeBrowserOverride relaunch_chrome_override_{
mock_relaunch_callback_.Get()};
};

IN_PROC_BROWSER_TEST_F(RelaunchIgnoreUnloadHandlersTest, Do) {
ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
content::WebContents* tab =
browser()->tab_strip_model()->GetActiveWebContents();
ASSERT_TRUE(content::ExecuteScript(
tab,
"window.addEventListener('beforeunload',"
"function(event) { event.returnValue = 'Foo'; });"));
content::PrepContentsForBeforeUnloadTest(tab);
base::PostTask(FROM_HERE, {content::BrowserThread::UI},
base::BindOnce(&chrome::RelaunchIgnoreUnloadHandlers));
ui_test_utils::WaitForBrowserToClose(browser());
}
12 changes: 7 additions & 5 deletions chrome/browser/lifetime/browser_close_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,10 @@ BrowserCloseManager::~BrowserCloseManager() {
}

void BrowserCloseManager::StartClosingBrowsers() {
// If the session is ending or a silent exit was requested, skip straight to
// closing the browsers without waiting for beforeunload dialogs.
if (browser_shutdown::ShouldIgnoreUnloadHandlers()) {
// If the session is ending, skip straight to closing the browsers. There's no
// time to wait for beforeunload dialogs.
if (browser_shutdown::GetShutdownType() ==
browser_shutdown::ShutdownType::kEndSession) {
// Tell everyone that we are shutting down.
browser_shutdown::SetTryingToQuit(true);
CloseBrowsers();
Expand Down Expand Up @@ -162,11 +163,12 @@ void BrowserCloseManager::CloseBrowsers() {
BrowserList::GetInstance()->end(),
std::back_inserter(browser_list_copy));

bool ignore_unload_handlers = browser_shutdown::ShouldIgnoreUnloadHandlers();
bool session_ending = browser_shutdown::GetShutdownType() ==
browser_shutdown::ShutdownType::kEndSession;

for (auto* browser : browser_list_copy) {
browser->window()->Close();
if (ignore_unload_handlers) {
if (session_ending) {
// This path is hit during logoff/power-down. In this case we won't get
// a final message and so we force the browser to be deleted.
// Close doesn't immediately destroy the browser
Expand Down
10 changes: 0 additions & 10 deletions chrome/browser/lifetime/browser_shutdown.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,6 @@ const char* ToShutdownTypeString(ShutdownType type) {
return "exit";
case ShutdownType::kEndSession:
return "end";
case ShutdownType::kSilentExit:
return "silent_exit";
}
return "";
}
Expand Down Expand Up @@ -148,11 +146,6 @@ bool HasShutdownStarted() {
return g_shutdown_type != ShutdownType::kNotValid;
}

bool ShouldIgnoreUnloadHandlers() {
return g_shutdown_type == ShutdownType::kEndSession ||
g_shutdown_type == ShutdownType::kSilentExit;
}

ShutdownType GetShutdownType() {
return g_shutdown_type;
}
Expand Down Expand Up @@ -320,9 +313,6 @@ void ReadLastShutdownFile(ShutdownType type,

switch (type) {
case ShutdownType::kNotValid:
case ShutdownType::kSilentExit:
// The histograms below have expired, so do not record metrics for silent
// exits; see https://crbug.com/975118.
break;

case ShutdownType::kWindowClose:
Expand Down
9 changes: 1 addition & 8 deletions chrome/browser/lifetime/browser_shutdown.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ enum class ShutdownType {
kBrowserExit = 2,
// User logoff or system shutdown.
kEndSession = 3,
// Exit without onbeforeunload or in-progress download prompts.
kSilentExit = 4,
kMaxValue = kSilentExit
kMaxValue = kEndSession
};

void RegisterPrefs(PrefRegistrySimple* registry);
Expand All @@ -67,11 +65,6 @@ void OnShutdownStarting(ShutdownType type);
// started.
bool HasShutdownStarted();

// Returns true if OnShutdownStarting has been called and unload handlers (e.g.,
// an in-progress download or a page's beforeunload handler) should be ignored.
// This is true for kEndSession and kSilentExit shutdown types.
bool ShouldIgnoreUnloadHandlers();

// Get the current shutdown type.
ShutdownType GetShutdownType();

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/views/relaunch_notification/relaunch_notification_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,5 +305,5 @@ void RelaunchNotificationController::Close() {
}

void RelaunchNotificationController::OnRelaunchDeadlineExpired() {
chrome::RelaunchIgnoreUnloadHandlers();
chrome::AttemptRelaunch();
}
3 changes: 0 additions & 3 deletions tools/metrics/histograms/enums.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57056,9 +57056,6 @@ Called by update_net_trust_anchors.py.-->
<int value="1" label="Window close">The last browser window was closed.</int>
<int value="2" label="Browser exit">User clicked on the Exit menu item.</int>
<int value="3" label="End session">OS is logging off or shutting down.</int>
<int value="4" label="Silent exit relaunch">
Exit without onbeforeunload or in-progress download prompts.
</int>
</enum>

<enum name="SideloadUIEvents">
Expand Down

0 comments on commit 76bd135

Please sign in to comment.