Skip to content

Commit

Permalink
Refer to Delegate via weak pointers.
Browse files Browse the repository at this point in the history 
In order to facilitate it, moved all references to the weak pointers to
a singleton task runner
`FileUploadJob::Manager::GetInstance()->sequenced_task_runner()`
Deletion of Delegate is also done there by calling `DeleteSoon`.

Additionally, modified FileUploadDelegate to use `ActionContext` and its
subclasses with `base::Unretained(this)` instead of weak pointers, since
it can only be destruct via call to `Complete` method, and so all
asynchronous callbacks are safe to be called like this.

Bug: b:264399295
Change-Id: I5a55afeab428766bd8a93f224b713f7f8d9d3dd5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4374314
Reviewed-by: Vignesh Shenvi <vshenvi@google.com>
Commit-Queue: Leonid Baraz <lbaraz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1123906}
  • Loading branch information
Leonid Baraz authored and Chromium LUCI CQ committed Mar 29, 2023
1 parent d3d3378 commit 629cd9a
Show file tree
Hide file tree
Showing 8 changed files with 269 additions and 189 deletions.
85 changes: 49 additions & 36 deletions chrome/browser/policy/messaging_layer/upload/file_upload_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ class ActionContext {
: delegate_(std::move(delegate)), result_cb_(std::move(result_cb)) {}

// Completes work returning result or status, and then self-destructs.
// This is the only way `ActionContext` ceases to exist, so any asynchronous
// callback in its subclasses is safe to use `base::Unretained(this)` and
// does not need weak pointers.
void Complete(R result) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(result_cb_) << "Already completed";
Expand Down Expand Up @@ -155,7 +158,10 @@ class FileUploadDelegate::AccessTokenRetriever

void RequestAccessToken() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(delegate());
if (!delegate()) {
Complete(Status(error::UNAVAILABLE, "Delegate is unavailable"));
return;
}

DCHECK(!access_token_request_);
DVLOG(1) << "Requesting access token.";
Expand Down Expand Up @@ -187,10 +193,6 @@ class FileUploadDelegate::AccessTokenRetriever
// The OAuth request to receive the access token.
std::unique_ptr<OAuth2AccessTokenManager::Request> access_token_request_
GUARDED_BY_CONTEXT(sequence_checker_);

// Should remain the last member so it will be destroyed first and
// invalidate all weak pointers.
base::WeakPtrFactory<AccessTokenRetriever> weak_ptr_factory_{this};
};

// Self-destructing context for FileUploadJob initiation.
Expand All @@ -213,20 +215,25 @@ class FileUploadDelegate::InitContext

void Run() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(delegate());
if (!delegate()) {
Complete(Status(error::UNAVAILABLE, "Delegate is unavailable"));
return;
}

// Perform file operation on a thread pool, then resume on the current task
// runner.
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()},
base::BindOnce(&InitContext::InitFile, origin_path_),
base::BindOnce(&InitContext::FileOpened,
weak_ptr_factory_.GetWeakPtr()));
base::BindOnce(&InitContext::FileOpened, base::Unretained(this)));
}

void FileOpened(StatusOr<int64_t> total_result) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(delegate());
if (!delegate()) {
Complete(Status(error::UNAVAILABLE, "Delegate is unavailable"));
return;
}

if (!total_result.ok()) {
Complete(total_result.status());
Expand Down Expand Up @@ -283,7 +290,7 @@ class FileUploadDelegate::InitContext
// Make a call and get response headers.
delegate()->SendAndGetResponse(
url_loader_.get(), base::BindOnce(&InitContext::OnInitURLLoadComplete,
weak_ptr_factory_.GetWeakPtr()));
base::Unretained(this)));
}

void OnInitURLLoadComplete(
Expand Down Expand Up @@ -348,10 +355,6 @@ class FileUploadDelegate::InitContext

// Total size.
int64_t total_ GUARDED_BY_CONTEXT(sequence_checker_) = 0L;

// Should remain the last member so it will be destroyed first and
// invalidate all weak pointers.
base::WeakPtrFactory<InitContext> weak_ptr_factory_{this};
};

// Self-destructing context for FileUploadJob next step.
Expand All @@ -376,7 +379,10 @@ class FileUploadDelegate::NextStepContext

void Run() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(delegate());
if (!delegate()) {
Complete(Status(error::UNAVAILABLE, "Delegate is unavailable"));
return;
}

// Parse session token.
const auto tokens = base::SplitStringPiece(
Expand Down Expand Up @@ -407,13 +413,16 @@ class FileUploadDelegate::NextStepContext
delegate()->SendAndGetResponse(
url_loader_.get(),
base::BindOnce(&NextStepContext::OnQueryURLLoadComplete,
weak_ptr_factory_.GetWeakPtr()));
base::Unretained(this)));
}

void OnQueryURLLoadComplete(
scoped_refptr<::net::HttpResponseHeaders> headers) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(delegate());
if (!delegate()) {
Complete(Status(error::UNAVAILABLE, "Delegate is unavailable"));
return;
}

