Skip to content

Commit

Permalink
Revert "sessions: change logic of when ExitTypeService marks session …
Browse files Browse the repository at this point in the history 
…as clean"

This reverts commit 51a209b.

Reason for revert: Added test fails on mac builds

Original change's description:
> sessions: change logic of when ExitTypeService marks session as clean
>
> This does two things:
> . After a crash, ExitTypeService will not mark the current
>   session as clean unless the user closes the crash bubble, or
>   creates another browser. This means the user may be prompted to
>   restore after a crash more than once.
> . After a crash, sessionservice will not write data until the user
>   closes the bubble, or creates a new browser.
>
> Both of these should make it so that after a crash the user has more
> opporunities to restore what is perceived as data from the last crash.
>
> BUG=294444
> TEST=ExitTypeServiceTests
>
> Change-Id: Ib71a298e7d5539865e22e0c2681801205905f762
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3215073
> Commit-Queue: Scott Violet <sky@chromium.org>
> Reviewed-by: David Bienvenu <davidbienvenu@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#931779}

Bug: 294444, 1260236
Change-Id: Ibc300203ea748d9c6099e3ad4bb89097557f2485
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3225263
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Rakina Zata Amni <rakina@google.com>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#931848}
  • Loading branch information
@rakina
rakina authored and Chromium LUCI CQ committed Oct 15, 2021
1 parent 37da24e commit 3466077
Show file tree
Hide file tree
Showing 15 changed files with 39 additions and 678 deletions.
8 changes: 0 additions & 8 deletions chrome/browser/content_settings/host_content_settings_map_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@
#include "chrome/browser/notifications/notification_channels_provider_android.h"
#endif // OS_ANDROID

#if BUILDFLAG(ENABLE_SESSION_SERVICE)
#include "chrome/browser/sessions/exit_type_service_factory.h"
#endif

HostContentSettingsMapFactory::HostContentSettingsMapFactory()
: RefcountedBrowserContextKeyedServiceFactory(
"HostContentSettingsMap",
Expand All @@ -56,10 +52,6 @@ HostContentSettingsMapFactory::HostContentSettingsMapFactory()
#endif
#if BUILDFLAG(ENABLE_EXTENSIONS)
DependsOn(extensions::ContentSettingsService::GetFactoryInstance());
#endif
// Used by way of ShouldRestoreOldSessionCookies().
#if BUILDFLAG(ENABLE_SESSION_SERVICE)
DependsOn(ExitTypeServiceFactory::GetInstance());
#endif
}

Expand Down
7 changes: 0 additions & 7 deletions chrome/browser/sessions/DEPS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,3 @@ include_rules = [
# For compressing data stored in SessionStorage.
"+third_party/bzip2",
]

specific_include_rules = {
'.*test\.cc': [
# Tests may depend upon views.
"+chrome/browser/ui/views",
],
}
184 changes: 8 additions & 176 deletions chrome/browser/sessions/exit_type_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,9 @@

#include "chrome/browser/sessions/exit_type_service.h"

#include "base/containers/flat_set.h"
#include "base/memory/ptr_util.h"
#include "chrome/browser/lifetime/browser_shutdown.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/sessions/exit_type_service_factory.h"
#include "chrome/browser/sessions/session_restore.h"
#include "chrome/browser/sessions/session_service.h"
#include "chrome/browser/sessions/session_service_utils.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_list.h"
#include "chrome/browser/ui/browser_list_observer.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "chrome/common/pref_names.h"
#include "components/prefs/pref_service.h"

Expand Down Expand Up @@ -51,79 +40,8 @@ std::string ExitTypeToSessionTypePrefValue(ExitType type) {

} // namespace

// Responsible for notifying ExitTypeService when a browser or tab is created
// under the right circumstances.
class ExitTypeService::BrowserTabObserverImpl : public BrowserListObserver,
public TabStripModelObserver {
public:
explicit BrowserTabObserverImpl(ExitTypeService* service)
: service_(service) {
BrowserList::GetInstance()->AddObserver(this);
}

~BrowserTabObserverImpl() override {
BrowserList::GetInstance()->RemoveObserver(this);
for (Browser* browser : browsers_)
browser->tab_strip_model()->RemoveObserver(this);
}

// BrowserListObserver:
void OnBrowserAdded(Browser* browser) override {
if (browser->profile() != service_->profile() ||
browser->omit_from_session_restore() ||
!SessionService::IsRelevantWindowType(
WindowTypeForBrowserType(browser->type()))) {
return;
}
if (browser->create_params().creation_source !=
Browser::CreationSource::kStartupCreator) {
// Ideally this would call directly to `service_`, but at the time this
// is called it is too early to do that. So, this waits for the first tab
// to be added.
// be added.
browsers_.insert(browser);
browser->tab_strip_model()->AddObserver(this);
}
}

void OnBrowserRemoved(Browser* browser) override {
auto iter = browsers_.find(browser);
if (iter == browsers_.end())
return;
browser->tab_strip_model()->RemoveObserver(this);
browsers_.erase(iter);
}

// TabStripModelObserver:
void OnTabStripModelChanged(
TabStripModel* tab_strip_model,
const TabStripModelChange& change,
const TabStripSelectionChange& selection) override {
if (change.type() == TabStripModelChange::kInserted)
service_->OnTabAddedToNonStartupBrowser();
}

private:
ExitTypeService* service_;

// Browsers whose TabStripModel this is observing.
base::flat_set<Browser*> browsers_;
};

