From f47546873b1517d358815a26cebc6c4d51addbdb Mon Sep 17 00:00:00 2001
From: Asa Karlsson <asaka@vewd.com>
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 "<callback>
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<mojom::URLLoader> receiver_;
+  OnUploadProgressCallback pending_upoad_callback_;
 
   // We need to save these for redirect, and DevTools.
   const int32_t process_id_;