Bug 1280044 - handle subdirs in browser.downloads filenames, r=aswan
authorTomislav Jovanovic <tomica@gmail.com>
Fri, 09 Sep 2016 19:38:01 +0200
changeset 414191 c22083b560781e55de9e1e459d8f60a3f527a0ac
parent 414190 dc208a4e42581ec88517115471ca625412ed9fab
child 414192 5b40e209b9a751a03c3c9691893f5be954b0dc4e
push id29614
push userbmo:jaws@mozilla.com
push dateThu, 15 Sep 2016 21:02:56 +0000
reviewersaswan
bugs1280044
milestone51.0a1
Bug 1280044 - handle subdirs in browser.downloads filenames, r=aswan MozReview-Commit-ID: B4WoMbdxYjV
toolkit/components/extensions/ext-downloads.js
toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js
--- a/toolkit/components/extensions/ext-downloads.js
+++ b/toolkit/components/extensions/ext-downloads.js
@@ -18,16 +18,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://devtools/shared/event-emitter.js");
 
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 const {
   ignoreEvent,
   normalizeTime,
   runSafeSync,
   SingletonEventManager,
+  PlatformInfo,
 } = ExtensionUtils;
 
 const DOWNLOAD_ITEM_FIELDS = ["id", "url", "referrer", "filename", "incognito",
                               "danger", "mime", "startTime", "endTime",
                               "estimatedEndTime", "state",
                               "paused", "canResume", "error",
                               "bytesReceived", "totalBytes",
                               "fileSize", "exists",
@@ -386,22 +387,28 @@ function queryHelper(query) {
   });
 }
 
 extensions.registerSchemaAPI("downloads", "addon_parent", context => {
   let {extension} = context;
   return {
     downloads: {
       download(options) {
-        if (options.filename != null) {
-          if (options.filename.length == 0) {
+        let {filename} = options;
+        if (filename && PlatformInfo.os === "win") {
+          // cross platform javascript code uses "/"
+          filename = filename.replace(/\//g, "\\");
+        }
+
+        if (filename != null) {
+          if (filename.length == 0) {
             return Promise.reject({message: "filename must not be empty"});
           }
 
-          let path = OS.Path.split(options.filename);
+          let path = OS.Path.split(filename);
           if (path.absolute) {
             return Promise.reject({message: "filename must not be an absolute path"});
           }
 
           if (path.components.some(component => component == "..")) {
             return Promise.reject({message: "filename must not contain back-references (..)"});
           }
         }
@@ -411,33 +418,37 @@ extensions.registerSchemaAPI("downloads"
           return Promise.reject({message: "conflictAction prompt not yet implemented"});
         }
 
         function createTarget(downloadsDir) {
           // TODO
           // if (options.saveAs) { }
 
           let target;
-          if (options.filename) {
-            target = OS.Path.join(downloadsDir, options.filename);
+          if (filename) {
+            target = OS.Path.join(downloadsDir, filename);
           } else {
             let uri = NetUtil.newURI(options.url);
 
-            let filename;
+            let remote = "download";
             if (uri instanceof Ci.nsIURL) {
-              filename = uri.fileName;
+              remote = uri.fileName;
             }
-            target = OS.Path.join(downloadsDir, filename || "download");
+            target = OS.Path.join(downloadsDir, remote);
           }
 
-          // This has a race, something else could come along and create
-          // the file between this test and them time the download code
-          // creates the target file.  But we can't easily fix it without
-          // modifying DownloadCore so we live with it for now.
-          return OS.File.exists(target).then(exists => {
+          // Create any needed subdirectories if required by filename.
+          const dir = OS.Path.dirname(target);
+          return OS.File.makeDir(dir, {from: downloadsDir}).then(() => {
+            return OS.File.exists(target);
+          }).then(exists => {
+            // This has a race, something else could come along and create
+            // the file between this test and them time the download code
+            // creates the target file.  But we can't easily fix it without
+            // modifying DownloadCore so we live with it for now.
             if (exists) {
               switch (options.conflictAction) {
                 case "uniquify":
                 default:
                   target = DownloadPaths.createNiceUniqueFile(new FileUtils.File(target)).path;
                   break;
 
                 case "overwrite":
--- a/toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js
@@ -89,20 +89,20 @@ function waitForDownloads() {
 // Create a file in the downloads directory.
 function touch(filename) {
   let file = downloadDir.clone();
   file.append(filename);
   file.create(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
 }
 
 // Remove a file in the downloads directory.
-function remove(filename) {
+function remove(filename, recursive = false) {
   let file = downloadDir.clone();
   file.append(filename);
-  file.remove(false);
+  file.remove(recursive);
 }
 
 add_task(function* test_downloads() {
   setup();
 
   let extension = ExtensionTestUtils.loadExtension({
     background: `(${backgroundScript})()`,
     manifest: {
@@ -116,17 +116,18 @@ add_task(function* test_downloads() {
   }
 
   function testDownload(options, localFile, expectedSize, description) {
     return download(options).then(msg => {
       equal(msg.status, "success", `downloads.download() works with ${description}`);
       return waitForDownloads();
     }).then(() => {
       let localPath = downloadDir.clone();
-      localPath.append(localFile);
+      let parts = Array.isArray(localFile) ? localFile : [localFile];
+      parts.map(p => localPath.append(p));
       equal(localPath.fileSize, expectedSize, "Downloaded file has expected size");
       localPath.remove(false);
     });
   }
 
   yield extension.startup();
   yield extension.awaitMessage("ready");
   do_print("extension started");
@@ -135,16 +136,45 @@ add_task(function* test_downloads() {
   yield testDownload({url: FILE_URL}, FILE_NAME, FILE_LEN, "just source");
 
   // Call download() with a filename property.
   yield testDownload({
     url: FILE_URL,
     filename: "newpath.txt",
   }, "newpath.txt", FILE_LEN, "source and filename");
 
+  // Call download() with a filename with subdirs.
+  yield testDownload({
+    url: FILE_URL,
+    filename: "sub/dir/file",
+  }, ["sub", "dir", "file"], FILE_LEN, "source and filename with subdirs");
+
+  // Call download() with a filename with existing subdirs.
+  yield testDownload({
+    url: FILE_URL,
+    filename: "sub/dir/file2",
+  }, ["sub", "dir", "file2"], FILE_LEN, "source and filename with existing subdirs");
+
+  // Only run Windows path separator test on Windows.
+  if (WINDOWS) {
+    // Call download() with a filename with Windows path separator.
+    yield testDownload({
+      url: FILE_URL,
+      filename: "sub\\dir\\file3",
+    }, ["sub", "dir", "file3"], FILE_LEN, "filename with Windows path separator");
+  }
+  remove("sub", true);
+
+  // Call download(), filename with subdir, skipping parts.
+  yield testDownload({
+    url: FILE_URL,
+    filename: "skip//part",
+  }, ["skip", "part"], FILE_LEN, "source, filename, with subdir, skipping parts");
+  remove("skip", true);
+
   // Check conflictAction of "uniquify".
   touch(FILE_NAME);
   yield testDownload({
     url: FILE_URL,
     conflictAction: "uniquify",
   }, FILE_NAME_UNIQUE, FILE_LEN, "conflictAction=uniquify");
   // todo check that preexisting file was not modified?
   remove(FILE_NAME);