From da543cb3896f2ee621b7ade0ee17ba566afbbfbb Mon Sep 17 00:00:00 2001 From: Daniel Brinkers Date: Tue, 20 Dec 2022 14:36:30 +0000 Subject: [PATCH] dlp: switch to IO thread to access class members Bug: 1402113 Change-Id: Iab0201f1f761c34af3973757525cc6f573745787 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4117069 Reviewed-by: Sergey Poromov Commit-Queue: Daniel Brinkers Cr-Commit-Position: refs/heads/main@{#1085416} --- .../dlp/dlp_copy_or_move_hook_delegate.cc | 79 ++++++++++--------- .../dlp/dlp_copy_or_move_hook_delegate.h | 4 - ...dlp_copy_or_move_hook_delegate_unittest.cc | 41 ++++++++++ 3 files changed, 84 insertions(+), 40 deletions(-) diff --git a/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate.cc b/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate.cc index b43861832cba4e..7ff737d4c124f7 100644 --- a/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate.cc +++ b/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate.cc @@ -34,9 +34,13 @@ void GotAccess(base::WeakPtr hook_delegate, const storage::FileSystemURL& destination, DlpCopyOrMoveHookDelegate::StatusCallback callback, std::unique_ptr access) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); bool is_allowed = access->is_allowed(); if (hook_delegate.MaybeValid()) { - hook_delegate->GotAccess(source, destination, std::move(access)); + content::GetIOThreadTaskRunner({})->PostTask( + FROM_HERE, + base::BindOnce(&DlpCopyOrMoveHookDelegate::GotAccess, hook_delegate, + source, destination, std::move(access))); } if (is_allowed) { std::move(callback).Run(base::File::FILE_OK); @@ -46,6 +50,36 @@ void GotAccess(base::WeakPtr hook_delegate, } #endif +void RequestCopyAccess(base::WeakPtr hook_delegate, + const storage::FileSystemURL& source, + const storage::FileSystemURL& destination, + DlpCopyOrMoveHookDelegate::StatusCallback callback) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + +// TODO(http://b/259183766): We might need to consider the lacros case, +// too. +#if BUILDFLAG(IS_CHROMEOS_ASH) + DlpRulesManager* dlp_rules_manager = + DlpRulesManagerFactory::GetForPrimaryProfile(); + if (!dlp_rules_manager) { + std::move(callback).Run(base::File::FILE_OK); + return; + } + DlpFilesController* dlp_files_controller = + dlp_rules_manager->GetDlpFilesController(); + if (!dlp_files_controller) { + std::move(callback).Run(base::File::FILE_OK); + return; + } + dlp_files_controller->RequestCopyAccess( + source, destination, + base::BindOnce(&policy::GotAccess, hook_delegate, source, destination, + std::move(callback))); +#else + NOTREACHED(); +#endif +} + } // namespace DlpCopyOrMoveHookDelegate::DlpCopyOrMoveHookDelegate(bool isComposite) @@ -57,6 +91,7 @@ void DlpCopyOrMoveHookDelegate::GotAccess( const storage::FileSystemURL& source, const storage::FileSystemURL& destination, std::unique_ptr access) { + DCHECK_CURRENTLY_ON(content::BrowserThread::IO); current_access_map_.emplace(std::make_pair(source.path(), destination.path()), std::move(access)); } @@ -65,12 +100,15 @@ void DlpCopyOrMoveHookDelegate::OnBeginProcessFile( const storage::FileSystemURL& source_url, const storage::FileSystemURL& destination_url, StatusCallback callback) { + DCHECK_CURRENTLY_ON(content::BrowserThread::IO); StatusCallback continuation = base::BindPostTask( base::SequencedTaskRunner::GetCurrentDefault(), std::move(callback)); content::GetUIThreadTaskRunner({})->PostTask( - FROM_HERE, base::BindOnce(&DlpCopyOrMoveHookDelegate::RequestCopyAccess, - base::Unretained(this), source_url, - destination_url, std::move(continuation))); + FROM_HERE, + base::BindOnce( + &RequestCopyAccess, + base::SupportsWeakPtr::AsWeakPtr(), + source_url, destination_url, std::move(continuation))); } void DlpCopyOrMoveHookDelegate::OnEndCopy( @@ -95,40 +133,9 @@ void DlpCopyOrMoveHookDelegate::OnError( void DlpCopyOrMoveHookDelegate::OnEnd( const storage::FileSystemURL& source_url, const storage::FileSystemURL& destination_url) { + DCHECK_CURRENTLY_ON(content::BrowserThread::IO); current_access_map_.erase( std::make_pair(source_url.path(), destination_url.path())); } -void DlpCopyOrMoveHookDelegate::RequestCopyAccess( - const storage::FileSystemURL& source, - const storage::FileSystemURL& destination, - DlpCopyOrMoveHookDelegate::StatusCallback callback) { - DCHECK_CURRENTLY_ON(content::BrowserThread::UI); - -// TODO(http://b/259183766): We might need to consider the lacros case, -// too. -#if BUILDFLAG(IS_CHROMEOS_ASH) - DlpRulesManager* dlp_rules_manager = - DlpRulesManagerFactory::GetForPrimaryProfile(); - if (!dlp_rules_manager) { - std::move(callback).Run(base::File::FILE_OK); - return; - } - DlpFilesController* dlp_files_controller = - dlp_rules_manager->GetDlpFilesController(); - if (!dlp_files_controller) { - std::move(callback).Run(base::File::FILE_OK); - return; - } - dlp_files_controller->RequestCopyAccess( - source, destination, - base::BindOnce( - &policy::GotAccess, - base::SupportsWeakPtr::AsWeakPtr(), source, - destination, std::move(callback))); -#else - NOTREACHED(); -#endif -} - } // namespace policy diff --git a/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate.h b/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate.h index 254e69a0f47f26..ef6a6f2373f7f1 100644 --- a/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate.h +++ b/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate.h @@ -50,10 +50,6 @@ class DlpCopyOrMoveHookDelegate void OnEnd(const storage::FileSystemURL& source_url, const storage::FileSystemURL& destination_url); - void RequestCopyAccess(const storage::FileSystemURL& source, - const storage::FileSystemURL& destination, - DlpCopyOrMoveHookDelegate::StatusCallback callback); - friend DlpCopyOrMoveHookDelegateTest; // Keeps the ScopedFileAccess object for the whole copy operation between the diff --git a/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate_unittest.cc b/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate_unittest.cc index c546db22c57814..558dfeceeb2266 100644 --- a/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate_unittest.cc +++ b/chrome/browser/chromeos/policy/dlp/dlp_copy_or_move_hook_delegate_unittest.cc @@ -184,6 +184,47 @@ TEST_F(DlpCopyOrMoveHookDelegateTest, OnBeginProcessFileDeny) { status_callback_run_loop.Run(); } +TEST_F(DlpCopyOrMoveHookDelegateTest, OnBeginProcessFileAllowHookDestruct) { + base::RunLoop hook_destruction_run_loop; + base::RunLoop continuation_run_loop; + base::RunLoop status_callback_run_loop; + base::MockCallback> destructor_continuation; + EXPECT_CALL(destructor_continuation, Run) + .WillOnce([&continuation_run_loop]() { continuation_run_loop.Quit(); }); + EXPECT_CALL(*manager_, GetDlpFilesController) + .WillOnce(testing::Return(controller_.get())); + + EXPECT_CALL(*controller_, RequestCopyAccess(source, destination, + base::test::IsNotNullCallback())) + .WillOnce( + [&](const storage::FileSystemURL& source, + const storage::FileSystemURL& destination, + base::OnceCallback)> callback) { + hook_.reset(); + hook_destruction_run_loop.Quit(); + std::move(callback).Run( + std::make_unique( + true, base::ScopedFD(), destructor_continuation.Get())); + }); + + auto task_runner = content::GetIOThreadTaskRunner({}); + base::MockCallback> status; + EXPECT_CALL(status, Run) + .WillOnce([&status_callback_run_loop](base::File::Error status) { + DCHECK_CURRENTLY_ON(content::BrowserThread::IO); + EXPECT_EQ(base::File::FILE_OK, status); + status_callback_run_loop.Quit(); + }); + task_runner->PostTask( + FROM_HERE, base::BindOnce(&DlpCopyOrMoveHookDelegate::OnBeginProcessFile, + base::Unretained(hook_.get()), source, + destination, status.Get())); + hook_destruction_run_loop.Run(); + status_callback_run_loop.Run(); + continuation_run_loop.Run(); +} + TEST_F(DlpCopyOrMoveHookDelegateTest, OnBeginProcessFileNoManager) { policy::DlpRulesManagerFactory::GetInstance()->SetTestingFactory( profile_.get(),