arc: Fix crash on accessing app info for secondary user.

chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc

Index: chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
diff --git a/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc b/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
index 73ad28dc30a862fee0f2d7683d919de77e5e38b6..2e5063ca67f1af7b4a30b74529e3e8b26b9e7524 100644
--- a/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
+++ b/chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc
@@ -14,6 +14,7 @@
 #include "chrome/browser/extensions/test_extension_environment.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/views/apps/app_info_dialog/app_info_header_panel.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "chrome/test/base/testing_profile.h"
 #include "extensions/browser/extension_system.h"
@@ -25,9 +26,25 @@
 #include "ui/views/window/dialog_delegate.h"
 
 #if defined(OS_CHROMEOS)
-#include "chrome/browser/chromeos/arc/arc_session_manager.h"
-#include "components/arc/arc_session_runner.h"
-#include "components/arc/test/fake_arc_session.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_test.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
+#endif
+
+#if defined(OS_CHROMEOS)
+namespace {
+
+std::vector<arc::mojom::AppInfoPtr> GetArcSettingsAppMojoInfo() {

[msw, 2017/02/15 21:48:28]

It's a little odd to return a vector for one item, but I guess that's what the
caller needs... nit: GetArcSettingsAppInfo

[khmel, 2017/02/16 15:47:52]

Done.

+  std::vector<arc::mojom::AppInfoPtr> apps;
+  arc::mojom::AppInfoPtr app(arc::mojom::AppInfo::New());
+  app->name = "settings";
+  app->package_name = "com.android.settings";
+  app->activity = "com.android.settings.Settings";
+  app->sticky = false;
+  apps.push_back(std::move(app));
+  return apps;
+}
+}  // namespace

[msw, 2017/02/15 21:48:28]

nit: add a blank line above

[khmel, 2017/02/16 15:47:51]

