326228 - No error message when download manager tried to store file in folder with insufficent access rights

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect, P4)

Description

[Delete]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060126 SUSE/1.5.0.1-6 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060126 SUSE/1.5.0.1-6 Firefox/1.5.0.1

If the pointer in setting "Save all files to this folder:" shows on a folder with 755 by example the download manager doesn't show any error messages when starting download.

The Bug is reproducable. 


Reproducible: Always

Steps to Reproduce:
- Login a normal user
- klick on download file in web site
- change to radio button "sate to Disk"
- Click on OK button



Expected Results:  
A Error message with text: Insufficent Access Rights on Folder XYZ

Comment 1

[Philip Withnall (unavailable)]

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9a1) Gecko/20060212 Firefox/1.6a1
Confirmed, but a correct error message is shown if you try to save a page to a folder for which you do not have the correct permissions.

Comment 2

[Philip Withnall (unavailable)]

Looking at the command-line output when this happens, you get the following:

*** exception in validateLeafName: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsILocalFile.create]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///home/philip/mozilla/fx-opt-dynamic/dist/bin/components/nsHelperAppDlg.js :: anonymous :: line 249"  data: no]

Comment 5

[[:Aleksej]]

The bug is still there in Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b3pre) Gecko/2008011404 Minefield/3.0b3pre

No error dialog is shown when initiating a download by following a link (and choosing “Save File”), adding this message in the Error console:
“Error: uncaught exception: unknown (can't convert to string)”.

An error dialog is shown when when using the context menu, though.

Comment 8

[Steve]

Attachment: The nsUnknownContentTypeDialog that appears when downloading files

Comment 9

[Steve]

Attachment: The text changes for the insufficeint permissions.

The proposed text changes... It's at least a start.  Not sure if it's the right place...

Comment 10

[Steve]

From my findings, the bug only happens when you have the "Save files to" selection made in the Options->General->Download section.  The "Always ask me where to save files" selection defers to the OS, which *should* provide the correct insufficient rights message.

The problem (as referenced in the patch) begins in the makeFileUnique function.  The exception handler assumes there won't be an access deined problem and continues to try and create a unique filename.  The exception eventually bubbles out and you get something reported in the error console.  

Let me know if you need other bits of information.  I'm new to this process, so please bear with me.

Comment 11

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

Steve, thanks for the patch and the testing!

Can you

a) post a combined patch
b) request review on it, from [e-mail]?  To do so, click 'details' on the newly combined patch, and in the "Flags:  Review" select widget, choose ?, and paste the above email address into the resulting textfield.

Comment 13

[Steve]

Attachment: The combined nsHelperAppDlg.js.in and unknownContentType.properites diff

As requested, the combined patch.

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 304366 [details] [diff] [review]
The combined nsHelperAppDlg.js.in and unknownContentType.properites diff

tabs to spaces please
also, line wrapping at 80 characters please

>Index: toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
>     // Note - this function is called without a dialog, so it cannot access any part
>     // of the dialog XUL as other functions on this object do. 
>     promptForSaveToFile: function(aLauncher, aContext, aDefaultFile, aSuggestedFileExtension) {
>       var result = null;
>-      
>+            
don't do this change please

>+        try{
>+			result = this.validateLeafName(defaultFolder, aDefaultFile, aSuggestedFileExtension);
>+		}
>+		catch(e){
>+			if (e.name == "NS_ERROR_FILE_ACCESS_DENIED"){					
you should check if e.result == Cr.NS_ERROR_FILE_ACCESS_DENIED

>+				var pBundle = Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService);
>+				pBundle  = pBundle.createBundle("chrome://mozapps/locale/downloads/unknownContentType.properties");
a little bit further down in this function this bundle is already created, so just move those lines up so you can use it in here as well please.

>+				// Get prompt service.
>+				var prompter = Components.classes[ "@mozilla.org/embedcomp/prompt-service;1" ]
>+                               .getService( Components.interfaces.nsIPromptService );
nit: no spaces after/before () and [] please

>-        result = this.validateLeafName(newDir, result.leafName, null);
>-      }
>+        
>+    
>+			result = this.validateLeafName(newDir, result.leafName, null);
>+	}
this change seems unnecessary

>+		//check to see if the creation was even allowed
>+		if (e.name == "NS_ERROR_FILE_ACCESS_DENIED")
>+			throw e;
same comment as before

>           if (aLocalFile.exists())
>             aLocalFile.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0600);
>-        }
>+        }       
??
>-          catch(e) { }
>-
>+          catch(e){ } 
>+			
????

>Index: toolkit/locales/en-US/chrome/mozapps/downloads/unknownContentType.properties
>+badPermissions=The file could not be saved because you do not have the proper permissions.  Choose another save directory.
>+badPermissions.title=Invalid Save Permissions
> selectDownloadDir=Select Download Folder
> unknownAccept.label=Save File
> unknownCancel.label=Cancel
> fileType=%S file
> 
>+
no extraneous space please

