subresource_filter: fix possible file IO on main thread

subresource-filter makes use of PostTaskAndReplyWithResult()
to open a background file and then do some processing back on
the main thread. This pattern can lead to file IO on the main
thread during shutdown. In particular, if the file openning
has completed, and shutdown starts, then the result callback is
not run and the file is destroyed on the main thread. This
results in file io on the main thread during shutdown.

For whatever reason, WebLayer browser_tests on the android-p
builder seem particularly susceptible to this, and have been
disabled because they trip over it regularly.

BUG=1182000
TEST=none

Change-Id: Iefa701896e8d3a1be7b808af3c53277368ed322d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2724861
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#859139}

chrome/browser/subresource_filter/subresource_filter_browser_test_harness.cc

@@ -14,6 +14,7 @@
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/bind.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
@@ -28,6 +29,7 @@
 #include "components/subresource_filter/content/browser/ruleset_service.h"
 #include "components/subresource_filter/content/browser/subresource_filter_profile_context.h"
 #include "components/subresource_filter/content/browser/test_ruleset_publisher.h"
+#include "components/subresource_filter/content/browser/verified_ruleset_dealer.h"
 #include "components/subresource_filter/core/common/common_features.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_paths.h"
@@ -239,20 +241,19 @@ void SubresourceFilterBrowserTest::SetRulesetWithRules(
 void SubresourceFilterBrowserTest::OpenAndPublishRuleset(
     RulesetService* ruleset_service,
     const base::FilePath& indexed_ruleset_path) {
-  base::File index_file;
+  RulesetFilePtr index_file(nullptr, base::OnTaskRunnerDeleter(nullptr));
   base::RunLoop open_loop;
-  auto open_callback = base::BindOnce(
-      [](base::OnceClosure quit_closure, base::File* out, base::File result) {
-        *out = std::move(result);
-        std::move(quit_closure).Run();
-      },
-      open_loop.QuitClosure(), &index_file);
+  auto open_callback = base::BindLambdaForTesting(
+      [&index_file, &open_loop](RulesetFilePtr result) {
+        index_file = std::move(result);
+        open_loop.Quit();
+      });
   IndexedRulesetVersion version =
       ruleset_service->GetMostRecentlyIndexedVersion();
   ruleset_service->GetRulesetDealer()->TryOpenAndSetRulesetFile(
       indexed_ruleset_path, version.checksum, std::move(open_callback));
   open_loop.Run();
-  ASSERT_TRUE(index_file.IsValid());
+  ASSERT_TRUE(index_file->IsValid());
   ruleset_service->OnRulesetSet(std::move(index_file));
 }
 

components/subresource_filter/content/browser/ruleset_publisher.h

@@ -23,15 +23,16 @@ class RulesetPublisher {
   // Schedules file open and use it as ruleset file. In the case of success,
   // the new and valid |base::File| is passed to |callback|. In the case of
   // error an invalid |base::File| is passed to |callback|. The previous
-  // ruleset file will be used (if any).
+  // ruleset file will be used (if any). In either case, the supplied
+  // unique_ptr always contains a non-null |base::File|.
   virtual void TryOpenAndSetRulesetFile(
       const base::FilePath& file_path,
       int expected_checksum,
-      base::OnceCallback<void(base::File)> callback) = 0;
+      base::OnceCallback<void(RulesetFilePtr)> callback) = 0;
 
   // Redistributes the new version of the |ruleset| to all existing consumers,
   // and sets up |ruleset| to be distributed to all future consumers.
-  virtual void PublishNewRulesetVersion(base::File ruleset_data) = 0;
+  virtual void PublishNewRulesetVersion(RulesetFilePtr ruleset_data) = 0;
 
   // Task queue for best effort tasks in the thread the object was created in.
   // Used for tasks triggered on RulesetService instantiation so it doesn't

components/subresource_filter/content/browser/ruleset_publisher_impl.cc

@@ -28,22 +28,6 @@
 
 namespace subresource_filter {
 
-namespace {
-
-// The file handle is closed when the argument goes out of scope.
-void CloseFile(base::File) {}
-
-// Posts the |file| handle to the file thread so it can be closed.
-void CloseFileOnFileThread(base::File* file) {
-  if (!file->IsValid())
-    return;
-  base::ThreadPool::PostTask(
-      FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()},
-      base::BindOnce(&CloseFile, std::move(*file)));
-}
-
-}  // namespace
-
 RulesetPublisherImpl::RulesetPublisherImpl(
     RulesetService* ruleset_service,
     scoped_refptr<base::SequencedTaskRunner> blocking_task_runner)
@@ -61,9 +45,7 @@ RulesetPublisherImpl::RulesetPublisherImpl(
       content::NotificationService::AllBrowserContextsAndSources());
 }
 
