Bug 1280044 - handle subdirs in browser.downloads filenames, r=aswan
authorTomislav Jovanovic <tomica@gmail.com>
Fri, 09 Sep 2016 19:38:01 +0200
changeset 355409 c22083b560781e55de9e1e459d8f60a3f527a0ac
parent 355408 dc208a4e42581ec88517115471ca625412ed9fab
child 355410 5b40e209b9a751a03c3c9691893f5be954b0dc4e
push id6570
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:26:13 +0000
treeherdermozilla-beta@f455459b2ae5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaswan
bugs1280044
milestone51.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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);