Done.

 #endif
 
 namespace test {
@@ -69,35 +86,19 @@ class AppInfoDialogViewsTest : public BrowserWithTestWindowTest,
   void SetUp() override {
     BrowserWithTestWindowTest::SetUp();
 #if defined(OS_CHROMEOS)
-    arc::ArcSessionManager::DisableUIForTesting();

[khmel, 2017/02/15 16:38:45]

This actually does not start ARC (we need extra preparation for flags and
setting primary profile). Use better method for this

[msw, 2017/02/15 21:48:28]

Acknowledged.

-    arc_session_manager_ = base::MakeUnique<arc::ArcSessionManager>(
-        base::MakeUnique<arc::ArcSessionRunner>(
-            base::Bind(arc::FakeArcSession::Create)));
-    arc_session_manager_->OnPrimaryUserProfilePrepared(
-        extension_environment_.profile());
+    arc_test_.SetUp(extension_environment_.profile());
 #endif
-    widget_ = views::DialogDelegate::CreateDialogWidget(
-        new views::DialogDelegateView(), GetContext(), nullptr);
-    widget_->AddObserver(this);
     extension_ = extension_environment_.MakePackagedApp(kTestExtensionId, true);
-    dialog_ = new AppInfoDialog(widget_->GetNativeWindow(),

[khmel, 2017/02/15 16:38:45]

Move creation and closing to separate methods Show, ShowForProfile, Close in
order to be able to call showing multiple times.

[msw, 2017/02/15 21:48:28]

Acknowledged.

-                                extension_environment_.profile(),
-                                extension_.get());
-
-    widget_->GetContentsView()->AddChildView(dialog_);
-    widget_->Show();
+    chrome_app_ = extension_environment_.MakePackagedApp(
+        extension_misc::kChromeAppId, true);
   }
 
   void TearDown() override {
-    if (!widget_destroyed_)
-      widget_->CloseNow();
-    EXPECT_TRUE(widget_destroyed_);
+    CloseAppInfo();
     extension_ = nullptr;
+    chrome_app_ = nullptr;
 #if defined(OS_CHROMEOS)
-    if (arc_session_manager_) {
-      arc_session_manager_->Shutdown();
-      arc_session_manager_ = nullptr;
-    }
+    arc_test_.TearDown();
 #endif
     BrowserWithTestWindowTest::TearDown();
   }
@@ -109,14 +110,43 @@ class AppInfoDialogViewsTest : public BrowserWithTestWindowTest,
 
   void DestroyProfile(TestingProfile* profile) override {
 #if defined(OS_CHROMEOS)
-    if (arc_session_manager_) {
-      arc_session_manager_->Shutdown();
-      arc_session_manager_ = nullptr;
-    }
+    arc_test_.TearDown();
 #endif
   }
 
  protected:
+  void ShowAppInfo(const std::string& app_id) {
+    ShowAppInfoForProfile(app_id, extension_environment_.profile());
+  }
+
+  void ShowAppInfoForProfile(const std::string& app_id, Profile* profile) {
+    const extensions::Extension* extension =
+        extensions::ExtensionSystem::Get(profile)
+            ->extension_service()
+            ->GetExtensionById(app_id, true);
+    DCHECK(extension);
+
+    DCHECK(!widget_);
+    widget_destroyed_ = false;
+    widget_ = views::DialogDelegate::CreateDialogWidget(
+        new views::DialogDelegateView(), GetContext(), nullptr);
+    widget_->AddObserver(this);
+    dialog_ = new AppInfoDialog(widget_->GetNativeWindow(), profile, extension);
+
+    widget_->GetContentsView()->AddChildView(dialog_);
+    widget_->Show();
+  }
+
+  void CloseAppInfo() {
+    if (!widget_destroyed_) {
+      DCHECK(widget_);
+      widget_->CloseNow();
+    }
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(widget_destroyed_);
+    DCHECK(!widget_);
+  }
+
   // Overridden from views::WidgetObserver:
   void OnWidgetDestroyed(views::Widget* widget) override {
     widget_destroyed_ = true;
@@ -134,12 +164,13 @@ class AppInfoDialogViewsTest : public BrowserWithTestWindowTest,
 
  protected:
   views::Widget* widget_ = nullptr;
-  bool widget_destroyed_ = false;
+  bool widget_destroyed_ = true;

[msw, 2017/02/15 21:48:28]

optional nit: invert this member as |widget_exists_ = false;| (or optionally
remove it, since it seems like |widget_| serves the same purpose)

[khmel, 2017/02/16 15:47:52]

Good catch. removed.

   AppInfoDialog* dialog_ = nullptr;  // Owned by |widget_|'s views hierarchy.
   scoped_refptr<extensions::Extension> extension_;
+  scoped_refptr<extensions::Extension> chrome_app_;
   extensions::TestExtensionEnvironment extension_environment_;
 #if defined(OS_CHROMEOS)
-  std::unique_ptr<arc::ArcSessionManager> arc_session_manager_;
+  ArcAppTest arc_test_;
 #endif
 
  private:
@@ -148,6 +179,7 @@ class AppInfoDialogViewsTest : public BrowserWithTestWindowTest,
 
 // Tests that the dialog closes when the current app is uninstalled.
 TEST_F(AppInfoDialogViewsTest, UninstallingAppClosesDialog) {
+  ShowAppInfo(kTestExtensionId);
   EXPECT_FALSE(widget_->IsClosed());
   EXPECT_FALSE(widget_destroyed_);
   UninstallApp(kTestExtensionId);
@@ -157,6 +189,7 @@ TEST_F(AppInfoDialogViewsTest, UninstallingAppClosesDialog) {
 
 // Tests that the dialog does not close when a different app is uninstalled.
 TEST_F(AppInfoDialogViewsTest, UninstallingOtherAppDoesNotCloseDialog) {
+  ShowAppInfo(kTestExtensionId);
   extension_environment_.MakePackagedApp(kTestOtherExtensionId, true);
 
   EXPECT_FALSE(widget_->IsClosed());
@@ -168,6 +201,7 @@ TEST_F(AppInfoDialogViewsTest, UninstallingOtherAppDoesNotCloseDialog) {
 
 // Tests that the dialog closes when the current profile is destroyed.
 TEST_F(AppInfoDialogViewsTest, DestroyedProfileClosesDialog) {
+  ShowAppInfo(kTestExtensionId);
   // First delete the test browser window. This ensures the test harness isn't
   // surprised by it being closed in response to the profile deletion below.
   // Note the base class doesn't own the profile, so that part is skipped.
@@ -187,6 +221,7 @@ TEST_F(AppInfoDialogViewsTest, DestroyedProfileClosesDialog) {
 
 // Tests that the dialog does not close when a different profile is destroyed.
 TEST_F(AppInfoDialogViewsTest, DestroyedOtherProfileDoesNotCloseDialog) {
+  ShowAppInfo(kTestExtensionId);
   std::unique_ptr<TestingProfile> other_profile(new TestingProfile);
   extension_environment_.CreateExtensionServiceForProfile(other_profile.get());
 
@@ -206,6 +241,7 @@ TEST_F(AppInfoDialogViewsTest, DestroyedOtherProfileDoesNotCloseDialog) {
 // Tests that clicking the View in Store link opens a browser tab and closes the
 // dialog cleanly.
 TEST_F(AppInfoDialogViewsTest, ViewInStore) {
+  ShowAppInfo(kTestExtensionId);
   EXPECT_TRUE(extension_->from_webstore());  // Otherwise there is no link.
   views::Link* link = test::AppInfoDialogTestApi(dialog_).view_in_store_link();
   EXPECT_TRUE(link);
@@ -231,3 +267,39 @@ TEST_F(AppInfoDialogViewsTest, ViewInStore) {
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(widget_destroyed_);
 }
+
+#if defined(OS_CHROMEOS)
+TEST_F(AppInfoDialogViewsTest, ArcAppInfoLinks) {
+  ShowAppInfo(extension_misc::kChromeAppId);
+  EXPECT_FALSE(widget_->IsClosed());
+  // App Info should not have ARC App info links section because ARC Settings
+  // app is not available yet.
+  EXPECT_FALSE(dialog_->arc_app_info_links_);
+
+  // Re-show App Info but with ARC Settings app enabled.
+  CloseAppInfo();
+  ArcAppListPrefs* arc_prefs =
+      ArcAppListPrefs::Get(extension_environment_.profile());
+  ASSERT_TRUE(arc_prefs);
+  arc::mojom::AppHost* app_host = arc_prefs;
+  app_host->OnAppListRefreshed(GetArcSettingsAppMojoInfo());
+  EXPECT_TRUE(arc_prefs->IsRegistered(arc::kSettingsAppId));
+  ShowAppInfo(extension_misc::kChromeAppId);
+  EXPECT_FALSE(widget_->IsClosed());
+  EXPECT_TRUE(dialog_->arc_app_info_links_);
+
+  // Re-show App Info but for non-primary profie.

[msw, 2017/02/15 21:48:28]

nit: 'profile'

[khmel, 2017/02/16 15:47:52]

Done.

+  CloseAppInfo();
+  std::unique_ptr<TestingProfile> other_profile(new TestingProfile);

[msw, 2017/02/15 21:48:28]

nit: base::MakeUnique

[khmel, 2017/02/16 15:47:51]

Done.

+  extension_environment_.CreateExtensionServiceForProfile(other_profile.get());
+  scoped_refptr<const extensions::Extension> other_app =
+      extension_environment_.MakePackagedApp(extension_misc::kChromeAppId,
+                                             true);
+  extensions::ExtensionSystem::Get(other_profile.get())
+      ->extension_service()
+      ->AddExtension(other_app.get());
+  ShowAppInfoForProfile(extension_misc::kChromeAppId, other_profile.get());
+  EXPECT_FALSE(widget_->IsClosed());
+  EXPECT_FALSE(dialog_->arc_app_info_links_);

[msw, 2017/02/15 21:48:28]

nit: add some explanation as to why the links aren't available here, ie. "The
ARC App info links are not available to the secondary profile with ARC
disabled."

[khmel, 2017/02/16 15:47:52]

Thanks for ready-to-use proposal :)

+}
+#endif