-RulesetPublisherImpl::~RulesetPublisherImpl() {
-  CloseFileOnFileThread(&ruleset_data_);
-}
+RulesetPublisherImpl::~RulesetPublisherImpl() = default;
 
 void RulesetPublisherImpl::SetRulesetPublishedCallbackForTesting(
     base::OnceClosure callback) {
@@ -73,14 +55,16 @@ void RulesetPublisherImpl::SetRulesetPublishedCallbackForTesting(
 void RulesetPublisherImpl::TryOpenAndSetRulesetFile(
     const base::FilePath& file_path,
     int expected_checksum,
-    base::OnceCallback<void(base::File)> callback) {
+    base::OnceCallback<void(RulesetFilePtr)> callback) {
   GetRulesetDealer()->TryOpenAndSetRulesetFile(file_path, expected_checksum,
                                                std::move(callback));
 }
 
-void RulesetPublisherImpl::PublishNewRulesetVersion(base::File ruleset_data) {
-  DCHECK(ruleset_data.IsValid());
-  CloseFileOnFileThread(&ruleset_data_);
+void RulesetPublisherImpl::PublishNewRulesetVersion(
+    RulesetFilePtr ruleset_data) {
+  DCHECK(ruleset_data);
+  DCHECK(ruleset_data->IsValid());
+  ruleset_data_.reset();
 
   // If Ad Tagging is running, then every request does a lookup and it's
   // important that we verify the ruleset early on.
@@ -93,7 +77,7 @@ void RulesetPublisherImpl::PublishNewRulesetVersion(base::File ruleset_data) {
   ruleset_data_ = std::move(ruleset_data);
   for (auto it = content::RenderProcessHost::AllHostsIterator(); !it.IsAtEnd();
        it.Advance()) {
-    SendRulesetToRenderProcess(&ruleset_data_, it.GetCurrentValue());
+    SendRulesetToRenderProcess(ruleset_data_.get(), it.GetCurrentValue());
   }
 
   if (!ruleset_published_callback_.is_null())
@@ -121,10 +105,10 @@ void RulesetPublisherImpl::Observe(
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK_EQ(type, content::NOTIFICATION_RENDERER_PROCESS_CREATED);
-  if (!ruleset_data_.IsValid())
+  if (!ruleset_data_ || !ruleset_data_->IsValid())
     return;
   SendRulesetToRenderProcess(
-      &ruleset_data_,
+      ruleset_data_.get(),
       content::Source<content::RenderProcessHost>(source).ptr());
 }
 

components/subresource_filter/content/browser/ruleset_publisher_impl.h

@@ -41,8 +41,8 @@ class RulesetPublisherImpl : public RulesetPublisher,
   void TryOpenAndSetRulesetFile(
       const base::FilePath& file_path,
       int expected_checksum,
-      base::OnceCallback<void(base::File)> callback) override;
-  void PublishNewRulesetVersion(base::File ruleset_data) override;
+      base::OnceCallback<void(RulesetFilePtr)> callback) override;
+  void PublishNewRulesetVersion(RulesetFilePtr ruleset_data) override;
   scoped_refptr<base::SingleThreadTaskRunner> BestEffortTaskRunner() override;
   VerifiedRulesetDealer::Handle* GetRulesetDealer() override;
   void SetRulesetPublishedCallbackForTesting(
@@ -63,7 +63,7 @@ class RulesetPublisherImpl : public RulesetPublisher,
                const content::NotificationDetails& details) override;
 
   content::NotificationRegistrar notification_registrar_;
-  base::File ruleset_data_;
+  RulesetFilePtr ruleset_data_{nullptr, base::OnTaskRunnerDeleter{nullptr}};
   base::OnceClosure ruleset_published_callback_;
 
   // The service owns the publisher, and therefore outlives it.

components/subresource_filter/content/browser/ruleset_publisher_impl_unittest.cc

@@ -20,6 +20,7 @@
 #include "base/run_loop.h"
 #include "base/task_runner.h"
 #include "base/test/test_simple_task_runner.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/subresource_filter/content/browser/ruleset_service.h"
@@ -140,9 +141,10 @@ TEST_F(SubresourceFilterRulesetPublisherImplTest,
   base::WriteFile(scoped_temp_file(), kTestFileContents,
                   strlen(kTestFileContents));
 
-  base::File file;
-  file.Initialize(scoped_temp_file(),
-                  base::File::FLAG_OPEN | base::File::FLAG_READ);
+  RulesetFilePtr file(
+      new base::File(scoped_temp_file(),
+                     base::File::FLAG_OPEN | base::File::FLAG_READ),
+      base::OnTaskRunnerDeleter(base::SequencedTaskRunnerHandle::Get()));
 
   NotifyingMockRenderProcessHost existing_renderer(browser_context());
   MockClosureTarget publish_callback_target;

components/subresource_filter/content/browser/ruleset_service.cc

@@ -472,12 +472,12 @@ void RulesetService::OpenAndPublishRuleset(
       base::BindOnce(&RulesetService::OnRulesetSet, AsWeakPtr()));
 }
 
-void RulesetService::OnRulesetSet(base::File file) {
+void RulesetService::OnRulesetSet(RulesetFilePtr file) {
   // The file has just been successfully written, so a failure here is unlikely
   // unless |indexed_ruleset_base_dir_| has been tampered with or there are disk
   // errors. Still, restore the invariant that a valid version in preferences
   // always points to an existing version of disk by invalidating the prefs.
-  if (!file.IsValid()) {
+  if (!file->IsValid()) {
     IndexedRulesetVersion().SaveToPrefs(local_state_);
     return;
   }

components/subresource_filter/content/browser/ruleset_service.h

@@ -258,7 +258,7 @@ class RulesetService : public base::SupportsWeakPtr<RulesetService> {
                         const IndexedRulesetVersion& version);
 
   void OpenAndPublishRuleset(const IndexedRulesetVersion& version);
-  void OnRulesetSet(base::File file);
+  void OnRulesetSet(RulesetFilePtr file);
 
   PrefService* const local_state_;