Fix an ARC test flake when the App Service is enabled.

ArcAppsPrivateApiTest.OnInstalled creates a FakeAppInstance, but the instance
is torn down without clearing the underlying connection. This causes test
flakes where the App Service is still trying to resolve callbacks against the
FakeAppInstance that is now destroyed at the end of the test.

This CL fixes the test by creating the FakeAppInstance as a member variable on
the test, and adding a TearDown step to properly clean up the connection.

BUG=992839

Change-Id: I748fcc6c64d66392848ad0c49ec7916e7e8686b6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1757950
Commit-Queue: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688574}

chrome/browser/apps/platform_apps/api/arc_apps_private/arc_apps_private_apitest.cc

@@ -22,28 +22,22 @@
 #include "extensions/test/extension_test_message_listener.h"
 #include "extensions/test/result_catcher.h"
 
-namespace {
-
-// Helper function to create a fake app instance and wait for the instance to be
-// ready.
-std::unique_ptr<arc::FakeAppInstance> CreateAppInstance(
-    ArcAppListPrefs* prefs) {
-  std::unique_ptr<arc::FakeAppInstance> app_instance =
-      std::make_unique<arc::FakeAppInstance>(prefs);
-  arc::ArcServiceManager::Get()->arc_bridge_service()->app()->SetInstance(
-      app_instance.get());
-  WaitForInstanceReady(
-      arc::ArcServiceManager::Get()->arc_bridge_service()->app());
-  return app_instance;
-}
-
-}  // namespace
-
 class ArcAppsPrivateApiTest : public extensions::ExtensionApiTest {
  public:
   ArcAppsPrivateApiTest() = default;
   ~ArcAppsPrivateApiTest() override = default;
 
+ protected:
+  // Helper function to create a fake app instance and wait for the instance to
+  // be ready.
+  void CreateAppInstance(ArcAppListPrefs* prefs) {
+    app_instance_ = std::make_unique<arc::FakeAppInstance>(prefs);
+    arc::ArcServiceManager::Get()->arc_bridge_service()->app()->SetInstance(
+        app_instance());
+    WaitForInstanceReady(
+        arc::ArcServiceManager::Get()->arc_bridge_service()->app());
+  }
+
   void SetUpCommandLine(base::CommandLine* command_line) override {
     extensions::ExtensionApiTest::SetUpCommandLine(command_line);
     arc::SetArcAvailableCommandLineForTesting(command_line);
@@ -66,25 +60,38 @@ class ArcAppsPrivateApiTest : public extensions::ExtensionApiTest {
     arc::SetArcPlayStoreEnabledForProfile(profile(), true);
   }
 
+  void TearDownOnMainThread() override {
+    extensions::ExtensionApiTest::TearDownOnMainThread();
+    if (app_instance_) {
+      arc::ArcServiceManager::Get()->arc_bridge_service()->app()->CloseInstance(
+          app_instance());
+    }
+    app_instance_.reset();
+  }
+
+  arc::FakeAppInstance* app_instance() { return app_instance_.get(); }
+
  private:
+  std::unique_ptr<arc::FakeAppInstance> app_instance_;
+
   DISALLOW_COPY_AND_ASSIGN(ArcAppsPrivateApiTest);
 };
 
 IN_PROC_BROWSER_TEST_F(ArcAppsPrivateApiTest, GetPackageNameAndLaunchApp) {
   ArcAppListPrefs* prefs = ArcAppListPrefs::Get(browser()->profile());
   ASSERT_TRUE(prefs);
-  std::unique_ptr<arc::FakeAppInstance> app_instance = CreateAppInstance(prefs);
+  CreateAppInstance(prefs);
   // Add one launchable app and one non-launchable app.
   arc::mojom::AppInfo launchable_app("App_0", "Package_0", "Dummy_activity_0");
-  app_instance->SendRefreshAppList({launchable_app});
+  app_instance()->SendRefreshAppList({launchable_app});
   static_cast<arc::mojom::AppHost*>(prefs)->OnTaskCreated(
       0 /* task_id */, "Package_1", "Dummy_activity_1", "App_1",
       base::nullopt /* intent */);
 
   // Stopping the service makes the app non-ready.
   arc::ArcServiceManager::Get()->arc_bridge_service()->app()->CloseInstance(
-      app_instance.get());
-  EXPECT_EQ(0u, app_instance->launch_requests().size());
+      app_instance());
+  EXPECT_EQ(0u, app_instance()->launch_requests().size());
   // Verify |chrome.arcAppsPrivate.getLaunchableApps| returns the package name
   // of the launchable app only. The JS test will attempt to launch the app.
   EXPECT_TRUE(
@@ -92,19 +99,20 @@ IN_PROC_BROWSER_TEST_F(ArcAppsPrivateApiTest, GetPackageNameAndLaunchApp) {
       << message_;
 
   // Verify the app is not launched because it's not ready.
-  EXPECT_EQ(0u, app_instance->launch_requests().size());
+  EXPECT_EQ(0u, app_instance()->launch_requests().size());
   // Restarting the service makes the app ready. Verify the app is launched
   // successfully.
-  app_instance = CreateAppInstance(prefs);
-  app_instance->SendRefreshAppList({launchable_app});
-  ASSERT_EQ(1u, app_instance->launch_requests().size());
-  EXPECT_TRUE(app_instance->launch_requests()[0]->IsForApp(launchable_app));
+  CreateAppInstance(prefs);
+  app_instance()->SendRefreshAppList({launchable_app});
+  ASSERT_EQ(1u, app_instance()->launch_requests().size());
+  EXPECT_TRUE(app_instance()->launch_requests()[0]->IsForApp(launchable_app));
 }
 
 IN_PROC_BROWSER_TEST_F(ArcAppsPrivateApiTest, OnInstalled) {
   ArcAppListPrefs* prefs = ArcAppListPrefs::Get(browser()->profile());
   ASSERT_TRUE(prefs);
-  std::unique_ptr<arc::FakeAppInstance> app_instance = CreateAppInstance(prefs);
+  CreateAppInstance(prefs);
+
   arc::mojom::AppInfo launchable_app("App_0", "Package_0", "Dummy_activity_0");
 
   // The JS test will observe the onInstalled event and attempt to launch the
@@ -119,17 +127,17 @@ IN_PROC_BROWSER_TEST_F(ArcAppsPrivateApiTest, OnInstalled) {
   ASSERT_TRUE(app);
   EXPECT_TRUE(ready_listener.WaitUntilSatisfied());
 
-  EXPECT_EQ(0u, app_instance->launch_requests().size());
+  EXPECT_EQ(0u, app_instance()->launch_requests().size());
   // Add one launchable app and one non-launchable app.
-  app_instance->SendRefreshAppList({launchable_app});
+  app_instance()->SendRefreshAppList({launchable_app});
   static_cast<arc::mojom::AppHost*>(prefs)->OnTaskCreated(
       0 /* task_id */, "Package_1", "Dummy_activity_1", "App_1",
       base::nullopt /* intent */);
   // Verify the JS test receives the onInstalled event for the launchable app
   // only, and the app is launched successfully.
   EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
-  ASSERT_EQ(1u, app_instance->launch_requests().size());
-  EXPECT_TRUE(app_instance->launch_requests()[0]->IsForApp(launchable_app));
+  ASSERT_EQ(1u, app_instance()->launch_requests().size());
+  EXPECT_TRUE(app_instance()->launch_requests()[0]->IsForApp(launchable_app));
 }
 
 IN_PROC_BROWSER_TEST_F(ArcAppsPrivateApiTest,
@@ -145,9 +153,9 @@ IN_PROC_BROWSER_TEST_F(ArcAppsPrivateApiTest,
   // Launch an arc app as done in the tests above.
   ArcAppListPrefs* prefs = ArcAppListPrefs::Get(browser()->profile());
   ASSERT_TRUE(prefs);
-  std::unique_ptr<arc::FakeAppInstance> app_instance = CreateAppInstance(prefs);
+  CreateAppInstance(prefs);
   arc::mojom::AppInfo launchable_app("App_0", "Package_0", "Dummy_activity_0");
-  app_instance->SendRefreshAppList({launchable_app});
+  app_instance()->SendRefreshAppList({launchable_app});
   EXPECT_TRUE(
       RunPlatformAppTestWithArg("arc_app_launcher/launch_app", "Package_0"))
       << message_;
@@ -170,9 +178,9 @@ IN_PROC_BROWSER_TEST_F(ArcAppsPrivateApiTest, DemoModeAppLaunchSourceReported) {
   // Launch an arc app as done in the tests above.
   ArcAppListPrefs* prefs = ArcAppListPrefs::Get(browser()->profile());
   ASSERT_TRUE(prefs);
-  std::unique_ptr<arc::FakeAppInstance> app_instance = CreateAppInstance(prefs);
+  CreateAppInstance(prefs);
   arc::mojom::AppInfo launchable_app("App_0", "Package_0", "Dummy_activity_0");
-  app_instance->SendRefreshAppList({launchable_app});
+  app_instance()->SendRefreshAppList({launchable_app});
   EXPECT_TRUE(
       RunPlatformAppTestWithArg("arc_app_launcher/launch_app", "Package_0"))
       << message_;