From add2a72a91ab90d826bd12631719015f6850ae51 Mon Sep 17 00:00:00 2001 From: Sylvain Defresne <sdefresne@chromium.org> Date: Wed, 20 Sep 2017 08:43:21 +0000 Subject: [PATCH] Fix restoration of session after a crash. Disable sending kTabModelNewTabWillOpenNotification notification when the TabModel is restoring a session as this breaks the BVC state that does not expect the notification to be sent when a tab is added due to session restoration. This is a reland of http://crrev.com/c/664557 that fixes EG tests by correctly initialising TabModelNotificationObserver (should not be disabled except during the restoration of the session). This CL also improves on the original CL by using a scoped closure runner to re-enable TabModelNotificationObserver to protect against early returns in -restoreSessionWindow:persistState:. Bug: 763964 Change-Id: Ie950ac3f35b13566abcc1ca6eae774512ed7a16a Reviewed-on: https://chromium-review.googlesource.com/665238 Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org> Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#501944}(cherry picked from commit e8ae6ad6b282f367b21c5c6bae38a27e7e8336cc) Reviewed-on: https://chromium-review.googlesource.com/674983 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#353} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} --- ios/chrome/browser/tabs/BUILD.gn | 2 + ios/chrome/browser/tabs/tab_model.mm | 26 ++++++++++- .../tabs/tab_model_notification_observer.h | 8 ++-- .../tabs/tab_model_notification_observer.mm | 26 +++-------- .../tab_model_web_usage_enabled_observer.h | 34 ++++++++++++++ .../tab_model_web_usage_enabled_observer.mm | 46 +++++++++++++++++++ 6 files changed, 116 insertions(+), 26 deletions(-) create mode 100644 ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.h create mode 100644 ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.mm diff --git a/ios/chrome/browser/tabs/BUILD.gn b/ios/chrome/browser/tabs/BUILD.gn index b54a8f64d2760..fc0f8a257b89a 100644 --- a/ios/chrome/browser/tabs/BUILD.gn +++ b/ios/chrome/browser/tabs/BUILD.gn @@ -58,6 +58,8 @@ source_set("tabs_internal") { "tab_model_synced_window_delegate_getter.mm", "tab_model_web_state_list_delegate.h", "tab_model_web_state_list_delegate.mm", + "tab_model_web_usage_enabled_observer.h", + "tab_model_web_usage_enabled_observer.mm", "tab_parenting_observer.h", "tab_parenting_observer.mm", ] diff --git a/ios/chrome/browser/tabs/tab_model.mm b/ios/chrome/browser/tabs/tab_model.mm index 1702e3f50fc2d..26f0210693ca2 100644 --- a/ios/chrome/browser/tabs/tab_model.mm +++ b/ios/chrome/browser/tabs/tab_model.mm @@ -9,6 +9,7 @@ #include <vector> #include "base/bind.h" +#include "base/callback_helpers.h" #include "base/logging.h" #import "base/mac/foundation_util.h" #include "base/metrics/histogram_macros.h" @@ -42,6 +43,7 @@ #import "ios/chrome/browser/tabs/tab_model_selected_tab_observer.h" #import "ios/chrome/browser/tabs/tab_model_synced_window_delegate.h" #import "ios/chrome/browser/tabs/tab_model_web_state_list_delegate.h" +#import "ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.h" #import "ios/chrome/browser/tabs/tab_parenting_observer.h" #import "ios/chrome/browser/web/page_placeholder_tab_helper.h" #import "ios/chrome/browser/web_state_list/web_state_list.h" @@ -156,6 +158,9 @@ @interface TabModel () { // The delegate for sync. std::unique_ptr<TabModelSyncedWindowDelegate> _syncedWindowDelegate; + // The observer that sends kTabModelNewTabWillOpenNotification notifications. + TabModelNotificationObserver* _tabModelNotificationObserver; + // Counters for metrics. WebStateListMetricsObserver* _webStateListMetricsObserver; @@ -313,7 +318,12 @@ - (instancetype)initWithSessionWindow:(SessionWindowIOS*)window _webStateListObservers.push_back(std::move(webStateListMetricsObserver)); _webStateListObservers.push_back( - base::MakeUnique<TabModelNotificationObserver>(self)); + base::MakeUnique<TabModelWebUsageEnabledObserver>(self)); + + auto tabModelNotificationObserver = + base::MakeUnique<TabModelNotificationObserver>(self); + _tabModelNotificationObserver = tabModelNotificationObserver.get(); + _webStateListObservers.push_back(std::move(tabModelNotificationObserver)); for (const auto& webStateListObserver : _webStateListObservers) _webStateList->AddObserver(webStateListObserver.get()); @@ -569,7 +579,8 @@ - (void)browserStateDestroyed { UnregisterTabModelFromChromeBrowserState(_browserState, self); _browserState = nullptr; - // Clear weak pointer to WebStateListMetricsObserver before destroying it. + // Clear weak pointer to observers before destroying them. + _tabModelNotificationObserver = nullptr; _webStateListMetricsObserver = nullptr; // Close all tabs. Do this in an @autoreleasepool as WebStateList observers @@ -643,6 +654,16 @@ - (BOOL)restoreSessionWindow:(SessionWindowIOS*)window DCHECK(_browserState); DCHECK(window); + // Disable sending the kTabModelNewTabWillOpenNotification notification + // while restoring a session as it breaks the BVC (see crbug.com/763964). + base::ScopedClosureRunner enableTabModelNotificationObserver; + if (_tabModelNotificationObserver) { + _tabModelNotificationObserver->SetDisabled(true); + enableTabModelNotificationObserver.ReplaceClosure( + base::BindOnce(&TabModelNotificationObserver::SetDisabled, + base::Unretained(_tabModelNotificationObserver), false)); + } + if (!window.sessions.count) return NO; @@ -695,6 +716,7 @@ - (BOOL)restoreSessionWindow:(SessionWindowIOS*)window _tabUsageRecorder->InitialRestoredTabs(self.currentTab.webState, restoredWebStates); } + return closedNTPTab; } diff --git a/ios/chrome/browser/tabs/tab_model_notification_observer.h b/ios/chrome/browser/tabs/tab_model_notification_observer.h index d376d28d4da47..056548aaf1ab1 100644 --- a/ios/chrome/browser/tabs/tab_model_notification_observer.h +++ b/ios/chrome/browser/tabs/tab_model_notification_observer.h @@ -15,18 +15,18 @@ class TabModelNotificationObserver : public WebStateListObserver { explicit TabModelNotificationObserver(TabModel* tab_model); ~TabModelNotificationObserver() override; + // Controls whether sending notification is enabled or not. + void SetDisabled(bool disabled); + // WebStateListObserver implementation. void WebStateInsertedAt(WebStateList* web_state_list, web::WebState* web_state, int index, bool activating) override; - void WebStateReplacedAt(WebStateList* web_state_list, - web::WebState* old_web_state, - web::WebState* new_web_state, - int index) override; private: __weak TabModel* tab_model_; + bool disabled_ = false; DISALLOW_COPY_AND_ASSIGN(TabModelNotificationObserver); }; diff --git a/ios/chrome/browser/tabs/tab_model_notification_observer.mm b/ios/chrome/browser/tabs/tab_model_notification_observer.mm index a1b8f437c41e5..712178474b8b6 100644 --- a/ios/chrome/browser/tabs/tab_model_notification_observer.mm +++ b/ios/chrome/browser/tabs/tab_model_notification_observer.mm @@ -12,29 +12,22 @@ #error "This file requires ARC support." #endif -namespace { - -// Sets |web_state| web usage enabled property and starts loading the content -// if necessary. -void SetWebUsageEnabled(web::WebState* web_state, bool web_usage_enabled) { - web_state->SetWebUsageEnabled(web_usage_enabled); - if (web_usage_enabled) - web_state->GetNavigationManager()->LoadIfNecessary(); -} - -} // namespace - TabModelNotificationObserver::TabModelNotificationObserver(TabModel* tab_model) : tab_model_(tab_model) {} TabModelNotificationObserver::~TabModelNotificationObserver() = default; +void TabModelNotificationObserver::SetDisabled(bool disabled) { + disabled_ = disabled; +} + void TabModelNotificationObserver::WebStateInsertedAt( WebStateList* web_state_list, web::WebState* web_state, int index, bool activating) { - SetWebUsageEnabled(web_state, tab_model_.webUsageEnabled); + if (disabled_) + return; Tab* tab = LegacyTabHelper::GetTabForWebState(web_state); [[NSNotificationCenter defaultCenter] @@ -46,10 +39,3 @@ void SetWebUsageEnabled(web::WebState* web_state, bool web_usage_enabled) { }]; } -void TabModelNotificationObserver::WebStateReplacedAt( - WebStateList* web_state_list, - web::WebState* old_web_state, - web::WebState* new_web_state, - int index) { - SetWebUsageEnabled(new_web_state, tab_model_.webUsageEnabled); -} diff --git a/ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.h b/ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.h new file mode 100644 index 0000000000000..6c2aa32b679b1 --- /dev/null +++ b/ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.h @@ -0,0 +1,34 @@ +// Copyright 2017 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. + +#ifndef IOS_CHROME_BROWSER_TABS_TAB_MODEL_WEB_USAGE_ENABLED_OBSERVER_H_ +#define IOS_CHROME_BROWSER_TABS_TAB_MODEL_WEB_USAGE_ENABLED_OBSERVER_H_ + +#include "base/macros.h" +#import "ios/chrome/browser/web_state_list/web_state_list_observer.h" + +@class TabModel; + +class TabModelWebUsageEnabledObserver : public WebStateListObserver { + public: + explicit TabModelWebUsageEnabledObserver(TabModel* tab_model); + ~TabModelWebUsageEnabledObserver() override; + + // WebStateListObserver implementation. + void WebStateInsertedAt(WebStateList* web_state_list, + web::WebState* web_state, + int index, + bool activating) override; + void WebStateReplacedAt(WebStateList* web_state_list, + web::WebState* old_web_state, + web::WebState* new_web_state, + int index) override; + + private: + __weak TabModel* tab_model_; + + DISALLOW_COPY_AND_ASSIGN(TabModelWebUsageEnabledObserver); +}; + +#endif // IOS_CHROME_BROWSER_TABS_TAB_MODEL_WEB_USAGE_ENABLED_OBSERVER_H_ diff --git a/ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.mm b/ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.mm new file mode 100644 index 0000000000000..30fda0b10cf20 --- /dev/null +++ b/ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.mm @@ -0,0 +1,46 @@ +// Copyright 2017 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. + +#import "ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.h" + +#import "ios/chrome/browser/tabs/tab_model.h" +#import "ios/web/public/web_state/web_state.h" + +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + +namespace { + +// Sets |web_state| web usage enabled property and starts loading the content +// if necessary. +void SetWebUsageEnabled(web::WebState* web_state, bool web_usage_enabled) { + web_state->SetWebUsageEnabled(web_usage_enabled); + if (web_usage_enabled) + web_state->GetNavigationManager()->LoadIfNecessary(); +} + +} // namespace + +TabModelWebUsageEnabledObserver::TabModelWebUsageEnabledObserver( + TabModel* tab_model) + : tab_model_(tab_model) {} + +TabModelWebUsageEnabledObserver::~TabModelWebUsageEnabledObserver() = default; + +void TabModelWebUsageEnabledObserver::WebStateInsertedAt( + WebStateList* web_state_list, + web::WebState* web_state, + int index, + bool activating) { + SetWebUsageEnabled(web_state, tab_model_.webUsageEnabled); +} + +void TabModelWebUsageEnabledObserver::WebStateReplacedAt( + WebStateList* web_state_list, + web::WebState* old_web_state, + web::WebState* new_web_state, + int index) { + SetWebUsageEnabled(new_web_state, tab_model_.webUsageEnabled); +}