From 5bfbe319fb176007b837096310f787aafe5da223 Mon Sep 17 00:00:00 2001 From: Scott Violet Date: Tue, 2 Mar 2021 21:44:21 +0000 Subject: [PATCH] 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 Reviewed-by: Josh Karlin Cr-Commit-Position: refs/heads/master@{#859139} --- ...subresource_filter_browser_test_harness.cc | 17 +++--- .../content/browser/ruleset_publisher.h | 7 ++- .../content/browser/ruleset_publisher_impl.cc | 36 +++-------- .../content/browser/ruleset_publisher_impl.h | 6 +- .../ruleset_publisher_impl_unittest.cc | 8 ++- .../content/browser/ruleset_service.cc | 4 +- .../content/browser/ruleset_service.h | 2 +- .../browser/ruleset_service_unittest.cc | 61 ++++++++++++------- .../browser/verified_ruleset_dealer.cc | 17 +++--- .../content/browser/verified_ruleset_dealer.h | 21 ++++--- .../verified_ruleset_dealer_unittest.cc | 49 ++++++++++----- 11 files changed, 130 insertions(+), 98 deletions(-) diff --git a/chrome/browser/subresource_filter/subresource_filter_browser_test_harness.cc b/chrome/browser/subresource_filter/subresource_filter_browser_test_harness.cc index b0ed3d223b963..a4571e77b1d42 100644 --- a/chrome/browser/subresource_filter/subresource_filter_browser_test_harness.cc +++ b/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)); } diff --git a/components/subresource_filter/content/browser/ruleset_publisher.h b/components/subresource_filter/content/browser/ruleset_publisher.h index be5d7bb385ac1..725f6df0dc159 100644 --- a/components/subresource_filter/content/browser/ruleset_publisher.h +++ b/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 callback) = 0; + base::OnceCallback 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 diff --git a/components/subresource_filter/content/browser/ruleset_publisher_impl.cc b/components/subresource_filter/content/browser/ruleset_publisher_impl.cc index d90059f01d9a4..2d67f1a1eb85a 100644 --- a/components/subresource_filter/content/browser/ruleset_publisher_impl.cc +++ b/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 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 callback) { + base::OnceCallback 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(source).ptr()); } diff --git a/components/subresource_filter/content/browser/ruleset_publisher_impl.h b/components/subresource_filter/content/browser/ruleset_publisher_impl.h index 111ca6c98fa22..ba1d2c0cba1e5 100644 --- a/components/subresource_filter/content/browser/ruleset_publisher_impl.h +++ b/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 callback) override; - void PublishNewRulesetVersion(base::File ruleset_data) override; + base::OnceCallback callback) override; + void PublishNewRulesetVersion(RulesetFilePtr ruleset_data) override; scoped_refptr 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. diff --git a/components/subresource_filter/content/browser/ruleset_publisher_impl_unittest.cc b/components/subresource_filter/content/browser/ruleset_publisher_impl_unittest.cc index 5d93ca0ac3bad..e324e70388cec 100644 --- a/components/subresource_filter/content/browser/ruleset_publisher_impl_unittest.cc +++ b/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; diff --git a/components/subresource_filter/content/browser/ruleset_service.cc b/components/subresource_filter/content/browser/ruleset_service.cc index 98985db47809d..d4488e634b299 100644 --- a/components/subresource_filter/content/browser/ruleset_service.cc +++ b/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; } diff --git a/components/subresource_filter/content/browser/ruleset_service.h b/components/subresource_filter/content/browser/ruleset_service.h index 2e9e6dcdd1dc1..33a3176e018d5 100644 --- a/components/subresource_filter/content/browser/ruleset_service.h +++ b/components/subresource_filter/content/browser/ruleset_service.h @@ -258,7 +258,7 @@ class RulesetService : public base::SupportsWeakPtr { const IndexedRulesetVersion& version); void OpenAndPublishRuleset(const IndexedRulesetVersion& version); - void OnRulesetSet(base::File file); + void OnRulesetSet(RulesetFilePtr file); PrefService* const local_state_; diff --git a/components/subresource_filter/content/browser/ruleset_service_unittest.cc b/components/subresource_filter/content/browser/ruleset_service_unittest.cc index b8272e88a99e1..c9e73d4cfd659 100644 --- a/components/subresource_filter/content/browser/ruleset_service_unittest.cc +++ b/components/subresource_filter/content/browser/ruleset_service_unittest.cc @@ -28,6 +28,7 @@ #include "base/test/metrics/histogram_tester.h" #include "base/test/task_environment.h" #include "base/test/test_simple_task_runner.h" +#include "base/threading/sequenced_task_runner_handle.h" #include "build/build_config.h" #include "components/prefs/testing_pref_service.h" #include "components/subresource_filter/content/browser/ruleset_publisher.h" @@ -103,7 +104,7 @@ class MockRulesetPublisherImpl : public RulesetPublisher { void TryOpenAndSetRulesetFile( const base::FilePath& path, int expected_checksum, - base::OnceCallback callback) override { + base::OnceCallback callback) override { // Emulate |VerifiedRulesetDealer::Handle| behaviour: // 1. Open file on task runner. // 2. Reply with result on current thread runner. @@ -113,7 +114,7 @@ class MockRulesetPublisherImpl : public RulesetPublisher { std::move(callback)); } - void PublishNewRulesetVersion(base::File ruleset_data) override { + void PublishNewRulesetVersion(RulesetFilePtr ruleset_data) override { published_rulesets_.push_back(std::move(ruleset_data)); } @@ -126,7 +127,9 @@ class MockRulesetPublisherImpl : public RulesetPublisher { void SetRulesetPublishedCallbackForTesting( base::OnceClosure callback) override {} - std::vector& published_rulesets() { return published_rulesets_; } + std::vector& published_rulesets() { + return published_rulesets_; + } void RunBestEffortUntilIdle() { best_effort_task_runner_->RunUntilIdle(); @@ -134,12 +137,15 @@ class MockRulesetPublisherImpl : public RulesetPublisher { } private: - static base::File OpenRulesetFile(base::FilePath file_path) { - return base::File(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ | - base::File::FLAG_SHARE_DELETE); + static RulesetFilePtr OpenRulesetFile(base::FilePath file_path) { + return RulesetFilePtr( + new base::File(file_path, base::File::FLAG_OPEN | + base::File::FLAG_READ | + base::File::FLAG_SHARE_DELETE), + base::OnTaskRunnerDeleter(base::SequencedTaskRunnerHandle::Get())); } - std::vector published_rulesets_; + std::vector published_rulesets_; scoped_refptr blocking_task_runner_; scoped_refptr best_effort_task_runner_; @@ -204,6 +210,15 @@ class SubresourceFilteringRulesetServiceTest : public ::testing::Test { kTestDisallowedSuffix3, &test_ruleset_3_)); } + void TearDown() override { + // Destroy the service to schedule deletion of files. + service_.reset(); + // Run the messageloops to ensure the files are deleted. + task_environment_.RunUntilIdle(); + blocking_task_runner_->RunUntilIdle(); + ::testing::Test::TearDown(); + } + virtual void SetUpTempDir() { ASSERT_TRUE(scoped_temp_dir_.CreateUniqueTempDir()); } @@ -650,7 +665,7 @@ TEST_F(SubresourceFilteringRulesetServiceTest, ASSERT_EQ(1u, mock_publisher()->published_rulesets().size()); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[0], + mock_publisher()->published_rulesets()[0].get(), test_ruleset_1().indexed.contents)); SimulateStartupCompletedAndWaitForTasks(); @@ -667,7 +682,7 @@ TEST_F(SubresourceFilteringRulesetServiceTest, NewRuleset_Published) { ASSERT_EQ(1u, mock_publisher()->published_rulesets().size()); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[0], + mock_publisher()->published_rulesets()[0].get(), test_ruleset_1().indexed.contents)); } @@ -680,7 +695,7 @@ TEST_F(SubresourceFilteringRulesetServiceTest, ASSERT_EQ(1u, mock_publisher()->published_rulesets().size()); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[0], + mock_publisher()->published_rulesets()[0].get(), test_ruleset_1().indexed.contents)); } @@ -703,7 +718,7 @@ TEST_F(SubresourceFilteringRulesetServiceTest, ASSERT_EQ(1u, mock_publisher()->published_rulesets().size()); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[0], + mock_publisher()->published_rulesets()[0].get(), test_ruleset_1().indexed.contents)); } @@ -741,7 +756,7 @@ TEST_F(SubresourceFilteringRulesetServiceTest, NewRuleset_Persisted) { ASSERT_EQ(1u, mock_publisher()->published_rulesets().size()); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[0], + mock_publisher()->published_rulesets()[0].get(), test_ruleset_1().indexed.contents)); SimulateStartupCompletedAndWaitForTasks(); @@ -787,7 +802,7 @@ TEST_F(SubresourceFilteringRulesetServiceTest, ASSERT_EQ(1u, mock_publisher()->published_rulesets().size()); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[0], + mock_publisher()->published_rulesets()[0].get(), test_ruleset_1().indexed.contents)); } @@ -987,10 +1002,10 @@ TEST_F(SubresourceFilteringRulesetServiceTest, // can still be read after it has been deprecated. ASSERT_EQ(2u, mock_publisher()->published_rulesets().size()); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[0], + mock_publisher()->published_rulesets()[0].get(), test_ruleset_1().indexed.contents)); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[1], + mock_publisher()->published_rulesets()[1].get(), test_ruleset_2().indexed.contents)); IndexedRulesetVersion stored_version; @@ -1011,7 +1026,7 @@ TEST_F(SubresourceFilteringRulesetServiceTest, ASSERT_EQ(1u, mock_publisher()->published_rulesets().size()); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[0], + mock_publisher()->published_rulesets()[0].get(), test_ruleset_1().indexed.contents)); IndexedRulesetVersion stored_version; @@ -1037,7 +1052,7 @@ TEST_F(SubresourceFilteringRulesetServiceTest, // Make sure the active ruleset is test_ruleset_3. ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets().back(), + mock_publisher()->published_rulesets().back().get(), test_ruleset_3().indexed.contents)); } @@ -1084,7 +1099,7 @@ TEST_F(SubresourceFilteringRulesetServiceTest, ASSERT_LE(1u, mock_publisher()->published_rulesets().size()); ASSERT_GE(2u, mock_publisher()->published_rulesets().size()); if (mock_publisher()->published_rulesets().size() == 2) { - base::File* file = &mock_publisher()->published_rulesets()[0]; + base::File* file = mock_publisher()->published_rulesets()[0].get(); ASSERT_TRUE(file->IsValid()); EXPECT_THAT( ReadFileContentsToVector(file), @@ -1092,7 +1107,7 @@ TEST_F(SubresourceFilteringRulesetServiceTest, ::testing::Eq(test_ruleset_2().indexed.contents))); } ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets().back(), + mock_publisher()->published_rulesets().back().get(), test_ruleset_2().indexed.contents)); IndexedRulesetVersion stored_version; @@ -1114,8 +1129,8 @@ TEST_F(SubresourceFilteringRulesetServiceTest, RulesetIsReadonly) { kTestContentVersion1); ASSERT_EQ(1u, mock_publisher()->published_rulesets().size()); - ASSERT_NO_FATAL_FAILURE( - AssertReadonlyRulesetFile(&mock_publisher()->published_rulesets()[0])); + ASSERT_NO_FATAL_FAILURE(AssertReadonlyRulesetFile( + mock_publisher()->published_rulesets()[0].get())); } TEST_F(SubresourceFilteringRulesetServiceTest, ParallelOpenOfTwoFiles) { @@ -1143,10 +1158,10 @@ TEST_F(SubresourceFilteringRulesetServiceTest, ParallelOpenOfTwoFiles) { base::RunLoop().RunUntilIdle(); ASSERT_EQ(2u, mock_publisher()->published_rulesets().size()); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[0], + mock_publisher()->published_rulesets()[0].get(), test_ruleset_1().indexed.contents)); ASSERT_NO_FATAL_FAILURE(AssertValidRulesetFileWithContents( - &mock_publisher()->published_rulesets()[1], + mock_publisher()->published_rulesets()[1].get(), test_ruleset_2().indexed.contents)); } diff --git a/components/subresource_filter/content/browser/verified_ruleset_dealer.cc b/components/subresource_filter/content/browser/verified_ruleset_dealer.cc index 0457a7cb7ea6f..015732dcc3938 100644 --- a/components/subresource_filter/content/browser/verified_ruleset_dealer.cc +++ b/components/subresource_filter/content/browser/verified_ruleset_dealer.cc @@ -14,6 +14,7 @@ #include "base/metrics/histogram_macros.h" #include "base/notreached.h" #include "base/task_runner_util.h" +#include "base/threading/sequenced_task_runner_handle.h" #include "base/trace_event/trace_event.h" #include "components/subresource_filter/core/common/indexed_ruleset.h" #include "components/subresource_filter/core/common/memory_mapped_ruleset.h" @@ -25,19 +26,21 @@ namespace subresource_filter { VerifiedRulesetDealer::VerifiedRulesetDealer() = default; VerifiedRulesetDealer::~VerifiedRulesetDealer() = default; -base::File VerifiedRulesetDealer::OpenAndSetRulesetFile( +RulesetFilePtr VerifiedRulesetDealer::OpenAndSetRulesetFile( int expected_checksum, const base::FilePath& file_path) { DCHECK(CalledOnValidSequence()); // On Windows, open the file with FLAG_SHARE_DELETE to allow deletion while // there are handles to it still open. - base::File file(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ | - base::File::FLAG_SHARE_DELETE); + RulesetFilePtr file( + new base::File(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ | + base::File::FLAG_SHARE_DELETE), + base::OnTaskRunnerDeleter(base::SequencedTaskRunnerHandle::Get())); TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("loading"), "VerifiedRulesetDealer::OpenAndSetRulesetFile", "file_valid", - file.IsValid()); - if (file.IsValid()) { - SetRulesetFile(file.Duplicate()); + file->IsValid()); + if (file->IsValid()) { + SetRulesetFile(file->Duplicate()); expected_checksum_ = expected_checksum; } return file; @@ -110,7 +113,7 @@ void VerifiedRulesetDealer::Handle::GetDealerAsync( void VerifiedRulesetDealer::Handle::TryOpenAndSetRulesetFile( const base::FilePath& path, int expected_checksum, - base::OnceCallback callback) { + base::OnceCallback callback) { DCHECK(sequence_checker_.CalledOnValidSequence()); // |base::Unretained| is safe here because the |OpenAndSetRulesetFile| task // will be posted before a task to delete the pointer upon destruction of diff --git a/components/subresource_filter/content/browser/verified_ruleset_dealer.h b/components/subresource_filter/content/browser/verified_ruleset_dealer.h index fc8781e67c340..87549357db176 100644 --- a/components/subresource_filter/content/browser/verified_ruleset_dealer.h +++ b/components/subresource_filter/content/browser/verified_ruleset_dealer.h @@ -38,6 +38,10 @@ enum class RulesetVerificationStatus { kMaxValue = kInvalidFile, }; +// The unique_ptr ensures that the file is always closed on the proper task +// runner. See crbug.com/1182000 +using RulesetFilePtr = std::unique_ptr; + // This class is the same as RulesetDealer, but additionally does a one-time // integrity checking on the ruleset before handing it out from GetRuleset(). // @@ -58,9 +62,10 @@ class VerifiedRulesetDealer : public RulesetDealer { // Opens file and use it as ruleset file on success. Returns valid // |base::File| in the case of file opened and set. Returns invalid // |base::File| in the case of file open error. In the case of error - // ruleset dealer continues to use the previous file (if any). - base::File OpenAndSetRulesetFile(int expected_checksum, - const base::FilePath& file_path); + // ruleset dealer continues to use the previous file (if any). In both + // cases, the returned unique_ptr contains a non-null |base::File|. + RulesetFilePtr OpenAndSetRulesetFile(int expected_checksum, + const base::FilePath& file_path); // For tests only. RulesetVerificationStatus status() const { return status_; } @@ -96,10 +101,12 @@ class VerifiedRulesetDealer::Handle { // Schedules file open to use as a new 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| and dealer continues - // to use previous ruleset file (if any). - void TryOpenAndSetRulesetFile(const base::FilePath& path, - int expected_checksum, - base::OnceCallback callback); + // to use previous ruleset file (if any). In either case, the unique_ptr + // supplied to the callback contains a non-null |base::File|. + void TryOpenAndSetRulesetFile( + const base::FilePath& path, + int expected_checksum, + base::OnceCallback callback); private: // Note: Raw pointer, |dealer_| already holds a reference to |task_runner_|. diff --git a/components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc b/components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc index df7dcd6caf305..73b989c3cc572 100644 --- a/components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc +++ b/components/subresource_filter/content/browser/verified_ruleset_dealer_unittest.cc @@ -126,6 +126,7 @@ class SubresourceFilterVerifiedRulesetDealerTest : public ::testing::Test { } private: + content::BrowserTaskEnvironment task_environment_; TestRulesets rulesets_; std::unique_ptr ruleset_dealer_; base::HistogramTester histogram_tester_; @@ -308,11 +309,11 @@ TEST_F(SubresourceFilterVerifiedRulesetDealerTest, // in this manner doesn't invalidate the Flatbuffer Verifier check. testing::TestRuleset::CorruptByFilling(rulesets().indexed_1(), 28250, 28251, 32); - base::File file = ruleset_dealer()->OpenAndSetRulesetFile( + RulesetFilePtr file = ruleset_dealer()->OpenAndSetRulesetFile( /*expected_checksum=*/0, rulesets().indexed_1().path); // Check the required file is opened. - ASSERT_TRUE(file.IsValid()); + ASSERT_TRUE(file->IsValid()); // Check |OpenAndSetRulesetFile| forwards call to |SetRulesetFile| on success. EXPECT_TRUE(ruleset_dealer()->IsRulesetFileAvailable()); @@ -341,11 +342,11 @@ TEST_F(SubresourceFilterVerifiedRulesetDealerTest, // in this manner doesn't invalidate the Flatbuffer Verifier check. testing::TestRuleset::CorruptByFilling(rulesets().indexed_1(), 28250, 28251, 32); - base::File file = ruleset_dealer()->OpenAndSetRulesetFile( + RulesetFilePtr file = ruleset_dealer()->OpenAndSetRulesetFile( expected_checksum, rulesets().indexed_1().path); // Check the required file is opened. - ASSERT_TRUE(file.IsValid()); + ASSERT_TRUE(file->IsValid()); // Check |OpenAndSetRulesetFile| forwards call to |SetRulesetFile| on success. EXPECT_TRUE(ruleset_dealer()->IsRulesetFileAvailable()); @@ -365,12 +366,12 @@ TEST_F(SubresourceFilterVerifiedRulesetDealerTest, TEST_F(SubresourceFilterVerifiedRulesetDealerTest, OpenAndSetRulesetFileReturnsCorrectFileOnSuccess) { - base::File file = ruleset_dealer()->OpenAndSetRulesetFile( + RulesetFilePtr file = ruleset_dealer()->OpenAndSetRulesetFile( /*expected_checksum=*/0, rulesets().indexed_1().path); // Check the required file is opened. - ASSERT_TRUE(file.IsValid()); - EXPECT_EQ(rulesets().indexed_1().contents, ReadFileContent(&file)); + ASSERT_TRUE(file->IsValid()); + EXPECT_EQ(rulesets().indexed_1().contents, ReadFileContent(file.get())); // Check |OpenAndSetRulesetFile| forwards call to |SetRulesetFile| on success. EXPECT_TRUE(ruleset_dealer()->IsRulesetFileAvailable()); @@ -382,11 +383,11 @@ TEST_F(SubresourceFilterVerifiedRulesetDealerTest, TEST_F(SubresourceFilterVerifiedRulesetDealerTest, OpenAndSetRulesetFileReturnsNullFileOnFailure) { - base::File file = ruleset_dealer()->OpenAndSetRulesetFile( + RulesetFilePtr file = ruleset_dealer()->OpenAndSetRulesetFile( /*expected_checksum=*/0, base::FilePath::FromUTF8Unsafe("non_existent_file")); - EXPECT_FALSE(file.IsValid()); + EXPECT_FALSE(file->IsValid()); EXPECT_FALSE(ruleset_dealer()->IsRulesetFileAvailable()); histogram_tester().ExpectTotalCount(kVerificationHistogram, 0); } @@ -461,6 +462,14 @@ class SubresourceFilterVerifiedRulesetDealerHandleTest rulesets_.CreateRulesets(false /* many_rules */); task_runner_ = new base::TestSimpleTaskRunner; } + void TearDown() override { + // This is needed to ensure the File created by VerifiedRulesetDealer is + // correctly destroyed (it's destroyed on `task_runner_` through a + // PostTask). + task_environment_.RunUntilIdle(); + task_runner_->RunUntilIdle(); + ::testing::Test::TearDown(); + } const TestRulesets& rulesets() const { return rulesets_; } base::TestSimpleTaskRunner* task_runner() { return task_runner_.get(); } @@ -540,8 +549,9 @@ TEST_F(SubresourceFilterVerifiedRulesetDealerHandleTest, dealer_handle->TryOpenAndSetRulesetFile( base::FilePath::FromUTF8Unsafe("non_existent_file"), - /*expected_checksum=*/0, - base::BindOnce([](base::File file) { EXPECT_FALSE(file.IsValid()); })); + /*expected_checksum=*/0, base::BindOnce([](RulesetFilePtr file) { + EXPECT_FALSE(file->IsValid()); + })); dealer_handle->GetDealerAsync(after_set_ruleset_2.GetCallback()); dealer_handle->GetDealerAsync(read_ruleset_2.GetCallback()); dealer_handle.reset(nullptr); @@ -610,7 +620,12 @@ class SubresourceFilterVerifiedRulesetHandleTest : public ::testing::Test { void TearDown() override { dealer_handle_.reset(nullptr); + // This is needed to ensure the File created by VerifiedRulesetDealer is + // correctly destroyed (it's destroyed on `task_runner_` through a + // PostTask). + task_environment_.RunUntilIdle(); task_runner_->RunUntilIdle(); + ::testing::Test::TearDown(); } const TestRulesets& rulesets() const { return rulesets_; } @@ -642,7 +657,8 @@ TEST_F(SubresourceFilterVerifiedRulesetHandleTest, dealer_handle()->TryOpenAndSetRulesetFile( rulesets().indexed_1().path, /*expected_checksum=*/0, - base::BindOnce([](base::File file) { EXPECT_TRUE(file.IsValid()); })); + base::BindOnce( + [](RulesetFilePtr file) { EXPECT_TRUE(file->IsValid()); })); auto ruleset_handle = CreateRulesetHandle(); dealer_handle()->GetDealerAsync(created_handle.GetCallback()); @@ -667,7 +683,8 @@ TEST_F(SubresourceFilterVerifiedRulesetHandleTest, dealer_handle()->TryOpenAndSetRulesetFile( rulesets().indexed_1().path, /*expected_checksum=*/0, - base::BindOnce([](base::File file) { EXPECT_TRUE(file.IsValid()); })); + base::BindOnce( + [](RulesetFilePtr file) { EXPECT_TRUE(file->IsValid()); })); auto ruleset_handle_1 = CreateRulesetHandle(); auto ruleset_handle_2 = CreateRulesetHandle(); @@ -710,7 +727,8 @@ TEST_F(SubresourceFilterVerifiedRulesetHandleTest, dealer_handle()->TryOpenAndSetRulesetFile( rulesets().indexed_1().path, /*expected_checksum=*/0, - base::BindOnce([](base::File file) { EXPECT_TRUE(file.IsValid()); })); + base::BindOnce( + [](RulesetFilePtr file) { EXPECT_TRUE(file->IsValid()); })); auto ruleset_handle_1 = CreateRulesetHandle(); dealer_handle()->GetDealerAsync(created_handle_1.GetCallback()); @@ -759,7 +777,8 @@ TEST_F(SubresourceFilterVerifiedRulesetHandleTest, testing::TestRuleset::CorruptByTruncating(rulesets().indexed_1(), 4096); dealer_handle()->TryOpenAndSetRulesetFile( rulesets().indexed_1().path, /*expected_checksum=*/0, - base::BindOnce([](base::File file) { EXPECT_TRUE(file.IsValid()); })); + base::BindOnce( + [](RulesetFilePtr file) { EXPECT_TRUE(file->IsValid()); })); auto ruleset_handle = CreateRulesetHandle(); dealer_handle()->GetDealerAsync(created_handle.GetCallback());