If we want this for beta 4, it needs to get in tonight.  Flagging for ui-r on strings.

Comment 15

[Steve]

Ok, I'll work in the suggested changes later on tonight.  

Some of the ??? areas were a result of me messing around. The venkman plug-in wasn't available for the build I had, so I had to figure out where the exception was being called from. 

Comment 16

[Steve]

Any idea where Cr is defined?  What would I need to pull in? I'll look for myself, but if anyone knows right away it'll save me some time.

Comment 17

[Shawn Wilsher :sdwilsh]

Sorry - Cr is shorthand for Components.results

Comment 18

[Steve]

Attachment: Contains the suggestions made from review

Comment 19

[Steve]

Ok, not real sure why the diff is coming out with the whole file?  Sorry! Any suggestions?  My guess would be my editor.  Not sure how to fix?

Comment 20

[Shawn Wilsher :sdwilsh]

you probably changed it to windows line endings.

Comment 21

[Steve]

Attachment: Updated patch in the correct format

Ok, maybe I'll get it right this time... Sorry!

Comment 22

[Shawn Wilsher :sdwilsh]

Comment on attachment 304993 [details] [diff] [review]
Updated patch in the correct format

>+        } catch (ex) {
>+          // The containing window may have gone away.  Break reference
>+          // cycles and stop doing the download.
>+          const NS_BINDING_ABORTED = 0x804b0002;
>+          this.mLauncher.cancel(NS_BINDING_ABORTED);
Components.results.NS_BINDING_ABORTED please

>+        try{
nit: space after try, before brace

>+            result = this.validateLeafName(defaultFolder, aDefaultFile, aSuggestedFileExtension);
nit: indentation of two paces here please

>+        }
>+        catch(e){
nit: space after catch, before brace

>+            if (e.result == Components.results.NS_ERROR_FILE_ACCESS_DENIED){
ditto
>+                var prompter = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                .getService(Components.interfaces.nsIPromptService);
nit: dot should line up with the dot after Components

>+        
>+        if (e.result == Components.results.NS_ERROR_FILE_ACCESS_DENIED)
>+            throw e;
is this suppose to be a != check?

almost there! :)

Comment 23

[Shawn Wilsher :sdwilsh]

requesting wanted-firefox3

Comment 24

[Steve]

        
> >+        if (e.result == Components.results.NS_ERROR_FILE_ACCESS_DENIED)
> >+            throw e;
> is this suppose to be a != check?

No, I didn't want to change the behavior of the exception handler in any way except when the access denied result comes through.  I guess you could have a != and throw on the else, but what's the difference?

Comment 25

[Steve]

Attachment: Updated patch

Contains suggested changes...

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Please note that this can't land until after Beta 4 as we're string frozen.

Comment 27

[Shawn Wilsher :sdwilsh]

Comment on attachment 305258 [details] [diff] [review]
Updated patch

>--- toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in	8 Feb 2008 22:22:36 -0000	1.60
>+++ toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in	23 Feb 2008 22:44:08 -0000
>@@ -67,19 +67,19 @@ function nsUnknownContentTypeDialog() {
>     this.givenDefaultApp = false;
>     this.updateSelf = true;
>     this.mTitle    = "";
> }
> 
> nsUnknownContentTypeDialog.prototype = {
>     nsIMIMEInfo  : Components.interfaces.nsIMIMEInfo,
> 
>-    // This "class" supports nsIHelperAppLauncherDialog, and nsISupports.
>     QueryInterface: function (iid) {
>         if (!iid.equals(Components.interfaces.nsIHelperAppLauncherDialog) &&
>+            !iid.equals(Components.interfaces.nsITimerCallback) &&
>             !iid.equals(Components.interfaces.nsISupports)) {
>             throw Components.results.NS_ERROR_NO_INTERFACE;
>         }
>         return this;
>     },
> 
>     // ---------- nsIHelperAppLauncherDialog methods ----------
> 
>@@ -96,25 +96,32 @@ nsUnknownContentTypeDialog.prototype = {
>       this._timer.initWithCallback(this, 0, nsITimer.TYPE_ONE_SHOT);
>     },
> 
>     // When opening from new tab, if tab closes while dialog is opening,
>     // (which is a race condition on the XUL file being cached and the timer
>     // in nsExternalHelperAppService), the dialog gets a blur and doesn't
>     // activate the OK button.  So we wait a bit before doing opening it.
>     reallyShow: function() {
>-        var ir = this.mContext.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
>-        var dwi = ir.getInterface(Components.interfaces.nsIDOMWindowInternal);
>-        var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>-                           .getService(Components.interfaces.nsIWindowWatcher);
>-        this.mDialog = ww.openWindow(dwi,
>-                                     "chrome://mozapps/content/downloads/unknownContentType.xul",
>-                                     null,
>-                                     "chrome,centerscreen,titlebar,dialog=yes,dependent",
>-                                     null);
>+        try {
>+          var ir = this.mContext.QueryInterface(Components.interfaces.nsIInterfaceRequestor);
>+          var dwi = ir.getInterface(Components.interfaces.nsIDOMWindowInternal);
>+          var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>+                             .getService(Components.interfaces.nsIWindowWatcher);
>+          this.mDialog = ww.openWindow(dwi,
>+                                       "chrome://mozapps/content/downloads/unknownContentType.xul",
>+                                       null,
>+                                       "chrome,centerscreen,titlebar,dialog=yes,dependent",
>+                                       null);
>+        } catch (ex) {
>+          // The containing window may have gone away.  Break reference
>+          // cycles and stop doing the download.          
>+          this.mLauncher.cancel(Components.results.NS_BINDING_ABORTED);
>+          return;
>+        }
> 
>         // Hook this object to the dialog.
>         this.mDialog.dialog = this;
> 
>         // Hook up utility functions.
>         this.getSpecialFolderKey = this.mDialog.getSpecialFolderKey;
> 
>         // Watch for error notifications.
Changes from bug 417949 made it in here?


>+      var bundle = Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService);
>+      bundle = bundle.createBundle("chrome://mozapps/locale/downloads/unknownContentType.properties");
since we are here, we should probably fix the line wrapping too.

>+        try {
>+          result = this.validateLeafName(defaultFolder, aDefaultFile, aSuggestedFileExtension);
>+        }
>+        catch(e) {
nit: catch should be on the same line as the }

>+          if (e.result == Components.results.NS_ERROR_FILE_ACCESS_DENIED) {
>+            var prompter = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                                     .getService(Components.interfaces.nsIPromptService);
>+            // Display error alert (using text supplied by back-end).
>+            prompter.alert(this.dialog, bundle.GetStringFromName("badPermissions.title"), bundle.GetStringFromName("badPermissions"));
nit: line wrapping at 80 chars please

>+        if (e.result == Components.results.NS_ERROR_FILE_ACCESS_DENIED)
>+            throw e;
nit: indent of only two spaces please

r=sdwilsh with these fixed.  We still need ui-r.

Comment 28

[Shawn Wilsher :sdwilsh]

Moving this to P4, although I think we should still take this post beta because it is low risk and a big win for some of our linux users.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305258 [details] [diff] [review]
Updated patch

a1.9=beltzner

Comment 30

[Reed Loden [:reed]]

Ugh, I landed this without seeing comment #27. I backed out the nsHelperAppDlg.js.in change but left the unknownContentType.properties change in since we're string frozen now.

Steve, can you address comment #27 and attach a new patch?

Comment 31

[Reed Loden [:reed]]

Checking in toolkit/locales/en-US/chrome/mozapps/downloads/unknownContentType.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/unknownContentType.properties,v  <--  unknownContentType.properties
new revision: 1.10; previous revision: 1.9
done

Comment 32

[Shawn Wilsher :sdwilsh]

Steve - can we get an updated patch please to get checked in?

Comment 33

[Steve]

Attachment: Update on comment #27 review

Sorry, I saw this marked for check-in and haven't checked back since.  

As for the line wrappings, I'm not sure how you can avoid exceeding 80 chars here:

var prompter = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
                         .getService(Components.interfaces.nsIPromptService);

I'm unsure of the accepted style for this block?  Is there a link that I can be referred to for development guidelines?

Comment 34

[Shawn Wilsher :sdwilsh]

(In reply to comment #33)
> var prompter = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>                          .getService(Components.interfaces.nsIPromptService);
In cases like that, that's the best we can do, which is fine.

Comment 35

[Steve]

I'm seeing a lot of errors that spit the following in the error console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMLocation.href]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://venkman/content/venkman-handlers.js :: isWindowFiltered :: line 80"  data: no]

I'm seeing an Assert also pop-up more specifically referencing the .cpp.  I'm not running the debug build so I can't exactly remember the true assert messasge.  Something about a top-level window not being there.

At any rate, I don't think it has anything to do w/ this bug and I'm curious if anyone else is seeing this?  

Comment 36

[Steve]

Note, this is when I click a link to download something with a different content type.  

Here's a silly example:

http://www.ipecac.com/archives/previews/pink_anvil-desert.mp3

Sometimes it'll work as advertised and sometimes it'll error in way described in comment #35

Comment 37

[Shawn Wilsher :sdwilsh]

Comment on attachment 309574 [details] [diff] [review]
Update on comment #27 review

clearing request - please address comment 34

Comment 38

[Shawn Wilsher :sdwilsh]

Steve - status?  Drivers note - this bug already had it's strings land.

Comment 39

[Mike Schroepfer]

Comment on attachment 305258 [details] [diff] [review]
Updated patch

Clearing approval since this missed landing for 1.9.

Comment 40

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/0f78503f1f5b