393342 - select "save to disk" for PDF, not downloaded or saved; NS_ERROR_FILE_UNRECOGNIZED_PATH

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Mike Shaver (:shaver emeritus)]

When I click to download a PDF file and pick "save to disk" (it's the default for me, FWIW), I don't get it downloaded.  I sometimes *do* get a pair of exceptions in the console, though, to read instead:

Error: [Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIDownloadManager.userDownloadsDirectory]"  nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)"  location: "JS frame :: file:///Applications/Minefield.app/Contents/MacOS/components/nsHelperAppDlg.js :: anonymous :: line 96"  data: no]
Source File: file:///Applications/Minefield.app/Contents/MacOS/components/nsHelperAppDlg.js
Line: 96

Error: uncaught exception: unknown (can't convert to string)

Started recently, possibly with today's nightly (2007082204; OS X on x86).  My download dir is "/Users/shaver/Downloads" in the prefs window, and for both browser.download.dir and browser.download.downloadDir the pref value is (ahem):

browser.download.downloadDir;AAAAAAFAAAIAAQhMNCBDYWNoZQAAAAAAAAAAAAAAAAAAAAAAAADBcRzJSCsAAAAH2LoJRG93bmxvYWRzAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAkRMcGpWicAAAAAAAAAAP////8AAAkgAAAAAAAAAAAAAAAAAAAABnNoYXZlcgAQAAgAAMFxjUkAAAARAAgAAMGpyqcAAAABAAgAB9i6AABvGQACAB9MNCBDYWNoZTpVc2VyczpzaGF2ZXI6RG93bmxvYWRzAAAOABQACQBEAG8AdwBuAGwAbwBhAGQAcwAPABIACABMADQAIABDAGEAYwBoAGUAEgAWVXNlcnMvc2hhdmVyL0Rvd25sb2FkcwATAAEvAAAVAAIADf//AAA=

HTH!

Comment 1

[Jim Mathies [:jimm]]

I'm not on OSX, so I can't test this. I'm wondering if this is a failure in the directory services code for osx for retreiving the user's download folder. It was not used much prior to bug 308073.

http://lxr.mozilla.org/mozilla/source/toolkit/components/downloads/src/nsDownloadManager.cpp#741

Can you post the result of the two new attributes in the download manager - userDownloadsDirectory and defaultDownloadsDirectory?

var dm = Components.classes["@mozilla.org/download-manager;1"]
         .getService(Components.interfaces.nsIDownloadManager);

dm.userDownloadsDirectory 
dm.defaultDownloadsDirectory
dm.userDownloadsDirectory.target 
dm.defaultDownloadsDirectory.target

(both should nsILocalFile)

One of these might result in the same error.

Note, browser.download.downloadDir is obsolete in Firefox, and is not used, so I'm not sure how it got set to that garbage. browser.download.dir is the pref we standardized on for custom download folder post 308073.

Comment 2

[Mike Shaver (:shaver emeritus)]

userDownloadsDirectory: NS_ERROR_FILE_UNRECOGNIZED_PATH (same error)
defaultDownloadsDirectory: [xpconnect wrapped (nsISupports, nsILocalFileMac, nsILocalFile, nsIFile, nsIHashable)]

user.target: NS_ERROR_FILE_UNRECOGNIZED_PATH (same error)
default.target: NS_ERROR_NOT_IMPLEMENTED from nsILocalFileMac.target

(That's not garbage, btw, it's how we encode Mac filehandles, because of pre-OS X legacy about names not being unique identifiers.  Welcome to the Mac!)

Comment 3

[Jim Mathies [:jimm]]

Hey Mike, what's your browser.download.folderList pref?

Comment 4

[Mike Shaver (:shaver emeritus)]

It's set to 2.

Comment 5

[Jim Mathies [:jimm]]

Ok, so you're saying the funny data in your browser.download.dir is correct on the Mac? I was looking at the patch, and afaict, the code that sets that didn't really change. However, in the new userDownloadsDirectory code I've made the assumption it's a path. I think that might be the problem, I should be getting the complex value as an nsilocalfile instead.

Comment 6

[Jim Mathies [:jimm]]

Yes, I think that's it. Mike, if I roll a patch, would you have the time to test it out to be sure it addresses the problem? I'm not on osx so i'm unable to test this.

Comment 7

[Jim Mathies [:jimm]]

Attachment: custom download folder patch v.1

Comment 8

[Jim Mathies [:jimm]]

Tested on windows, works great. It would be nice if we could find someone to try it out on osx as well.

Comment 9

[Mike Shaver (:shaver emeritus)]

Slightly different error now, after rebuilding with the patch, probably just line-number fuzz:

Error: [Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIDownloadManager.userDownloadsDirectory]"  nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)"  location: "JS frame :: file:///Users/shaver/src/trunk/obj-out/dist/Minefield.app/Contents/MacOS/components/nsHelperAppDlg.js :: anonymous :: line 107"  data: no]
Source File: file:///Users/shaver/src/trunk/obj-out/dist/Minefield.app/Contents/MacOS/components/nsHelperAppDlg.js
Line: 107

