[BrowserSwitcher] Fix cache.dat race condition with extension

When disabled, built-in LBS deletes cache.dat on startup, even if the
LBS extension is also installed. Depending on the order things happen
in, this may cause built-in LBS to delete the extension's cache.dat
file, which is needed for IE=>Chrome switching to work correctly.

After this change, built-in LBS keeps cache.dat and sitelistcache.dat
if the extension is enabled.

Bug: 1000099
Change-Id: I2ecf2f0942fe5888da43b49927cf2734f6e4ae00
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1829478
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701196}

chrome/browser/browser_switcher/browser_switcher_policy_migrator.cc

@@ -18,8 +18,6 @@ namespace browser_switcher {
 
 namespace {
 
-const char kLBSExtensionId[] = "heildphpnddilhkemkielfhnkaagiabh";
-
 void SecondsToMilliseconds(base::Value* val) {
   const int ms_per_second = 1000;
   *val = base::Value(val->GetInt() * ms_per_second);
@@ -43,6 +41,8 @@ void StringToList(base::Value* val) {
 
 }  // namespace
 
+const char kLBSExtensionId[] = "heildphpnddilhkemkielfhnkaagiabh";
+
 BrowserSwitcherPolicyMigrator::BrowserSwitcherPolicyMigrator() = default;
 BrowserSwitcherPolicyMigrator::~BrowserSwitcherPolicyMigrator() = default;
 

chrome/browser/browser_switcher/browser_switcher_policy_migrator.h

@@ -9,6 +9,8 @@
 
 namespace browser_switcher {
 
+extern const char kLBSExtensionId[];
+
 class BrowserSwitcherPolicyMigrator
     : public policy::ChromeExtensionPolicyMigrator {
  public:

chrome/browser/browser_switcher/browser_switcher_service.cc

@@ -262,6 +262,10 @@ BrowserSwitcherPrefs& BrowserSwitcherService::prefs() {
   return prefs_;
 }
 
+Profile* BrowserSwitcherService::profile() {
+  return profile_;
+}
+
 XmlDownloader* BrowserSwitcherService::sitelist_downloader() {
   return sitelist_downloader_.get();
 }

chrome/browser/browser_switcher/browser_switcher_service.h

@@ -130,6 +130,7 @@ class BrowserSwitcherService : public KeyedService {
   AlternativeBrowserDriver* driver();
   BrowserSwitcherSitelist* sitelist();
   BrowserSwitcherPrefs& prefs();
+  Profile* profile();
 
   base::TimeDelta fetch_delay();
   base::TimeDelta refresh_delay();

chrome/browser/browser_switcher/browser_switcher_service_browsertest.cc

@@ -32,7 +32,13 @@
 #if defined(OS_WIN)
 #include "base/files/scoped_temp_dir.h"
 #include "base/path_service.h"
+#include "chrome/browser/browser_switcher/browser_switcher_policy_migrator.h"
 #include "chrome/browser/browser_switcher/browser_switcher_service_win.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
 #endif
 
 namespace browser_switcher {
@@ -870,7 +876,7 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
   policy_provider().UpdateChromePolicy(policies);
   base::RunLoop().RunUntilIdle();
 
-  base::CreateDirectory(cache_dir());
+  ASSERT_TRUE(base::CreateDirectory(cache_dir()));
   base::WriteFile(sitelist_cache_file_path(), "", 0);
   ASSERT_TRUE(base::PathExists(sitelist_cache_file_path()));
 
@@ -913,6 +919,51 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, WritesNothingIfDisabled) {
       action_timeout());
   run_loop.Run();
 }
+
+IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
+                       DoesNotDeleteIfExtensionIsEnabled) {
+  base::ScopedAllowBlockingForTesting allow_blocking;
+
+  // No policies configured.
+
+  // LBS extension is installed.
+  auto extension = extensions::ExtensionBuilder()
+                       .SetLocation(extensions::Manifest::INTERNAL)
+                       .SetID(kLBSExtensionId)
+                       .SetManifest(extensions::DictionaryBuilder()
+                                        .Set("name", "Legacy Browser Support")
+                                        .Set("manifest_version", 2)
+                                        .Set("version", "5.9")
+                                        .Build())
+                       .Build();
+  extensions::ExtensionSystem::Get(browser()->profile())
+      ->extension_service()
+      ->AddExtension(extension.get());
+
+  // Cache files already exist.
+  ASSERT_TRUE(base::CreateDirectory(cache_dir()));
+  base::WriteFile(cache_file_path(), "", 0);
+  base::WriteFile(sitelist_cache_file_path(), "", 0);
+  ASSERT_TRUE(base::PathExists(cache_file_path()));
+  ASSERT_TRUE(base::PathExists(sitelist_cache_file_path()));
+
+  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
+  base::RunLoop run_loop;
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](base::FilePath cache_dir, base::FilePath cache_file_path,
+             base::FilePath sitelist_cache_file_path, base::OnceClosure quit) {
+            EXPECT_TRUE(base::PathExists(cache_dir));
+            EXPECT_TRUE(base::PathExists(cache_file_path));
+            EXPECT_TRUE(base::PathExists(sitelist_cache_file_path));
+            std::move(quit).Run();
+          },
+          cache_dir(), cache_file_path(), sitelist_cache_file_path(),
+          run_loop.QuitClosure()),
+      action_timeout());
+  run_loop.Run();
+}
 #endif
 
 }  // namespace browser_switcher

