[iOS] Consolidate SceneController observation ownership.

This is a speculative bug fix; I wasn't able to repro the issue
on trunk. It's a shutdown crash where the SceneController web state
list observer method is triggered by TabModel closing all open tabs,
while the interface provider isn't in a consistent state.

In general, having BrowserViewWrangler be responsible for setting an
observer on the web state lists in the browsers it creates seems
problematic, since it doesn't know the purpose of the observation being
set.

Since SceneController is the one who cares about this observation, it
(with this CL) becomes responsible for adding/removing itself as an
observer. The specific fix for this bug is for SceneController (which
observes the web state list so it can handle the last tab closing) to
stop observing before shutting down BrowserViewWrangler.

I wasn't able to repro the crash, so I couldn't verify the fix, but
I could verify that with this fix in place, a force-quit in the
simulator didn't cause any crashes.

I also updated the comments for BrowserInterfaceProvider to not say
some things that aren't true anymore.

Bug: 1062241
Change-Id: I0d4b69f5889e141a243754c61450f77ea39ac523
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2128068Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Commit-Queue: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754899}

ios/chrome/browser/ui/main/browser_interface_provider.h

@@ -25,8 +25,6 @@ class ChromeBrowserState;
 // interface; this protocol allows for easy encapsulation of that process.
 // For legacy reasons, the primary UI entry point for an interface is a visible
 // tab.
-// A given interface is scoped (currently) to a browser state; thus there can
-// be two interfaces available (a one incognito and the other not).
 @protocol BrowserInterface
 
 // The view controller showing the current tab for this interface. This property
@@ -67,9 +65,7 @@ class ChromeBrowserState;
 // |currentInterface| is an implementation decision, but |mainInterface| is
 // typical.
 // Changing this value may or may not trigger actual UI changes, or may just be
-// bookkeeping associated with UI changes handled elsewhere. The only invariant
-// is that |currentInterface.current| must be YES, and the |current| value of
-// any other interface must be NO.
+// bookkeeping associated with UI changes handled elsewhere.
 @property(nonatomic, weak) id<BrowserInterface> currentInterface;
 // The "main" (meaning non-incognito -- the nomenclature is legacy) interface.
 // This interface's |incognito| property is expected to be NO.

ios/chrome/browser/ui/main/browser_view_wrangler.h

@@ -12,10 +12,8 @@
 
 @protocol ApplicationCommands;
 class AppUrlLoadingService;
-@class BrowserCoordinator;
 @protocol BrowsingDataCommands;
 class ChromeBrowserState;
-@protocol WebStateListObserving;
 
 namespace {
 
@@ -38,7 +36,6 @@ NSString* kIncognitoCurrentKey = @"IncognitoActive";
 // storage associated with the interfaces when the current interface changes;
 // this is handled in the implementation of -setCurrentInterface:.
 - (instancetype)initWithBrowserState:(ChromeBrowserState*)browserState
-                webStateListObserver:(id<WebStateListObserving>)observer
           applicationCommandEndpoint:
               (id<ApplicationCommands>)applicationCommandEndpoint
          browsingDataCommandEndpoint:

ios/chrome/browser/ui/main/browser_view_wrangler.mm

@@ -26,10 +26,6 @@
 #import "ios/chrome/browser/ui/commands/browsing_data_commands.h"
 #import "ios/chrome/browser/ui/commands/command_dispatcher.h"
 #import "ios/chrome/browser/url_loading/app_url_loading_service.h"
-#import "ios/chrome/browser/web_state_list/web_state_list.h"
-#import "ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h"
-#import "ios/web/public/web_state.h"
-#import "ios/web/public/web_state_observer_bridge.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
@@ -107,7 +103,6 @@
 
   std::unique_ptr<Browser> _mainBrowser;
   std::unique_ptr<Browser> _otrBrowser;
-  std::unique_ptr<WebStateListObserverBridge> _webStateListForwardingObserver;
 }
 
 @property(nonatomic, strong, readwrite) WrangledBrowser* mainInterface;