auto status_result =
CheckResponseAndGetStatus(std::move(url_loader_), headers);
Expand Down Expand Up @@ -507,9 +516,8 @@ class FileUploadDelegate::NextStepContext
base::BindOnce(&NextStepContext::LoadFileData,
std::string(origin_path_), total_, upload_received,
size),
base::BindOnce(&NextStepContext::PerformUpload,
weak_ptr_factory_.GetWeakPtr(), upload_received, size,
std::move(resource_request)));
base::BindOnce(&NextStepContext::PerformUpload, base::Unretained(this),
upload_received, size, std::move(resource_request)));
}

void PerformUpload(
Expand All @@ -518,7 +526,10 @@ class FileUploadDelegate::NextStepContext
std::unique_ptr<::network::ResourceRequest> resource_request,
StatusOr<std::string> buffer_result) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(delegate());
if (!delegate()) {
Complete(Status(error::UNAVAILABLE, "Delegate is unavailable"));
return;
}

if (!buffer_result.ok()) {
Complete(buffer_result.status());
Expand All @@ -534,7 +545,7 @@ class FileUploadDelegate::NextStepContext
delegate()->SendAndGetResponse(
url_loader_.get(),
base::BindOnce(&NextStepContext::OnUploadURLLoadComplete,
weak_ptr_factory_.GetWeakPtr(), upload_received, size));
base::Unretained(this), upload_received, size));
}

void OnUploadURLLoadComplete(
Expand Down Expand Up @@ -632,10 +643,6 @@ class FileUploadDelegate::NextStepContext

// Memory usage by upload.
ScopedReservation scoped_reservation_ GUARDED_BY_CONTEXT(sequence_checker_);

// Should remain the last member so it will be destroyed first and
// invalidate all weak pointers.
base::WeakPtrFactory<NextStepContext> weak_ptr_factory_{this};
};

// Self-destructing context for FileUploadJob finalization.
Expand All @@ -652,7 +659,10 @@ class FileUploadDelegate::FinalContext

void Run() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(delegate());
if (!delegate()) {
Complete(Status(error::UNAVAILABLE, "Delegate is unavailable"));
return;
}

// Parse session token.
const auto tokens = base::SplitStringPiece(
Expand Down Expand Up @@ -682,13 +692,16 @@ class FileUploadDelegate::FinalContext
// Make a call and get response headers.
delegate()->SendAndGetResponse(
url_loader_.get(), base::BindOnce(&FinalContext::OnQueryURLLoadComplete,
weak_ptr_factory_.GetWeakPtr()));
base::Unretained(this)));
}

void OnQueryURLLoadComplete(
scoped_refptr<::net::HttpResponseHeaders> headers) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(delegate());
if (!delegate()) {
Complete(Status(error::UNAVAILABLE, "Delegate is unavailable"));
return;
}

auto status_result =
CheckResponseAndGetStatus(std::move(url_loader_), headers);
Expand Down Expand Up @@ -739,7 +752,7 @@ class FileUploadDelegate::FinalContext
delegate()->SendAndGetResponse(
url_loader_.get(),
base::BindOnce(&FinalContext::OnFinalizeURLLoadComplete,
weak_ptr_factory_.GetWeakPtr()));
base::Unretained(this)));
}

void OnFinalizeURLLoadComplete(
Expand Down Expand Up @@ -787,15 +800,15 @@ class FileUploadDelegate::FinalContext
// Helper to upload the data.
std::unique_ptr<network::SimpleURLLoader> url_loader_
GUARDED_BY_CONTEXT(sequence_checker_);

// Should remain the last member so it will be destroyed first and
// invalidate all weak pointers.
base::WeakPtrFactory<FinalContext> weak_ptr_factory_{this};
};

FileUploadDelegate::FileUploadDelegate() = default;
FileUploadDelegate::FileUploadDelegate() {
DETACH_FROM_SEQUENCE(sequence_checker_);
}

FileUploadDelegate::~FileUploadDelegate() = default;
FileUploadDelegate::~FileUploadDelegate() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
}

void FileUploadDelegate::InitializeOnce() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
Expand Down
5 changes: 2 additions & 3 deletions chrome/browser/policy/messaging_layer/upload/file_upload_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,6 @@ class FakeOAuth2AccessTokenManagerDelegate
return CoreAccountId(kRobotAccountId) == account_id;
}
};

} // namespace

class FileUploadDelegateTest : public ::testing::Test {
Expand Down Expand Up @@ -953,8 +952,8 @@ TEST_F(FileUploadDelegateTest, FinishFailures) {
{
test::TestEvent<StatusOr<std::string /*access_parameters*/>> finish_done;
delegate->DoFinalize(
/*session_token=*/
base::StrCat({origin_path(), "\n", GetServerURL(kResumableUrl).spec()}),
/*session_token=*/base::StrCat(
{origin_path(), "\n", GetServerURL(kResumableUrl).spec()}),
finish_done.cb());
const auto& result = finish_done.result();
ASSERT_THAT(
Expand Down
Loading

0 comments on commit 629cd9a

Please sign in to comment.