Commit 1313bb87 authored by Mila Green's avatar Mila Green Committed by Chromium LUCI CQ

Updater: Fix NetworkFetcherMacDownloadToFile failing official builder.

There are 2 issues being fixed here:
1. base::ReplaceFile is used to move the downloaded file from the temp location to the desired file location. base::ReplaceFile fails if the temp file and the destination file are on different volumes. Using base::Move instead, as it doesn't have that limitation.
2. Getting the size of the downloaded file directly from the NSURLSessionTask instead of from the file system.

Bug: 1153304
Change-Id: I6d38a34a9893e2dfb59d605f657b42d381ec9e90
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2583338
Commit-Queue: Mila Green <milagreen@chromium.org>
Reviewed-by: default avatarSorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836347}
parent b7a0650c
...@@ -207,6 +207,9 @@ constexpr bool kUpdatePolicyDefault = kPolicyEnabled; ...@@ -207,6 +207,9 @@ constexpr bool kUpdatePolicyDefault = kPolicyEnabled;
constexpr int kUninstallPingReasonUninstalled = 0; constexpr int kUninstallPingReasonUninstalled = 0;
constexpr int kUninstallPingReasonUserNotAnOwner = 1; constexpr int kUninstallPingReasonUserNotAnOwner = 1;
// The file downloaded to a temporary location could not be moved.
constexpr int kErrorFailedToMoveDownloadedFile = 5;
} // namespace updater } // namespace updater
#endif // CHROME_UPDATER_CONSTANTS_H_ #endif // CHROME_UPDATER_CONSTANTS_H_
...@@ -26,6 +26,7 @@ source_set("network_fetcher_sources") { ...@@ -26,6 +26,7 @@ source_set("network_fetcher_sources") {
deps = [ deps = [
"//base", "//base",
"//chrome/updater:base",
"//components/crash/core/common:crash_key_lib", "//components/crash/core/common:crash_key_lib",
"//components/update_client", "//components/update_client",
"//net", "//net",
......
...@@ -19,6 +19,7 @@ ...@@ -19,6 +19,7 @@
#include "base/strings/stringprintf.h" #include "base/strings/stringprintf.h"
#include "base/strings/sys_string_conversions.h" #include "base/strings/sys_string_conversions.h"
#include "base/threading/thread_task_runner_handle.h" #include "base/threading/thread_task_runner_handle.h"
#include "chrome/updater/constants.h"
#include "chrome/updater/mac/net/network.h" #include "chrome/updater/mac/net/network.h"
#import "net/base/mac/url_conversions.h" #import "net/base/mac/url_conversions.h"
#include "url/gurl.h" #include "url/gurl.h"
...@@ -193,6 +194,7 @@ using DownloadToFileCompleteCallback = ...@@ -193,6 +194,7 @@ using DownloadToFileCompleteCallback =
@implementation CRUUpdaterNetworkDownloadDelegate { @implementation CRUUpdaterNetworkDownloadDelegate {
base::FilePath _filePath; base::FilePath _filePath;
bool _moveTempFileSuccessful;
DownloadToFileCompleteCallback _downloadToFileCompleteCallback; DownloadToFileCompleteCallback _downloadToFileCompleteCallback;
} }
...@@ -207,6 +209,7 @@ using DownloadToFileCompleteCallback = ...@@ -207,6 +209,7 @@ using DownloadToFileCompleteCallback =
initWithResponseStartedCallback:std::move(responseStartedCallback) initWithResponseStartedCallback:std::move(responseStartedCallback)
progressCallback:progressCallback]) { progressCallback:progressCallback]) {
_filePath = filePath; _filePath = filePath;
_moveTempFileSuccessful = false;
_downloadToFileCompleteCallback = std::move(downloadToFileCompleteCallback); _downloadToFileCompleteCallback = std::move(downloadToFileCompleteCallback);
} }
return self; return self;
...@@ -230,12 +233,11 @@ using DownloadToFileCompleteCallback = ...@@ -230,12 +233,11 @@ using DownloadToFileCompleteCallback =
const base::FilePath tempPath = const base::FilePath tempPath =
base::mac::NSStringToFilePath([location path]); base::mac::NSStringToFilePath([location path]);
base::File::Error fileError; _moveTempFileSuccessful = base::Move(tempPath, _filePath);
if (!base::ReplaceFile(tempPath, _filePath, &fileError)) { if (!_moveTempFileSuccessful) {
DLOG(ERROR) DPLOG(ERROR)
<< "Failed to move the downloaded file from the temporary location: " << "Failed to move the downloaded file from the temporary location: "
<< tempPath << "to: " << _filePath << tempPath << " to: " << _filePath;
<< " Error: " << base::File::ErrorToString(fileError);
} }
} }
...@@ -246,18 +248,19 @@ using DownloadToFileCompleteCallback = ...@@ -246,18 +248,19 @@ using DownloadToFileCompleteCallback =
didCompleteWithError:(NSError*)error { didCompleteWithError:(NSError*)error {
[super URLSession:session task:task didCompleteWithError:error]; [super URLSession:session task:task didCompleteWithError:error];
// TODO(crbug.com/1157996): Add handling for NSError.
NSHTTPURLResponse* response = (NSHTTPURLResponse*)task.response; NSHTTPURLResponse* response = (NSHTTPURLResponse*)task.response;
NSURL* destination = base::mac::FilePathToNSURL(_filePath); NSInteger result = response.statusCode == 200 ? 0 : response.statusCode;
NSString* filePath = [destination path];
NSDictionary<NSFileAttributeKey, id>* attributes = if (!_moveTempFileSuccessful) {
[[NSFileManager defaultManager] attributesOfItemAtPath:filePath DLOG(ERROR) << "File not moved. Original status code: "
error:nil]; << response.statusCode;
NSNumber* fileSizeAttribute = attributes[NSFileSize]; result = updater::kErrorFailedToMoveDownloadedFile;
int64_t fileSize = [fileSizeAttribute integerValue]; }
NSInteger statusCode = response.statusCode == 200 ? 0 : response.statusCode;
_callbackRunner->PostTask( _callbackRunner->PostTask(
FROM_HERE, base::BindOnce(std::move(_downloadToFileCompleteCallback), FROM_HERE, base::BindOnce(std::move(_downloadToFileCompleteCallback),
statusCode, fileSize)); result, [task countOfBytesReceived]));
} }
@end @end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment