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

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.

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]

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.

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

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.

Steve, thanks for the patch and the testing! Can you a) post a combined patch b) request review on it, from sdwilsh@forerunnerdesigns.com? 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.

As requested, the combined patch.

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.

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.

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.

Sorry - Cr is shorthand for Components.results

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?

you probably changed it to windows line endings.

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

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! :)

requesting wanted-firefox3

> >+ 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?

Contains suggested changes...

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

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.

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 on attachment 305258 [details] [diff] [review] Updated patch a1.9=beltzner

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?

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

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

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?

(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.

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?

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 on attachment 309574 [details] [diff] [review] Update on comment #27 review clearing request - please address comment 34

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

Comment on attachment 305258 [details] [diff] [review] Updated patch Clearing approval since this missed landing for 1.9.