ExitTypeService::CrashedLock::~CrashedLock() {
service_->CrashedLockDestroyed();
}

ExitTypeService::CrashedLock::CrashedLock(ExitTypeService* service)
: service_(service) {}

ExitTypeService::~ExitTypeService() {
// If the user did not takesome action that would constitute a new session
// (such as closing the crash bubble, or creating a new browser), do not
// reset the crash status. This way the user will have another chance to
// restore when chrome starts again.
if (!waiting_for_user_to_ack_crash_)
SetCurrentSessionExitType(ExitType::kClean);
SetCurrentSessionExitType(ExitType::kClean);
}

// static
Expand All @@ -133,23 +51,17 @@ ExitTypeService* ExitTypeService::GetInstanceForProfile(Profile* profile) {

// static
ExitType ExitTypeService::GetLastSessionExitType(Profile* profile) {
ExitTypeService* exit_type_service =
ExitTypeService* tracker =
GetInstanceForProfile(profile->GetOriginalProfile());
// `exit_type_service` may be null for certain profile types (such as signin
// profile on chromeos).
return exit_type_service ? exit_type_service->last_session_exit_type()
: ExitType::kClean;
// `tracker` may be null for certain profile types (such as signin profile
// on chromeos).
return tracker ? tracker->last_session_exit_type() : ExitType::kClean;
}

void ExitTypeService::SetCurrentSessionExitType(ExitType exit_type) {
if (waiting_for_user_to_ack_crash_) {
if (!exit_type_to_apply_on_ack_.has_value())
exit_type_to_apply_on_ack_ = exit_type;
return;
}

// This may be invoked multiple times during shutdown. Only persist the value
// first passed in.
// first passed in (unless it's a reset to the crash state, which happens when
// foregrounding the app on mobile).
if (exit_type == ExitType::kCrashed ||
current_session_exit_type_ == ExitType::kCrashed) {
current_session_exit_type_ = exit_type;
Expand All @@ -158,92 +70,12 @@ void ExitTypeService::SetCurrentSessionExitType(ExitType exit_type) {
}
}

std::unique_ptr<ExitTypeService::CrashedLock>
ExitTypeService::CreateCrashedLock() {
if (!waiting_for_user_to_ack_crash_)
return nullptr;

++crashed_lock_count_;
// Uses WrapUnique() as constructor is private.
return base::WrapUnique(new CrashedLock(this));
}

void ExitTypeService::AddCrashAckCallback(base::OnceClosure callback) {
// `waiting_for_user_to_ack_crash_` never goes from false to true. So any
// callback added when false can be ignored.
if (!waiting_for_user_to_ack_crash_)
return;
crash_ack_callbacks_.push_back(std::move(callback));
}

ExitTypeService::ExitTypeService(Profile* profile)
: profile_(profile),
last_session_exit_type_(SessionTypePrefValueToExitType(
profile_->GetPrefs()->GetString(prefs::kSessionExitType))),
current_session_exit_type_(ExitType::kCrashed),
waiting_for_user_to_ack_crash_(last_session_exit_type_ ==
ExitType::kCrashed) {
current_session_exit_type_(ExitType::kCrashed) {
// Mark the session as open.
profile_->GetPrefs()->SetString(prefs::kSessionExitType,
kPrefExitTypeCrashed);
if (waiting_for_user_to_ack_crash_)
browser_tab_observer_ = std::make_unique<BrowserTabObserverImpl>(this);
}

void ExitTypeService::CheckUserAckedCrash() {
// Once shutdown has started the state this checks may be torn down. During
// tear down we shouldn't run the callbacks, as the user did not really ack
// the crash.
if (browser_shutdown::HasShutdownStarted())
return;

if (!DidUserAckCrash())
return;

if (!IsWaitingForRestore() && SessionRestore::IsRestoring(profile_) &&
!did_restore_complete_) {
// Wait for restore to finish before enabling. This way if there is a crash
// before the actual restore, the last session isn't lost.
restore_subscription_ =
SessionRestore::RegisterOnSessionRestoredCallback(base::BindRepeating(
&ExitTypeService::OnSessionRestoreDone, base::Unretained(this)));
return;
}

waiting_for_user_to_ack_crash_ = false;

if (exit_type_to_apply_on_ack_.has_value())
SetCurrentSessionExitType(*exit_type_to_apply_on_ack_);

std::vector<base::OnceClosure> callbacks;
std::swap(callbacks, crash_ack_callbacks_);
for (auto& callback : callbacks)
std::move(callback).Run();
}

void ExitTypeService::CrashedLockDestroyed() {
--crashed_lock_count_;
DCHECK_GE(crashed_lock_count_, 0);
if (crashed_lock_count_ == 0)
CheckUserAckedCrash();
}

void ExitTypeService::OnTabAddedToNonStartupBrowser() {
browser_tab_observer_.reset();
CheckUserAckedCrash();
}

bool ExitTypeService::DidUserAckCrash() const {
// See class description for details on this.
return waiting_for_user_to_ack_crash_ && !IsWaitingForRestore() &&
(!IsWaitingForBrowser() || (crashed_lock_count_ == 0));
}

void ExitTypeService::OnSessionRestoreDone(Profile* profile,
int tabs_restored) {
if (profile != profile_)
return;
did_restore_complete_ = true;
restore_subscription_ = {};
CheckUserAckedCrash();
}
Loading

0 comments on commit 3466077

Please sign in to comment.