Bug 1254327 (Part 1) Add filename callback to DownloadTarget r?paolo draft
authorAndrew Swan <aswan@mozilla.com>
Tue, 29 Mar 2016 13:36:31 -0700
changeset 345690 300a334f49a62653cc9513dc830ebc3f3ce894b3
parent 345629 39b92bd043997494e37c13943eeea480b6339607
child 345691 a6ccd1e564097780b1bd8d8b65dda32182c2d64b
push id14141
push useraswan@mozilla.com
push dateWed, 30 Mar 2016 03:01:53 +0000
reviewerspaolo
bugs1254327
milestone48.0a1
Bug 1254327 (Part 1) Add filename callback to DownloadTarget r?paolo MozReview-Commit-ID: 7vIqUIdJWm7
toolkit/components/jsdownloads/src/DownloadCore.jsm
toolkit/components/jsdownloads/src/DownloadList.jsm
toolkit/components/jsdownloads/test/unit/common_test_Download.js
toolkit/components/jsdownloads/test/unit/test_DownloadList.js
--- a/toolkit/components/jsdownloads/src/DownloadCore.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ -1092,18 +1092,19 @@ this.Download.prototype = {
   toSerializable: function ()
   {
     let serializable = {
       source: this.source.toSerializable(),
       target: this.target.toSerializable(),
     };
 
     let saver = this.saver.toSerializable();
-    if (!saver) {
-      // If we are unable to serialize the saver, we won't persist the download.
+    if (!serializable.target || !saver) {
+      // If we are unable to serialize the target or saver,
+      // we won't persist the download.
       return null;
     }
 
     // Simplify the representation for the most common saver type.  If the saver
     // is an object instead of a simple string, we can't simplify it because we
     // need to persist all its properties, not only "type".  This may happen for
     // savers of type "copy" as well as other types.
     if (saver !== "copy") {
@@ -1387,16 +1388,40 @@ this.DownloadTarget.prototype = {
    *
    * This is a dynamic property updated when the download finishes or when the
    * "refresh" method of the Download object is called. It can be used by the
    * front-end to reduce I/O compared to checking the target file directly.
    */
   size: 0,
 
   /**
+   * Callback to establish the target filename.
+   *
+   * If null, the DownloadTarget should already have the final path
+   * (and optional partFilePath).  If non-null, should be a callable
+   * that takes an nsIRequest and returns a promise that resolves with
+   * the target filename.  The callback will be invoked upon getting
+   * onStartRequest from the underlying channel, so any redirects will
+   * have already been handled and the callback can consult the actual
+   * URI being downloaded, can check for HTTP errors, etc.  Note that even
+   * if callback is set, path must also be set to the directory where the
+   * download target will be saved while the filename callback is pending.
+   */
+  _finalizeFilename: null,
+
+  /**
+   * Indicates if the current target path has been finalized.
+   *
+   * When a finalize callback is being used (see above), this property is
+   * true until the callback has completed and the actual target has been
+   * established, at which points it becomes false.
+   */
+  isTempFile: false,
+
+  /**
    * Sets the "exists" and "size" properties based on the actual file on disk.
    *
    * @return {Promise}
    * @resolves When the operation has finished successfully.
    * @rejects JavaScript exception.
    */
   refresh: Task.async(function* () {
     try {
@@ -1408,22 +1433,41 @@ this.DownloadTarget.prototype = {
       if (!(ex instanceof OS.File.Error && ex.becauseNoSuchFile)) {
         Cu.reportError(ex);
       }
       this.exists = false;
     }
   }),
 
   /**
+   * Returns a boolean indicating whether this target is finalized.
+   *
+   * If this target has a finalize callback that has not yet been called
+   * or if that callback is still pending, then this target is not yet
+   * serializable and this method returns false.  In all other cases, the
+   * target file has been determined and will not change any more, so the
+   * target is serializable.
+   */
+  isSerializable: function() {
+    return (this._finalizeFilename == null && !this.isTempFile);
+  },
+
+  /**
    * Returns a static representation of the current object state.
    *
    * @return A JavaScript object that can be serialized to JSON.
    */
   toSerializable: function ()
   {
+    // If there is an unresolved filename callback,
+    // this target is not serialiazable.
+    if (!this.isSerializable()) {
+      return null;
+    }
+
     // Simplify the representation if we don't have other details.
     if (!this.partFilePath && !this._unknownProperties) {
       return this.path;
     }
 
     let serializable = { path: this.path,
                          partFilePath: this.partFilePath };
     serializeUnknownProperties(this, serializable);
@@ -1455,16 +1499,19 @@ this.DownloadTarget.fromSerializable = f
     target.path = aSerializable.path;
   } else {
     // Read the "path" property of the serializable DownloadTarget
     // representation, converting String objects to primitive strings.
     target.path = aSerializable.path.toString();
     if ("partFilePath" in aSerializable) {
       target.partFilePath = aSerializable.partFilePath;
     }
+    if ("filenameCallback" in aSerializable) {
+      target._finalizeFilename = aSerializable.filenameCallback;
+    }
 
     deserializeUnknownProperties(target, aSerializable, property =>
       property != "path" && property != "partFilePath");
   }
   return target;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1885,49 +1932,52 @@ this.DownloadCopySaver.prototype = {
     this._canceled = false;
 
     let download = this.download;
     let targetPath = download.target.path;
     let partFilePath = download.target.partFilePath;
     let keepPartialData = download.tryToKeepPartialData;
 
     return Task.spawn(function* task_DCS_execute() {
-      // Add the download to history the first time it is started in this
-      // session.  If the download is restarted in a different session, a new
-      // history visit will be added.  We do this just to avoid the complexity
-      // of serializing this state between sessions, since adding a new visit
-      // does not have any noticeable side effect.
-      if (!this.alreadyAddedToHistory) {
-        this.addToHistory();
-        this.alreadyAddedToHistory = true;
-      }
+      if (!download.target._finalizeFilename) {
+        // Add the download to history the first time it is started in this
+        // session.  If the download is restarted in a different session, a new
+        // history visit will be added.  We do this just to avoid the complexity
+        // of serializing this state between sessions, since adding a new visit
+        // does not have any noticeable side effect.
+        if (!this.alreadyAddedToHistory) {
+          this.addToHistory();
+          this.alreadyAddedToHistory = true;
+        }
 
-      // To reduce the chance that other downloads reuse the same final target
-      // file name, we should create a placeholder as soon as possible, before
-      // starting the network request.  The placeholder is also required in case
-      // we are using a ".part" file instead of the final target while the
-      // download is in progress.
-      try {
-        // If the file already exists, don't delete its contents yet.
-        let file = yield OS.File.open(targetPath, { write: true });
-        yield file.close();
-      } catch (ex) {
-        if (!(ex instanceof OS.File.Error)) {
-          throw ex;
+        // To reduce the chance that other downloads reuse the same final target
+        // file name, we should create a placeholder as soon as possible, before
+        // starting the network request.  The placeholder is also required in
+        // case we are using a ".part" file instead of the final target while
+        // the download is in progress.
+        try {
+          // If the file already exists, don't delete its contents yet.
+          let file = yield OS.File.open(targetPath, { write: true });
+          yield file.close();
+        } catch (ex) {
+          if (!(ex instanceof OS.File.Error)) {
+            throw ex;
+          }
+          // Throw a DownloadError indicating that the operation failed because of
+          // the target file.  We cannot translate this into a specific result
+          // code, but we preserve the original message using the toString method.
+          let error = new DownloadError({ message: ex.toString() });
+          error.becauseTargetFailed = true;
+          throw error;
         }
-        // Throw a DownloadError indicating that the operation failed because of
-        // the target file.  We cannot translate this into a specific result
-        // code, but we preserve the original message using the toString method.
-        let error = new DownloadError({ message: ex.toString() });
-        error.becauseTargetFailed = true;
-        throw error;
       }
 
       try {
         let deferSaveComplete = Promise.defer();
+        let filenamePromise = Promise.resolve();
 
         if (this._canceled) {
           // Don't create the BackgroundFileSaver object if we have been
           // canceled meanwhile.
           throw new DownloadError({ message: "Saver canceled." });
         }
 
         // Create the object that will save the file in a background thread.
@@ -2066,31 +2116,65 @@ this.DownloadCopySaver.prototype = {
                   keepPartialData = false;
                 }
               }
 
               // Enable hashing and signature verification before setting the
               // target.
               backgroundFileSaver.enableSha256();
               backgroundFileSaver.enableSignatureInfo();
-              if (partFilePath) {
-                // If we actually resumed a request, append to the partial data.
-                if (resumeAttempted) {
-                  // TODO: Handle Cr.NS_ERROR_ENTITY_CHANGED
-                  backgroundFileSaver.enableAppend();
+
+              let targetFile;
+              if (download.target._finalizeFilename) {
+                aRequest.QueryInterface(Ci.nsIChannel);
+                let uri = aRequest.URI;
+
+                // target.path must be the download dir in this case
+                let tmpfile = new FileUtils.File(targetPath);
+                if (!tmpfile.isDirectory()) {
+                  tmpfile = tmpfile.parent;
                 }
+                uri.QueryInterface(Ci.nsIURL);
+                tmpfile.append("inprogress." + uri.fileExtension);
+                tmpfile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
+
+                targetFile = tmpfile;
 
-                // Use a part file, determining if we should keep it on failure.
-                backgroundFileSaver.setTarget(new FileUtils.File(partFilePath),
-                                              keepPartialData);
+                download.target.isTempFile = true;
+                let fn = download.target._finalizeFilename;
+                download.target._finalizeFilename = null;
+                filenamePromise = fn(aRequest).then(filename => {
+                  download.target.path = filename;
+                  download.target.isTempFile = false;
+                  if (keepPartialData) {
+                    download.target.partFilePath = filename + ".part";
+                    OS.File.move(tmpfile.path, download.target.partFilePath);
+                  } else {
+                    OS.File.move(tmpfile.path, download.target.path);
+                  }
+
+                  if (!this.alreadyAddedToHistory) {
+                    this.addToHistory();
+                    this.alreadyAddedToHistory = true;
+                  }
+                });
               } else {
-                // Set the final target file, and delete it on failure.
-                backgroundFileSaver.setTarget(new FileUtils.File(targetPath),
-                                              false);
+                if (partFilePath) {
+                  // If we actually resumed a request, append to the partial data.
+                  if (resumeAttempted) {
+                    // TODO: Handle Cr.NS_ERROR_ENTITY_CHANGED
+                    backgroundFileSaver.enableAppend();
+                  }
+                  targetFile = new FileUtils.File(partFilePath);
+                } else {
+                  targetFile = new FileUtils.File(targetPath);
+                  keepPartialData = false;
+                }
               }
+              backgroundFileSaver.setTarget(targetFile, keepPartialData);
             }.bind(copySaver),
 
             onStopRequest: function (aRequest, aContext, aStatusCode) {
               try {
                 backgroundFileSaver.onStopRequest(aRequest, aContext,
                                                   aStatusCode);
               } finally {
                 // If the data transfer completed successfully, indicate to the
@@ -2128,16 +2212,20 @@ this.DownloadCopySaver.prototype = {
           deferSaveComplete.promise.catch(() => {});
           throw ex;
         }
 
         // We will wait on this promise in case no error occurred while setting
         // up the chain of objects for the download.
         yield deferSaveComplete.promise;
 
+        // If we have a filename callback, make sure it is finished before
+        // we proceed.
+        yield filenamePromise;
+
         yield this._checkReputationAndMove();
       } catch (ex) {
         // Ensure we always remove the placeholder for the final target file on
         // failure, independently of which code path failed.  In some cases, the
         // background file saver may have already removed the file.
         try {
           yield OS.File.remove(targetPath);
         } catch (e2) {
--- a/toolkit/components/jsdownloads/src/DownloadList.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadList.jsm
@@ -85,16 +85,19 @@ this.DownloadList.prototype = {
    * @param aDownload
    *        The Download object to add.
    *
    * @return {Promise}
    * @resolves When the download has been added.
    * @rejects JavaScript exception.
    */
   add: function DL_add(aDownload) {
+    if (!aDownload.target.isSerializable()) {
+      throw new Error("Cannot add download since its target is not yet serializable");
+    }
     this._downloads.push(aDownload);
     aDownload.onchange = this._change.bind(this, aDownload);
     this._notifyAllViews("onDownloadAdded", aDownload);
 
     return Promise.resolve();
   },
 
   /**
--- a/toolkit/components/jsdownloads/test/unit/common_test_Download.js
+++ b/toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ -2318,8 +2318,88 @@ add_task(function* test_launchWhenSuccee
 
   // Simulate browser shutdown
   let expire = Cc["@mozilla.org/uriloader/external-helper-app-service;1"]
                  .getService(Ci.nsIObserver);
   expire.observe(null, "profile-before-change", null);
   do_check_false(yield OS.File.exists(autoDeleteTargetPathTwo));
   do_check_true(yield OS.File.exists(noAutoDeleteTargetPath));
 });
+
+function makeResolver(fn) {
+  let resolve;
+  let promise = new Promise(r => { resolve = r; });
+
+  return {
+    promise,
+    callback: request => {
+      resolve();
+      request.QueryInterface(Ci.nsIHttpChannel);
+      return fn(request.URI);
+    }
+  };
+}
+
+/**
+ * Test using a filename callback to set the filename
+ */
+add_task(function* test_filenameCallback() {
+  // filename callback only works with DownloadCore
+  if (gUseLegacySaver) {
+    return;
+  }
+
+  let url = httpUrl("source.txt");
+  let targetFile = getTempFile(TEST_TARGET_FILE_NAME);
+  let { promise: filenamePromise, callback } = makeResolver(uri => {
+    uri.QueryInterface(Ci.nsIURL);
+    do_check_eq(uri.spec, url);
+    return Promise.resolve(targetFile.path);
+  });
+
+  let download = yield Downloads.createDownload({
+    source: url,
+    target: {
+      path: targetFile.parent.path,
+      filenameCallback: callback
+    }
+  });
+  yield download.start();
+  yield filenamePromise;
+  yield promiseVerifyContents(targetFile.path, TEST_DATA_SHORT);
+});
+
+/**
+ * Test using a filename callback on a redirected URL
+ */
+add_task(function* test_filenameCallbackRedirect() {
+  // filename callback only works with DownloadCore
+  if (gUseLegacySaver) {
+    return;
+  }
+
+  const path = "/redirect";
+  gHttpServer.registerPathHandler(path, (req, resp) => {
+    resp.setStatusLine(req.httpVersion, 301, "Moved Permanently");
+    resp.setHeader("Location", httpUrl("source.txt"), false);
+    return;
+  });
+  do_register_cleanup(() => { gHttpServer.registerPathHandler(path, null); });
+
+  let targetFile = getTempFile(TEST_TARGET_FILE_NAME);
+  let { promise: filenamePromise, callback } = makeResolver(uri => {
+    // make sure we get the redirected url
+    uri.QueryInterface(Ci.nsIURL);
+    do_check_eq(uri.spec, httpUrl("source.txt"));
+    return Promise.resolve(targetFile.path);
+  });
+
+  let download = yield Downloads.createDownload({
+    source: httpUrl("redirect"),
+    target: {
+      path: targetFile.parent.path,
+      filenameCallback: callback
+    }
+  });
+  yield download.start();
+  yield filenamePromise;
+  yield promiseVerifyContents(targetFile.path, TEST_DATA_SHORT);
+});
--- a/toolkit/components/jsdownloads/test/unit/test_DownloadList.js
+++ b/toolkit/components/jsdownloads/test/unit/test_DownloadList.js
@@ -105,16 +105,38 @@ add_task(function* test_add_getAll()
   do_check_eq(itemsTwo[0], downloadOne);
   do_check_eq(itemsTwo[1], downloadTwo);
 
   // The first snapshot should not have been modified.
   do_check_eq(itemsOne.length, 1);
 });
 
 /**
+ * Checks that adding a non-serializable Download throws
+ */
+add_task(function* test_add_nonSerializable() {
+  let list = yield promiseNewList();
+
+  let download = yield Downloads.createDownload({
+    source: httpUrl("source.txt"),
+    target: {
+      path: "/bleah",
+      filenameCallback: () => Promise.resolve("")
+    }
+  });
+
+  try {
+    yield list.add(download);
+    do_report_result(false, "DownloadList.add() should have failed on non-serializable download", Components.stack);
+  } catch (err) {
+    do_check_eq(err.message, "Cannot add download since its target is not yet serializable");
+  }
+});
+
+/**
  * Checks the method to remove items from the list.
  */
 add_task(function* test_remove()
 {
   let list = yield promiseNewList();
 
   yield list.add(yield promiseNewDownload());
   yield list.add(yield promiseNewDownload());