chrome/browser/browser_switcher/browser_switcher_service_win.cc

@@ -21,10 +21,12 @@
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "base/win/registry.h"
+#include "chrome/browser/browser_switcher/browser_switcher_policy_migrator.h"
 #include "chrome/browser/browser_switcher/browser_switcher_prefs.h"
 #include "chrome/browser/browser_switcher/browser_switcher_sitelist.h"
 #include "chrome/browser/browser_switcher/ieem_sitelist_parser.h"
 #include "chrome/browser/profiles/profile.h"
+#include "extensions/browser/extension_registry.h"
 
 namespace browser_switcher {
 
@@ -140,6 +142,13 @@ base::Optional<std::string>* IeemSitelistUrlForTesting() {
   return ieem_sitelist_url_for_testing.get();
 }
 
+bool IsLBSExtensionEnabled(Profile* profile) {
+  auto* reg = extensions::ExtensionRegistry::Get(profile);
+  DCHECK(reg);
+  return reg->GetExtensionById(kLBSExtensionId,
+                               extensions::ExtensionRegistry::ENABLED);
+}
+
 }  // namespace
 
 BrowserSwitcherServiceWin::BrowserSwitcherServiceWin(Profile* profile)
@@ -148,14 +157,7 @@ BrowserSwitcherServiceWin::BrowserSwitcherServiceWin(Profile* profile)
           {base::ThreadPool(), base::MayBlock(),
            base::TaskPriority::BEST_EFFORT,
            base::TaskShutdownBehavior::BLOCK_SHUTDOWN})) {
-  if (prefs().IsEnabled())
-    SavePrefsToFile();
-  else
-    DeletePrefsFile();
-
-  // Clean up sitelistcache.dat from the extension, or from a previous Chrome
-  // version.
-  DeleteSitelistCacheFile();
+  UpdateAllCacheFiles();
 }
 
 BrowserSwitcherServiceWin::~BrowserSwitcherServiceWin() = default;
@@ -165,10 +167,7 @@ void BrowserSwitcherServiceWin::OnBrowserSwitcherPrefsChanged(
     const std::vector<std::string>& changed_prefs) {
   BrowserSwitcherService::OnBrowserSwitcherPrefsChanged(prefs, changed_prefs);
 
-  if (prefs->IsEnabled())
-    SavePrefsToFile();
-  else
-    DeletePrefsFile();
+  UpdateAllCacheFiles();
 }
 
 // static
@@ -239,21 +238,36 @@ void BrowserSwitcherServiceWin::OnIeemSitelistParsed(ParsedXml xml) {
 }
 
 void BrowserSwitcherServiceWin::SavePrefsToFile() {
+  DCHECK(prefs().IsEnabled());
   sequenced_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&SaveDataToFile, SerializeCacheFile(prefs(), sitelist()),
                      "cache.dat"));
 }
 
-void BrowserSwitcherServiceWin::DeletePrefsFile() const {
+void BrowserSwitcherServiceWin::DeletePrefsFile() {
   sequenced_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&DoRemoveFileFromCacheDir, "cache.dat"));
 }
 
-void BrowserSwitcherServiceWin::DeleteSitelistCacheFile() const {
+void BrowserSwitcherServiceWin::DeleteSitelistCacheFile() {
   sequenced_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&DoRemoveFileFromCacheDir, "sitelistcache.dat"));
 }
 
+void BrowserSwitcherServiceWin::UpdateAllCacheFiles() {
+  const bool has_extension = IsLBSExtensionEnabled(profile());
+
+  if (prefs().IsEnabled())
+    SavePrefsToFile();
+  else if (!has_extension)
+    DeletePrefsFile();
+
+  // Clean up sitelistcache.dat from the extension, or from a previous Chrome
+  // version.
+  if (!has_extension)
+    DeleteSitelistCacheFile();
+}
+
 }  // namespace browser_switcher

chrome/browser/browser_switcher/browser_switcher_service_win.h

@@ -48,10 +48,14 @@ class BrowserSwitcherServiceWin : public BrowserSwitcherService {
   void SavePrefsToFile();
   // Delete the "cache.dat" file created by |SavePrefsToFile()|. This call does
   // not block, it only posts a task to a worker thread.
-  void DeletePrefsFile() const;
+  void DeletePrefsFile();
   // Delete the "sitelistcache.dat" file that might be left from the LBS
   // extension, or from a previous Chrome version. Called during initialization.
-  void DeleteSitelistCacheFile() const;
+  void DeleteSitelistCacheFile();
+
+  // Updates or cleans up cache.dat and sitelistcache.dat, based on whether
+  // BrowserSwitcher is enabled or disabled.
+  void UpdateAllCacheFiles();
 
   scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;