Fix crash in the experimental app list StartPageView.

This CL fixes a crash in the StartPageView that was happening because
an observer was not being cleaned up on destruction.

This CL also adds a profile change unit test which serves as a
regression test for this issue.

BUG=378572
TEST=Have experimental app list enabled and switch between signed in profiles repeatedly. There should be no crash.

Review URL: https://codereview.chromium.org/305123002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274162 0039d316-1c4b-4281-b951-d872f2087c98

ui/app_list/test/app_list_test_view_delegate.cc

@@ -54,6 +54,7 @@ AppListTestViewDelegate::AppListTestViewDelegate()
     : dismiss_count_(0),
       toggle_speech_recognition_count_(0),
       open_search_result_count_(0),
+      next_profile_app_count_(0),
       test_signin_delegate_(new TestSigninDelegate),
       model_(new AppListTestModel),
       speech_ui_(SPEECH_RECOGNITION_OFF) {
@@ -78,6 +79,11 @@ bool AppListTestViewDelegate::ForceNativeDesktop() const {
   return false;
 }
 
+void AppListTestViewDelegate::SetProfileByPath(
+    const base::FilePath& profile_path) {
+  ReplaceTestModel(next_profile_app_count_);
+}
+
 AppListModel* AppListTestViewDelegate::GetModel() {
   return model_.get();
 }

ui/app_list/test/app_list_test_view_delegate.h

@@ -36,6 +36,10 @@ class AppListTestViewDelegate : public AppListViewDelegate {
     return open_search_result_counts_;
   }
 
+  // Sets the number of apps that the model will be created with the next time
+  // SetProfileByPath() is called.
+  void set_next_profile_app_count(int apps) { next_profile_app_count_ = apps; }
+
   void set_auto_launch_timeout(const base::TimeDelta& timeout) {
     auto_launch_timeout_ = timeout;
   }
@@ -50,7 +54,7 @@ class AppListTestViewDelegate : public AppListViewDelegate {
 
   // AppListViewDelegate overrides:
   virtual bool ForceNativeDesktop() const OVERRIDE;
-  virtual void SetProfileByPath(const base::FilePath& profile_path) OVERRIDE {}
+  virtual void SetProfileByPath(const base::FilePath& profile_path) OVERRIDE;
   virtual AppListModel* GetModel() OVERRIDE;
   virtual SigninDelegate* GetSigninDelegate() OVERRIDE;
   virtual SpeechUIModel* GetSpeechUI() OVERRIDE;
@@ -94,6 +98,7 @@ class AppListTestViewDelegate : public AppListViewDelegate {
   int dismiss_count_;
   int toggle_speech_recognition_count_;
   int open_search_result_count_;
+  int next_profile_app_count_;
   std::map<size_t, int> open_search_result_counts_;
   Users users_;
   scoped_ptr<TestSigninDelegate> test_signin_delegate_;

ui/app_list/views/app_list_view_unittest.cc

@@ -76,6 +76,9 @@ class AppListViewTestContext {
   // Tests displaying of the experimental app list and shows the start page.
   void RunStartPageTest();
 
+  // Tests that changing the App List profile.
+  void RunProfileChangeTest();
+
   // A standard set of checks on a view, e.g., ensuring it is drawn and visible.
   static void CheckView(views::View* subview);
 
@@ -301,6 +304,50 @@ void AppListViewTestContext::RunStartPageTest() {
   Close();
 }
 
+void AppListViewTestContext::RunProfileChangeTest() {
+  EXPECT_FALSE(view_->GetWidget()->IsVisible());
+  EXPECT_EQ(-1, pagination_model_.total_pages());
+  delegate_->GetTestModel()->PopulateApps(kInitialItems);
+
+  Show();
+
+  if (is_landscape())
+    EXPECT_EQ(2, pagination_model_.total_pages());
+  else
+    EXPECT_EQ(3, pagination_model_.total_pages());
+
+  // Change the profile. The original model needs to be kept alive for
+  // observers to unregister themselves.
+  scoped_ptr<AppListTestModel> original_test_model(
+      delegate_->ReleaseTestModel());
+  delegate_->set_next_profile_app_count(1);
+
+  // The original ContentsView is destroyed here.
+  view_->SetProfileByPath(base::FilePath());
+  EXPECT_EQ(1, pagination_model_.total_pages());
+
+  StartPageView* start_page_view =
+      view_->app_list_main_view()->contents_view()->start_page_view();
+  if (test_type_ == EXPERIMENTAL) {
+    EXPECT_NO_FATAL_FAILURE(CheckView(start_page_view));
+    EXPECT_EQ(1u, GetVisibleTileItemViews(start_page_view->tile_views()));
+  } else {
+    EXPECT_EQ(NULL, start_page_view);
+  }
+
+  // New model updates should be processed by the start page view.
+  delegate_->GetTestModel()->CreateAndAddItem("Test App");
+  if (test_type_ == EXPERIMENTAL)
+    EXPECT_EQ(2u, GetVisibleTileItemViews(start_page_view->tile_views()));
+
+  // Old model updates should be ignored.
+  original_test_model->CreateAndAddItem("Test App 2");
+  if (test_type_ == EXPERIMENTAL)
+    EXPECT_EQ(2u, GetVisibleTileItemViews(start_page_view->tile_views()));
+
+  Close();
+}
+
 class AppListViewTestAura : public views::ViewsTestBase,
                             public ::testing::WithParamInterface<int> {
  public:
@@ -414,6 +461,15 @@ TEST_P(AppListViewTestDesktop, StartPageTest) {
   EXPECT_NO_FATAL_FAILURE(test_context_->RunStartPageTest());
 }
 
+// Tests that the profile changes operate correctly.
+TEST_P(AppListViewTestAura, ProfileChangeTest) {
+  EXPECT_NO_FATAL_FAILURE(test_context_->RunProfileChangeTest());
+}
+
+TEST_P(AppListViewTestDesktop, ProfileChangeTest) {
+  EXPECT_NO_FATAL_FAILURE(test_context_->RunProfileChangeTest());
+}
+
 INSTANTIATE_TEST_CASE_P(AppListViewTestAuraInstance,
                         AppListViewTestAura,
                         ::testing::Range<int>(TEST_TYPE_START, TEST_TYPE_END));

ui/app_list/views/start_page_view.cc

@@ -121,6 +121,8 @@ StartPageView::StartPageView(AppListMainView* app_list_main_view,
 
 StartPageView::~StartPageView() {
   view_delegate_->RemoveObserver(this);
+  if (model_)
+    model_->RemoveObserver(this);
 }
 
 void StartPageView::SetModel(AppListModel* model) {