Comment 10

[Jim Mathies [:jimm]]

Mike, would you mind posting your browser.download.dir pref in here again? The first example you gave had "browser.download.downloadDir;" on the front of what appears to be valid base64 encoded data. Was that correct? 

Comment 11

[Mike Shaver (:shaver emeritus)]

No, that was an artifact of choosing "Copy" instead of "Copy Value" in the about:config window.  The pref value starts after the ;, and is

AAAAAAFAAAIAAQhMNCBDYWNoZQAAAAAAAAAAAAAAAAAAAAAAAADBcRzJSCsAAAAH2LoJRG93bmxvYWRzAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAkRMcGpWicAAAAAAAAAAP////8AAAkgAAAAAAAAAAAAAAAAAAAABnNoYXZlcgAQAAgAAMFxjUkAAAARAAgAAMGpyqcAAAABAAgAB9i6AABvGQACAB9MNCBDYWNoZTpVc2VyczpzaGF2ZXI6RG93bmxvYWRzAAAOABQACQBEAG8AdwBuAGwAbwBhAGQAcwAPABIACABMADQAIABDAGEAYwBoAGUAEgAWVXNlcnMvc2hhdmVyL0Rvd25sb2FkcwATAAEvAAAVAAIADf//AAA=

Comment 12

[Jim Mathies [:jimm]]

ahh, ok, just confirming - is Users/shaver/Downloads a valid directory on your system?

Comment 13

[Mike Shaver (:shaver emeritus)]

With a leading / it is, yes, and it happens to be my Downloads directory of choice. :)

("/Users/shaver/Downloads")

Comment 14

[Jim Mathies [:jimm]]

Decoding that base64 chunk shows 

"Users/shaver/Downloads"

within it, and I'm not sure if the starting '/' is implicit in these things. But if not, InitWithNativePath, which I believe is being called, has this check - 

1349   else if (filePath.IsEmpty() || filePath.First() != '/')
1350     return NS_ERROR_FILE_UNRECOGNIZED_PATH;

and it's one of the few places in OSX file code that returns NS_ERROR_FILE_UNRECOGNIZED_PATH. I'm wondering if there is a missing '/' in your custom dir pref. 

Comment 15

[Mike Shaver (:shaver emeritus)]

There's a leading slash in my pref as shown in the prefs window.

Where is InitWithNativePath being called?  Inside the complex-value deserialization?

Comment 16

[Jim Mathies [:jimm]]

Yes, and that error code really isn't returned very often, which is why I was looking at that. I hate to be a pain but do you have any time to place a break point in GetUserDwonloadsFolder and step through it to see where the failure is actually occuring? If not I'll try to find somebody on irc that can help us out. 

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 278426 [details] [diff] [review]
custom download folder patch v.1

