From f47546873b1517d358815a26cebc6c4d51addbdb Mon Sep 17 00:00:00 2001 From: Asa Karlsson Date: Wed, 11 Mar 2020 10:51:08 +0100 Subject: [PATCH] Fix lifetime management of URLLoaderClient::OnUpdateProgress callback. The mojo setup for network URLLoader is a 3-way connection: URLLoader <-> CorsURLLoader <-> URLLoaderClientImpl. The network::mojom::URLLoaderClient interface has one function with a callback defined: URLLoaderClient::OnUpdateProgress. Depending on timing, this callback may be in progress while the mojo connection is torn down. If the callback (originating in URLLoader) is in transfer from CorsURLLoader to URLLoaderClientImpl at that time, the callback would be dropped (destroyed) due to Receiver (URLLoaderClientImpl) being destroyed. Since the callback was successfully handed over to CorsURLLoader first, this will lead to a mojo DCHECK for " was destroyed without first either being run or its corresponding binding being closed". By storing the callback in CorsURLLoader and calling it on mojo disconnect, running the callback is ensured also at untimely disconnects. Bug: 1060502 Change-Id: Iaedeac975508b3c0e5b4819d4f0b1a061f9c5460 --- diff --git a/services/network/cors/cors_url_loader.cc b/services/network/cors/cors_url_loader.cc index c25dded..e51d916 100644 --- a/services/network/cors/cors_url_loader.cc +++ b/services/network/cors/cors_url_loader.cc @@ -383,8 +383,20 @@ OnUploadProgressCallback ack_callback) { DCHECK(network_loader_); DCHECK(forwarding_client_); - forwarding_client_->OnUploadProgress(current_position, total_size, - std::move(ack_callback)); + + // We need to store the original callback here to allow us to call + // it during mojo disconnect in the case the OnUploadProgress has + // not reached the forwarding_client_ yet. Otherwise, the + // un-triggered callback will generate a DCHECK when destroyed. + pending_upoad_callback_ = std::move(ack_callback); + forwarding_client_->OnUploadProgress( + current_position, total_size, + base::BindOnce(&CorsURLLoader::OnUploadProgressACK, + weak_factory_.GetWeakPtr())); +} + +void CorsURLLoader::OnUploadProgressACK() { + std::move(pending_upoad_callback_).Run(); } void CorsURLLoader::OnReceiveCachedMetadata(mojo_base::BigBuffer data) { @@ -538,6 +550,8 @@ } void CorsURLLoader::OnMojoDisconnect() { + if (pending_upoad_callback_) + std::move(pending_upoad_callback_).Run(); HandleComplete(URLLoaderCompletionStatus(net::ERR_ABORTED)); } diff --git a/services/network/cors/cors_url_loader.h b/services/network/cors/cors_url_loader.h index 3f47a81..6611c6e 100644 --- a/services/network/cors/cors_url_loader.h +++ b/services/network/cors/cors_url_loader.h @@ -108,6 +108,8 @@ // Handles OnComplete() callback. void HandleComplete(const URLLoaderCompletionStatus& status); + void OnUploadProgressACK(); + void OnMojoDisconnect(); void SetCorsFlagIfNeeded(); @@ -124,6 +126,7 @@ const std::string& header_name); mojo::Receiver receiver_; + OnUploadProgressCallback pending_upoad_callback_; // We need to save these for redirect, and DevTools. const int32_t process_id_;