DPWA: suppressOsHooks in all relevant tests

Make sure web app tests on desktop do not unintentionally
deploy OS hooks. A separate CL will handle chromeOS specific
tests.

Bug: 1124383
Change-Id: I3b5130cdf3754cd84267260d53dbf5ff12a6f59e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2399500
Commit-Queue: Phillis Tang <phillis@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820981}

chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc

@@ -38,6 +38,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
 #include "chrome/browser/web_applications/components/install_finalizer.h"
+#include "chrome/browser/web_applications/components/os_integration_manager.h"
 #include "chrome/browser/web_applications/components/web_app_provider_base.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_render_frame.mojom.h"
@@ -122,6 +123,13 @@ class ContextMenuBrowserTest : public InProcessBrowserTest {
  public:
   ContextMenuBrowserTest() {}
 
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+    WebAppProviderBase::GetProviderBase(browser()->profile())
+        ->os_integration_manager()
+        .SuppressOsHooksForTesting();
+  }
+
  protected:
   std::unique_ptr<TestRenderViewContextMenu> CreateContextMenuMediaTypeNone(
       const GURL& unfiltered_url,

chrome/browser/sync/test/integration/two_client_web_apps_sync_test.cc

@@ -29,6 +29,18 @@ namespace web_app {
 class TwoClientWebAppsSyncTest : public SyncTest {
  public:
   TwoClientWebAppsSyncTest() : SyncTest(TWO_CLIENT) {}
+
+  bool SetupClients() override {
+    bool result = SyncTest::SetupClients();
+    if (!result)
+      return result;
+    for (Profile* profile : GetAllProfiles()) {
+      auto* web_app_provider = WebAppProvider::Get(profile);
+      web_app_provider->os_integration_manager().SuppressOsHooksForTesting();
+    }
+    return true;
+  }
+
   ~TwoClientWebAppsSyncTest() override = default;
 
   void SetUpOnMainThread() override {

chrome/browser/ui/views/web_apps/web_app_uninstall_dialog_browsertest.cc

@@ -17,7 +17,9 @@
 #include "chrome/browser/ui/test/test_browser_dialog.h"
 #include "chrome/browser/ui/views/web_apps/web_app_uninstall_dialog_view.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
+#include "chrome/browser/web_applications/components/os_integration_manager.h"
 #include "chrome/browser/web_applications/test/web_app_icon_test_utils.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/common/web_application_info.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/test/browser_test.h"
@@ -41,7 +43,14 @@ AppId InstallTestWebApp(Profile* profile) {
 
 }  // namespace
 
-using WebAppUninstallDialogViewBrowserTest = InProcessBrowserTest;
+class WebAppUninstallDialogViewBrowserTest : public InProcessBrowserTest {
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+    web_app::WebAppProvider::Get(browser()->profile())
+        ->os_integration_manager()
+        .SuppressOsHooksForTesting();
+  }
+};
 
 // Test that WebAppUninstallDialog cancels the uninstall if the Window
 // which is passed to WebAppUninstallDialog::Create() is destroyed before

chrome/browser/ui/web_applications/web_app_engagement_browsertest.cc

@@ -24,6 +24,7 @@
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
 #include "chrome/browser/ui/web_applications/web_app_controller_browsertest.h"
 #include "chrome/browser/web_applications/components/external_install_options.h"
+#include "chrome/browser/web_applications/components/os_integration_manager.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
@@ -161,6 +162,13 @@ class WebAppEngagementBrowserTest : public WebAppControllerBrowserTestBase {
       delete;
   ~WebAppEngagementBrowserTest() override = default;
 
+  void SetUpOnMainThread() override {
+    WebAppControllerBrowserTestBase::SetUpOnMainThread();
+    WebAppProvider::Get(browser()->profile())
+        ->os_integration_manager()
+        .SuppressOsHooksForTesting();
+  }
+
   void TestEngagementEventWebAppLaunch(const base::HistogramTester& tester,
                                        const Histograms& histograms) {
     ExpectUniqueSamples(

chrome/browser/web_applications/components/os_integration_manager.cc

@@ -20,6 +20,12 @@
 #include "chrome/browser/web_applications/components/app_shim_registry_mac.h"
 #endif
 
+namespace {
+// Used to  disable os hooks globally when OsIntegrationManager::SuppressOsHooks
+// can't be easily used.
+bool g_suppress_os_hooks_for_testing_ = false;
+}  // namespace
+
 namespace web_app {
 
 InstallOsHooksOptions::InstallOsHooksOptions() = default;
@@ -95,8 +101,7 @@ void OsIntegrationManager::InstallOsHooks(
     std::unique_ptr<WebApplicationInfo> web_app_info,
     InstallOsHooksOptions options) {
   DCHECK(shortcut_manager_);
-
-  if (suppress_os_hooks_for_testing_) {
+  if (suppress_os_hooks_for_testing_ || g_suppress_os_hooks_for_testing_) {
     OsHooksResults os_hooks_results{true};
     base::SequencedTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::BindOnce(std::move(callback), os_hooks_results));
@@ -150,7 +155,7 @@ void OsIntegrationManager::UninstallOsHooks(const AppId& app_id,
                                             UninstallOsHooksCallback callback) {
   DCHECK(shortcut_manager_);
 
-  if (suppress_os_hooks_for_testing_) {
+  if (suppress_os_hooks_for_testing_ || g_suppress_os_hooks_for_testing_) {
     OsHooksResults os_hooks_results{true};
     base::SequencedTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::BindOnce(std::move(callback), os_hooks_results));
@@ -212,7 +217,7 @@ void OsIntegrationManager::UpdateOsHooks(
     const WebApplicationInfo& web_app_info) {
   DCHECK(shortcut_manager_);
 
-  if (suppress_os_hooks_for_testing_)
+  if (suppress_os_hooks_for_testing_ || g_suppress_os_hooks_for_testing_)
     return;
 
   // TODO(crbug.com/1079439): Update file handlers.
@@ -286,8 +291,20 @@ FileHandlerManager& OsIntegrationManager::file_handler_manager_for_testing() {
   return *file_handler_manager_;
 }
 
+void OsIntegrationManager::GlobalSuppressOsHooksForTesting() {
+// Creating OS hooks on ChromeOS doesn't write files to disk, so it's
+// unnecessary to suppress and it provides better crash coverage.
+#if !defined(OS_CHROMEOS)
+  g_suppress_os_hooks_for_testing_ = true;
+#endif
+}
+
 void OsIntegrationManager::SuppressOsHooksForTesting() {
+// Creating OS hooks on ChromeOS doesn't write files to disk, so it's
+// unnecessary to suppress and it provides better crash coverage.
+#if !defined(OS_CHROMEOS)
   suppress_os_hooks_for_testing_ = true;
+#endif
 }
 
 void OsIntegrationManager::OnShortcutsCreated(

chrome/browser/web_applications/components/os_integration_manager.h

@@ -121,6 +121,7 @@ class OsIntegrationManager {
   // Getter for testing FileHandlerManager
   FileHandlerManager& file_handler_manager_for_testing();
 
+  static void GlobalSuppressOsHooksForTesting();
   void SuppressOsHooksForTesting();
 
  protected:

chrome/browser/web_applications/external_web_app_manager_browsertest.cc

@@ -12,6 +12,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
+#include "chrome/browser/web_applications/components/os_integration_manager.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/test/test_file_utils.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
@@ -31,6 +32,13 @@ class ExternalWebAppManagerBrowserTest
     ExternalWebAppManager::SkipStartupForTesting();
   }
 
+  void SetUpOnMainThread() override {
+    ExtensionBrowserTest::SetUpOnMainThread();
+    WebAppProvider::Get(browser()->profile())
+        ->os_integration_manager()
+        .SuppressOsHooksForTesting();
+  }
+
   GURL GetAppUrl() const {
     return embedded_test_server()->GetURL("/web_apps/basic.html");
   }

chrome/browser/web_applications/external_web_app_migration_browsertest.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/web_applications/test/ssl_test_utils.h"
 #include "chrome/browser/web_applications/components/external_app_install_features.h"
+#include "chrome/browser/web_applications/components/os_integration_manager.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/external_web_app_manager.h"
 #include "chrome/browser/web_applications/preinstalled_web_apps.h"
@@ -91,6 +92,9 @@ class ExternalWebAppMigrationBrowserTest : public InProcessBrowserTest {
     SetUpExtensionTestExternalProvider();
 
     InProcessBrowserTest::SetUpOnMainThread();
+    WebAppProvider::Get(browser()->profile())
+        ->os_integration_manager()
+        .SuppressOsHooksForTesting();
   }
 
   std::unique_ptr<net::test_server::HttpResponse> RequestHandlerOverride(

chrome/browser/web_applications/manifest_update_manager_browsertest.cc

@@ -197,12 +197,13 @@ class ManifestUpdateManagerBrowserTest : public InProcessBrowserTest {
         &ManifestUpdateManagerBrowserTest::RequestHandlerOverride,
         base::Unretained(this)));
     ASSERT_TRUE(http_server_.Start());
-
+    // Suppress globally to avoid OS hooks deployed for system web app during
+    // WebAppProvider setup.
+    OsIntegrationManager::GlobalSuppressOsHooksForTesting();
     InProcessBrowserTest::SetUp();
   }
 
   void SetUpOnMainThread() override {
-    GetProvider().os_integration_manager().SuppressOsHooksForTesting();
     // Cannot construct RunLoop in constructor due to threading restrictions.
     shortcut_run_loop_.emplace();
   }

chrome/browser/web_applications/pending_app_manager_impl_browsertest.cc

@@ -37,6 +37,9 @@ class PendingAppManagerImplBrowserTest : public InProcessBrowserTest {
     InProcessBrowserTest::SetUpOnMainThread();
     // Allow different origins to be handled by the embedded_test_server.
     host_resolver()->AddRule("*", "127.0.0.1");
+    WebAppProviderBase::GetProviderBase(browser()->profile())
+        ->os_integration_manager()
+        .SuppressOsHooksForTesting();
   }
 
   AppRegistrar& registrar() {
@@ -44,11 +47,6 @@ class PendingAppManagerImplBrowserTest : public InProcessBrowserTest {
         ->registrar();
   }
 
-  OsIntegrationManager& os_integration_manager() {
-    return WebAppProviderBase::GetProviderBase(browser()->profile())
-        ->os_integration_manager();
-  }
-
   PendingAppManager& pending_app_manager() {
     return WebAppProviderBase::GetProviderBase(browser()->profile())
         ->pending_app_manager();
@@ -149,7 +147,6 @@ IN_PROC_BROWSER_TEST_F(PendingAppManagerImplBrowserTest,
 IN_PROC_BROWSER_TEST_F(PendingAppManagerImplBrowserTest,
                        PlaceholderInstallSucceedsWithShortcuts) {
   ASSERT_TRUE(embedded_test_server()->Start());
-  os_integration_manager().SuppressOsHooksForTesting();
 
   GURL final_url = embedded_test_server()->GetURL(
       "other.origin.com", "/banners/manifest_test_page.html");

chrome/browser/web_applications/web_app_migration_manager_browsertest.cc

@@ -106,6 +106,7 @@ class WebAppMigrationManagerBrowserTest : public InProcessBrowserTest {
 
   void SetUpOnMainThread() override {
     InProcessBrowserTest::SetUpOnMainThread();
+    provider().os_integration_manager().SuppressOsHooksForTesting();
 
     // We use a URLLoaderInterceptor, rather than the EmbeddedTestServer, since
     // a stable app_id across tests requires stable origin, whereas
@@ -147,8 +148,6 @@ class WebAppMigrationManagerBrowserTest : public InProcessBrowserTest {
         /*auto_accept=*/true,
         /*auto_open_in_window=*/true);
 
-    provider().os_integration_manager().SuppressOsHooksForTesting();
-
     AppId app_id;
     base::RunLoop run_loop;
     bool started = CreateWebAppFromManifest(