@@ -125,9 +120,6 @@
 // saved for the |browser| will be loaded.
 - (void)restoreSessionToBrowser:(Browser*)browser;
 
-// Add the common observers to the browser's |webStateList|.
-- (void)addObserversToWebStateList:(WebStateList*)webStateList;
-
 // Setters for the main and otr Browsers.
 - (void)setMainBrowser:(std::unique_ptr<Browser>)browser;
 - (void)setOtrBrowser:(std::unique_ptr<Browser>)browser;
@@ -146,7 +138,6 @@
 @synthesize currentInterface = _currentInterface;
 
 - (instancetype)initWithBrowserState:(ChromeBrowserState*)browserState
-                webStateListObserver:(id<WebStateListObserving>)observer
           applicationCommandEndpoint:
               (id<ApplicationCommands>)applicationCommandEndpoint
          browsingDataCommandEndpoint:
@@ -158,8 +149,6 @@
     _applicationCommandEndpoint = applicationCommandEndpoint;
     _browsingDataCommandEndpoint = browsingDataCommandEndpoint;
     _appURLLoadingService = appURLLoadingService;
-    _webStateListForwardingObserver =
-        std::make_unique<WebStateListObserverBridge>(observer);
   }
   return self;
 }
@@ -175,8 +164,7 @@
   browserList->AddBrowser(_mainBrowser.get());
   [self dispatchToEndpointsForBrowser:_mainBrowser.get()];
   [self restoreSessionToBrowser:_mainBrowser.get()];
-  [self addObserversToWebStateList:_mainBrowser->GetWebStateList()];
-
+  breakpad::MonitorTabStateForWebStateList(_mainBrowser->GetWebStateList());
   // Follow loaded URLs in the main tab model to send those in case of
   // crashes.
   breakpad::MonitorURLsForWebStateList(self.mainBrowser->GetWebStateList());
@@ -255,7 +243,6 @@
     breakpad::StopMonitoringTabStateForWebStateList(webStateList);
     breakpad::StopMonitoringURLsForWebStateList(webStateList);
     [tabModel disconnect];
-    webStateList->RemoveObserver(_webStateListForwardingObserver.get());
   }
 
   _mainBrowser = std::move(mainBrowser);
@@ -267,7 +254,6 @@
     WebStateList* webStateList = self.otrBrowser->GetWebStateList();
     breakpad::StopMonitoringTabStateForWebStateList(webStateList);
     [tabModel disconnect];
-    webStateList->RemoveObserver(_webStateListForwardingObserver.get());
   }
 
   _otrBrowser = std::move(otrBrowser);
@@ -389,7 +375,7 @@
   if (restorePersistedState)
     [self restoreSessionToBrowser:browser.get()];
 
-  [self addObserversToWebStateList:browser->GetWebStateList()];
+  breakpad::MonitorTabStateForWebStateList(browser->GetWebStateList());
 
   return browser;
 }
@@ -435,9 +421,4 @@
       sessionWindow);
 }
 
-- (void)addObserversToWebStateList:(WebStateList*)webStateList {
-  webStateList->AddObserver(_webStateListForwardingObserver.get());
-  breakpad::MonitorTabStateForWebStateList(webStateList);
-}
-
 @end

ios/chrome/browser/ui/main/browser_view_wrangler_unittest.mm

