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) <[email protected]>
Commit-Queue: Sylvain Defresne <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#501944}(cherry picked from commit e8ae6ad)
Reviewed-on: https://chromium-review.googlesource.com/674983
Reviewed-by: Sylvain Defresne <[email protected]>
Cr-Commit-Position: refs/branch-heads/3202@{crosswalk-project#353}
Cr-Branched-From: fa6a5d8-refs/heads/master@{#499098}

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",
   ]

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;
 }
 

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);
 };

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);
-}

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_

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);
+}