> I'm wondering if there is a missing '/' in your custom dir pref.

Isn't the call to InitWithNativePath returning that error because you're passing it the base64 encoded string?

Anyhow, some review comments:

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp

>         if (customDirectory) {
>-          nsCOMPtr<nsILocalFile> aFile = 
>-            do_CreateInstance("@mozilla.org/file/local;1", &rv);

Glad you're removing this, using an "a" prefix for local variables is just confusing :)

>+          customDirectory->Exists(&bRes);
>           if (bRes) {
>-            NS_ADDREF(*aResult = aFile);
>+            NS_ADDREF(*aResult = customDirectory);
>             return NS_OK;
>           }

>-          rv = aFile->Create(nsIFile::DIRECTORY_TYPE, 755);
>+          rv = customDirectory->Create(nsIFile::DIRECTORY_TYPE, 755);
>           NS_ENSURE_SUCCESS(rv, rv);
>           if (bRes) {
>-            NS_ADDREF(*aResult = aFile);
>+            NS_ADDREF(*aResult = customDirectory);
>             return NS_OK;
>           }

This second block of code you're changing looks dead - bRes can't be true here, otherwise you would have returned in the previous block. It looks like perhaps you meant to include another Exists() call before checking it, but even then Exists() shouldn't return false after a succesful Create() call. Seems like you should just remove the second "if (bRes)" (or rewrite it so that the Create() call is in a (!bRes) block rather than returning early, for clarity).

This patch does fix the problem for me, though, so r+, but please do either attach a new patch here that includes the dead code removal, or file a bug to remove it separately.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

And it'd sure be nice to have some tests for for existent and non-existent "download dirs". The latter would have caught the dead code bug, I believe, by causing that method to return the default download dir when it creates the directory.

Comment 19

[Jim Mathies [:jimm]]

Attachment: custom download folder patch v.2

Comment 20

[Jim Mathies [:jimm]]

Attachment: custom download folder patch v.2

(removed an uneeded rv =)

Gavin, on the tests, I've not had the chance to write one of those. The purpose would be to check to make sure the directory returned was valid?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #20)
> Created an attachment (id=278606) [details]
> custom download folder patch v.2 

I don't think this is correct either - removing the rv checking for both calls makes the problem worse (one or both could fail while bRes remains true). I do think you want to go with the first patch, but just additionally remove the second "if (bRes)" check.

> Gavin, on the tests, I've not had the chance to write one of those. The
> purpose would be to check to make sure the directory returned was valid?

Right, just verifying that the nsIFile you get back from the call to GetUserDownloadsDirectory() is correct for various values of the pref. Perhaps Shawn can help with this, he probably knows the DM tests best.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 278606 [details] [diff] [review]
custom download folder patch v.2 

Beltzner doesn't do code reviews, and this doesn't need any additional (ui-)review once my comments are addressed.

Comment 23

[Jim Mathies [:jimm]]

Attachment: custom download folder patch v.3

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Landed on the trunk.

mozilla/browser/components/preferences/main.js 	1.11
mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp 	1.110

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

I can confirm that in the 2007-08-22-04 builds of Minefield on OS X 10.4, with a 'Users/stephend/Downloads/' directory, and the pref to prompt me where to save, I get the following exception thrown:

Error: [Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIDownloadManager.userDownloadsDirectory]"  nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)"  location: "JS frame :: file:///Applications/Minefield.app/Contents/MacOS/components/nsHelperAppDlg.js :: anonymous :: line 96"  data: no]
Source File: file:///Applications/Minefield.app/Contents/MacOS/components/nsHelperAppDlg.js
Line: 96

With the latest Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007090204 Minefield/3.0a8pre build, I can successfully save to 'Users/stephend/Downloads,' with no exceptions thrown.

shaver: thanks for logging this bug!

Verified FIXED