@@ -39,7 +39,6 @@ TEST_F(BrowserViewWranglerTest, TestInitNilObserver) {
   @autoreleasepool {
     BrowserViewWrangler* wrangler = [[BrowserViewWrangler alloc]
                initWithBrowserState:chrome_browser_state_.get()
-               webStateListObserver:nil
          applicationCommandEndpoint:(id<ApplicationCommands>)nil
         browsingDataCommandEndpoint:nil
                appURLLoadingService:nil];
@@ -70,7 +69,6 @@ TEST_F(BrowserViewWranglerTest, TestBrowserList) {
 
   BrowserViewWrangler* wrangler = [[BrowserViewWrangler alloc]
              initWithBrowserState:chrome_browser_state_.get()
-             webStateListObserver:nil
        applicationCommandEndpoint:nil
       browsingDataCommandEndpoint:nil
              appURLLoadingService:nil];

ios/chrome/browser/ui/main/scene_controller.mm

@@ -116,7 +116,9 @@ const NSTimeInterval kDisplayPromoDelay = 0.1;
 
 @interface SceneController () <UserFeedbackDataSource,
                                SettingsNavigationControllerDelegate,
-                               WebStateListObserving>
+                               WebStateListObserving> {
+  std::unique_ptr<WebStateListObserverBridge> _webStateListForwardingObserver;
+}
 
 // Navigation View controller for the settings.
 @property(nonatomic, strong)
@@ -212,6 +214,9 @@ const NSTimeInterval kDisplayPromoDelay = 0.1;
     }
     _appURLLoadingService = new AppUrlLoadingService();
     _appURLLoadingService->SetDelegate(self);
+
+    _webStateListForwardingObserver =
+        std::make_unique<WebStateListObserverBridge>(self);
   }
   return self;
 }
@@ -303,12 +308,11 @@ const NSTimeInterval kDisplayPromoDelay = 0.1;
 
   self.browserViewWrangler = [[BrowserViewWrangler alloc]
              initWithBrowserState:self.mainController.mainBrowserState
-             webStateListObserver:self
        applicationCommandEndpoint:self
       browsingDataCommandEndpoint:self.mainController
              appURLLoadingService:self.appURLLoadingService];
 
-  // Ensure the main tab model is created. This also creates the BVC.
+  // Ensure the main browser is created. This also creates the BVC.
   [self.browserViewWrangler createMainBrowser];
 
   // Only create the restoration helper if the browser state was backed up
@@ -364,6 +368,12 @@ const NSTimeInterval kDisplayPromoDelay = 0.1;
   TabModel* mainTabModel = self.mainInterface.tabModel;
   TabModel* otrTabModel = self.incognitoInterface.tabModel;
 
+  // Observe the web state lists for both browsers.
+  self.mainInterface.browser->GetWebStateList()->AddObserver(
+      _webStateListForwardingObserver.get());
+  self.incognitoInterface.browser->GetWebStateList()->AddObserver(
+      _webStateListForwardingObserver.get());
+
   // Enables UI initializations to query the keyWindow's size.
   [self.sceneState.window makeKeyAndVisible];
 
@@ -431,6 +441,13 @@ const NSTimeInterval kDisplayPromoDelay = 0.1;
   [_mainCoordinator stop];
   _mainCoordinator = nil;
 
+  // Stop observing web state list changes before tearing down the web state
+  // lists.
+  self.mainInterface.browser->GetWebStateList()->RemoveObserver(
+      _webStateListForwardingObserver.get());
+  self.incognitoInterface.browser->GetWebStateList()->RemoveObserver(
+      _webStateListForwardingObserver.get());
+
   [self.browserViewWrangler shutdown];
   self.browserViewWrangler = nil;
 
@@ -1576,6 +1593,8 @@ const NSTimeInterval kDisplayPromoDelay = 0.1;
   }
 }
 
+#pragma mark - Helpers for web state list events
+
 // Called when the last incognito tab was closed.
 - (void)lastIncognitoTabClosed {
   // This seems the best place to mark the start of destroying the incognito
@@ -1690,7 +1709,11 @@ const NSTimeInterval kDisplayPromoDelay = 0.1;
             self.incognitoInterface.browserState));
   }
 
+  self.incognitoInterface.browser->GetWebStateList()->RemoveObserver(
+      _webStateListForwardingObserver.get());
   [self.browserViewWrangler destroyAndRebuildIncognitoBrowser];
+  self.incognitoInterface.browser->GetWebStateList()->AddObserver(
+      _webStateListForwardingObserver.get());
 
   if (base::FeatureList::IsEnabled(kLogBreadcrumbs)) {
     breakpad::MonitorBreadcrumbManagerService(