Download Feedback Service should upload all eligible downloads

content/browser/download/download_item_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // File method ordering: Methods in this file are in the same order as
 // in download_item_impl.h, with the following exception: The public
 // interface Start is placed in chronological order with the other
 // (private) routines that together define a DownloadItem's state
 // transitions as the download progresses.  See "Download progression
 // cascade" later in this file.
@@ skipping 75 matching lines @@
 // takes ownership of the DownloadFile and hence implicitly destroys it
 // at the end of the function.
 static base::FilePath DownloadFileDetach(
     std::unique_ptr<DownloadFile> download_file) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   base::FilePath full_path = download_file->FullPath();
   download_file->Detach();
   return full_path;
 }
 
+static base::FilePath MakeCopyOfDownloadFile(
+  DownloadFile* download_file) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  base::FilePath temp_file_path;
+  if(base::CreateTemporaryFile(&temp_file_path) &&
+     base::CopyFile(download_file->FullPath(), temp_file_path))
+    return temp_file_path;
+  else
+    return base::FilePath();

[asanka, 2016/11/01 21:09:23]

Can this branch delete the file at |temp_file_path| if |temp_file_path| contains
a valid path? 

base::CopyFile may leave a partial file around if it fails part-way. Leaving
those files around can be harmful in some cases. E.g. if CopyFile failed because
the temporary file location ran out of space lots of things will go wrong
quickly unless we free up the space again.

Also FYI the download file and the temp file may end up on different storage
devices and may be subject to different size restrictions.

[Jialiu Lin, 2016/11/03 20:16:50]

Done.

+}
+
 static void DownloadFileCancel(std::unique_ptr<DownloadFile> download_file) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   download_file->Cancel();
 }
 
 }  // namespace
 
 const uint32_t DownloadItem::kInvalidId = 0;
 
 // The maximum number of attempts we will make to resume automatically.
@@ skipping 186 matching lines @@
       base::Bind(&ItemCheckedNetLogCallback, GetDangerType()));
 
   UpdateObservers();  // TODO(asanka): This is potentially unsafe. The download
                       // may not be in a consistent state or around at all after
                       // invoking observers. http://crbug.com/586610
 
   MaybeCompleteDownload();
 }
 
 void DownloadItemImpl::StealDangerousDownload(
+    bool delete_file_afterward,
     const AcquireFileCallback& callback) {
   DVLOG(20) << __func__ << "() download = " << DebugString(true);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(IsDangerous());

[asanka, 2016/11/01 20:58:58]

Can you also DCHECK AllDataSaved() ?

It appears that the logic below will only have the desired effect if
AllDataSaved() is true. I.e. the file is complete.

[Jialiu Lin, 2016/11/03 20:16:50]

Done.

 
-  if (download_file_) {
+  if (delete_file_afterward) {
+    if (download_file_){
+      BrowserThread::PostTaskAndReplyWithResult(
+          BrowserThread::FILE,
+          FROM_HERE,
+          base::Bind(&DownloadFileDetach, base::Passed(&download_file_)),
+          callback);
+    } else {
+      callback.Run(current_path_);
+    }
+    current_path_.clear();
+    Remove();
+    // We have now been deleted.
+  } else if (download_file_) {

[asanka, 2016/11/01 20:58:58]

If there's no download_file_, then you can callback.Run(current_path_).
Otherwise that branch will cause the callback to never be run. All branches must
invoke |callback| at some point. Otherwise the download will stall.

[Jialiu Lin, 2016/11/03 20:16:50]

Done.

     BrowserThread::PostTaskAndReplyWithResult(
         BrowserThread::FILE,
         FROM_HERE,
-        base::Bind(&DownloadFileDetach, base::Passed(&download_file_)),
+        base::Bind(&MakeCopyOfDownloadFile, download_file_.get()),
         callback);
-  } else {
-    callback.Run(current_path_);
   }
-  current_path_.clear();
-  Remove();
-  // We have now been deleted.
 }
 
 void DownloadItemImpl::Pause() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // Ignore irrelevant states.
   if (is_paused_)
     return;
 
   switch (state_) {
@@ skipping 1039 matching lines @@
 // downloads. SavePackage always uses its own Finish() to mark downloads
 // complete.
 void DownloadItemImpl::MaybeCompleteDownload() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!is_save_package_download_);
 
   if (!IsDownloadReadyForCompletion(
           base::Bind(&DownloadItemImpl::MaybeCompleteDownload,
                      weak_ptr_factory_.GetWeakPtr())))
     return;
-
   // Confirm we're in the proper set of states to be here; have all data, have a
   // history handle, (validated or safe).
   DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
   DCHECK(!IsDangerous());
   DCHECK(all_data_saved_);
 
   OnDownloadCompleting();
 }
 
 // Called by MaybeCompleteDownload() when it has determined that the download
@@ skipping 704 matching lines @@
     case RESUME_MODE_USER_CONTINUE:
       return "USER_CONTINUE";
     case RESUME_MODE_USER_RESTART:
       return "USER_RESTART";
   }
   NOTREACHED() << "Unknown resume mode " << mode;
   return "unknown";
 }
 
 }  // namespace content