114399 - Suspend when open the directory,it is not allow read.

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Antonio.Xu]

The mozilla will suspend when open the directory,it is not allow read.
Download a file,and open a directory that not allow read, and save the file 
under this directory.mozilla will suspend.

Comment 1

[Antonio.Xu]

build NETSCAPE_6_2_1_RELEASE

Comment 2

[Antonio.Xu]

Attachment: Fix this bug,that can resolve the problem that is suspend the mozilla when open a disallow read directory.

This patch can fix this bug and bug #85309,

Comment 3

[Keyser Sose]

Adding keywords....This needs a  review etc...Antonio you might want to contact
the module owners for a review.

Comment 4

[Antonio.Xu]

Attachment: This patch can fix this bug for trunk.

Please review this patch and let me check in.

Comment 5

[Asa Dotzler [:asa]]

adding a couple of people that might be able to review this patch. 

Comment 6

[Boris Zbarsky [:bzbarsky]]

Will this throw up the permission alert from the filepicker _and_ close the 
filepicker?  Or will the user get the alert and then be able to select a 
different file?

Also,

> +      if (!parnt.isWritable() || (!file.isWritable())){

use "parentDir" or something, not "parnt".  The only time the dir needs to be 
writable is if the file does not exist.  So maybe something more like:

if ((file.exists() && !file.isWritable()) ||
    !parentDir.isWritable()) {

Comment 7

[Antonio.Xu]

Attachment: I have some change with my patch

if ((file.exists() && !file.isWritable()) ||
    !parentDir.isWritable()) {
I think "file.exists()" should not add,because this only allow replease a exist
file,not allow save file.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> because this only allow replease a exist file, not allow save file.

Um... no. If you look at my suggested "if" statement, it can be stated in words 
as "if we need to create a file and can't or if the file exists but can't be 
written then throw an error".  Your statement says "if we can't create a file or 
can't write the file then throw an error".  Your version will throw an error in 
the case when we have a non-writable directory but a writable file in the 
directory and try to overwrite the file.  There is no reason to throw an error 
in those conditions, since we have the permissions needed to save the data.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.... I'm on crack.  The whole thing is in a "if (isFile)" statement, so the 
file does exist.  And we need to make sure the parent is writable because the 
"save web page, complete" mode will need to create a directory, not just write 
our file.  The patch looks pretty good overall, with the following nits:

1)  Looks like it uses tabs; spacing is a little odd....
2) +errorOpenDirDoesntExistMessage=Permission denied! Could not open the 
directory %S.

I would do:
+errorDirNotReadableMessage=Directory %S is not readable or does not exist.
(note the different name for the string -- one can have an existing 
non-readable dir)

3) +saveWithoutPermissionMessage_file=Permission denied! Could not overwrite the 
file %S.Please use a different filename!

I would do:
+saveWithoutPermissionMessage_file=File %S is not writable.

4) +saveWithoutPermissionMessage_dir=Permission denied! Could not save the file 
%S under this directory.

I would do:
+saveWithoutPermissionMessage_dir=Cannot create file. Directory %S is not 
writable.

mpt?  Thoughts on phrasing?

Comment 10

[Bill Law]

I'm adding Brian Rynder to cc: list.  He is the person who should approve this 
patch since he has done much of the recent Unix file picker work.

Comment 11

[Antonio.Xu]

Attachment: This is final patch,please review my patch!

Thanks for bzbarsky's advice,i had changed my patch for this bug,the patch is
diff with trunc,i had test the patch on trunc,that is ok.So please review my
patch,and let me check in.

Comment 12

[Brian Ryner (not reading)]

Comment on attachment 63718 [details] [diff] [review]
This is final patch,please review my patch!

>@@ -243,9 +258,13 @@
>     break;
>   case nsIFilePicker.modeSave:
>     if (isFile) { // can only be true if file.exists()
>+      if (!file.isWritable()){
>+          throwFileSavingErrorDialog(file);
>+          ret = nsIFilePicker.returnCancel;
>+      }else{
>       // we need to pop up a dialog asking if you want to save
>-      var message = gFilePickerBundle.getFormattedString("confirmFileReplacing",
>-                                                         [file.unicodePath]);
>+            var message = gFilePickerBundle.getFormattedString("confirmFileReplacing",[file.unicodePath]);
>+
>       var rv = window.confirm(message);
>       if (rv) {
>         ret = nsIFilePicker.returnReplace;

Please try not to mess up the spacing. Other than that, looks fine.  r=bryner.

Comment 13

[jag (Peter Annema)]

Comment on attachment 63718 [details] [diff] [review]
This is final patch,please review my patch!

Patch looks good, just some nits:

> errorSavingFileTitle=Error saving %S
> saveParentIsFileMessage=%S is a file, can't save %S
> saveParentDoesntExistMessage=Path %S doesn't exist, can't save %S
>-
>+saveWithoutPermissionMessage_file=File %S is not writable.
>+saveWithoutPermissionMessage_dir=Cannot create file. Directory %S is not writable.

Could you insert a white line here? This'll keep the grouping intact.

> noPermissionTitle=Error opening directory
> noPermissionError=You do not have the permissions necessary to view this directory.
>Index: filepicker.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/filepicker/res/content/filepicker.js,v
>retrieving revision 1.62
>diff -u -w -r1.62 filepicker.js
>--- filepicker.js	2001/12/12 05:05:09	1.62
>+++ filepicker.js	2002/01/06 08:31:22
>@@ -44,7 +44,6 @@
> var okButton;
> 
> var gFilePickerBundle;
>-
> function filepickerLoad() {
>   gFilePickerBundle = document.getElementById("bundle_filepicker");
> 

Please keep the white line there, it makes the var easier to find.

>@@ -160,10 +159,26 @@
>   outlinerView.setFilter(filterTypes);
>   window.setCursor("auto");
> }

Please insert a white line here.

>+function throwFileSavingErrorDialog(file){
>+  var errorTitle, errorMessage, promptService;
>+  errorTitle = gFilePickerBundle.getFormattedString("errorSavingFileTitle", [file.unicodePath]);
>+  errorMessage = gFilePickerBundle.getFormattedString("saveWithoutPermissionMessage_file", [file.unicodePath]);
>+  promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
>+  promptService.alert(window, errorTitle, errorMessage)
>+}

Could you call this |showFileSavingErrorDialog|? Naming style in Mozilla
generally
uses |show| here, not |throw|.

> 
> function openOnOK()
> {
>   var dir = outlinerView.getSelectedFile();
>+  if (!dir.isReadable()){
>+        errorTitle = gFilePickerBundle.getFormattedString("errorOpenFileDoesntExistTitle",[dir.unicodePath]);
>+        errorMessage = gFilePickerBundle.getFormattedString("errorDirNotReadableMessage",[dir.unicodePath]);
>+        promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                                  .getService(Components.interfaces.nsIPromptService);
>+        promptService.alert(window, errorTitle, errorMessage);
>+        return false;
>+   }

The rest of this file uses 2 space indent, please use that style. Same for
space before '{'.

You haven't explicitely declared the vars errorTitle, errorMessage and
promptService
here, though I suspect those names are declared lower so you'll have to remove
the
declaration there.

>+
>   if (dir)
>     gotoDirectory(dir);
>   retvals.file = dir;
>@@ -243,9 +258,13 @@
>     break;
>   case nsIFilePicker.modeSave:
>     if (isFile) { // can only be true if file.exists()
>+      if (!file.isWritable()){
>+          throwFileSavingErrorDialog(file);
>+          ret = nsIFilePicker.returnCancel;
>+      }else{

Same here, space after '}' and before '{' (twice), and use 2 space indentation.

>       // we need to pop up a dialog asking if you want to save
>-      var message = gFilePickerBundle.getFormattedString("confirmFileReplacing",
>-                                                         [file.unicodePath]);
>+            var message = gFilePickerBundle.getFormattedString("confirmFileReplacing",[file.unicodePath]);
>+

Two nits here: space after comma and indentation.

However, I'd rather you undo this change, which seems purely a formatting
change. The old
formatting (distributing over two lines) keeps the line length within 80
characters. You
should (try to) apply this to the other lines you're adding (I forgot to
mention this
above).

>       var rv = window.confirm(message);
>       if (rv) {
>         ret = nsIFilePicker.returnReplace;
>@@ -253,6 +272,7 @@
>       } else {
>         ret = nsIFilePicker.returnCancel;
>       }
>+     }

This looks like it's only indented one, not two spaces.

Could you attach a version of this patch (after you've addressed the nits I've
picked)
without -w?

>     } else if (isDir) {
>       if (!sfile.equals(file)) {
>         gotoDirectory(file);
>@@ -262,7 +282,7 @@
>       ret = nsIFilePicker.returnCancel;
>     } else {
>       var parent = file.parent;
>-      if (parent.exists() && parent.isDirectory()) {
>+      if (parent.exists() && parent.isDirectory() && parent.isWritable()) {
>         ret = nsIFilePicker.returnOK;
>         retvals.directory = parent.unicodePath;
>       } else {
>@@ -280,6 +300,7 @@
>           errorMessage = gFilePickerBundle.getFormattedString("saveParentDoesntExistMessage",
>                                                               [oldParent.unicodePath, file.unicodePath]);
>         }
>+        if (!parent.isWritable()) errorMessage = gFilePickerBundle.getFormattedString("saveWithoutPermissionMessage_dir", [file.unicodePath]);

Could you spread this over two or three lines to make the code more readable
and keep
in style with the rest of the file?

>         promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>                                   .getService(Components.interfaces.nsIPromptService);
>         promptService.alert(window, errorTitle, errorMessage);

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: patch addressing jag's whitespace nits

This just updates attachment 63718 [details] [diff] [review] to jag's comments.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 64982 [details] [diff] [review]
patch addressing jag's whitespace nits

carrying over bryner's r=

Comment 16

[jag (Peter Annema)]

Comment on attachment 64982 [details] [diff] [review]
patch addressing jag's whitespace nits

sr=jag

Comment 17

[Antonio.Xu]

Attachment: I complete the new patch,please r my patch and sr my patch.

Thanks for Brian Ryner's advice,I change my patch according to Brian Ryner's
advice,so i attach a new attachment,please r and sr my patch

Comment 18

[Boris Zbarsky [:bzbarsky]]

Antonio, jag _has_ sr'ed your patch.  You seem to have completely ignored his
comments....  I can check in the patch that has r and sr sometime today if you'd
like, or we could go through the r/sr cycle on a new patch that you attach (not
using -w) that addresses jag's comments.  Please let me know.

Comment 19

[Bill Law]

Sorry for being late to the party, but Boris says in comment 9:

>And we need to make sure the parent is writable because the 
>"save web page, complete" mode will need to create a directory, not just write 
>our file.

We're not *always* saving as "web page, complete" so maybe this check is overly
restrictive.  It would kick in when you open the file picker from Composer, too,
correct?

Maybe we need to pass in another file picker flag or let the calling code deal
with the problem of creating the associated sub-directory downstream someplace
(we do get an I/O error at some point; we just don't pay attention to it; that
problem is being remedied).

Comment 20

[Antonio.Xu]

Hi Boris Zbarsky
Thanks for your help.Because i didn't pay attention to the comment,I created a 
new attachment,so this is my fault.Please check in the patch that has r and 
sr.I feel so happy,and thank you,thanks for your help.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Bill, sorry about my confusing comments... now that I have a tree and have
applied the patch, the code is something like:

if (isFile) {
 // do stuff
} else if (isDir) {
 // do more stuff
} else {
 // creating file
 // check in question happens
}

So we do need parent.isWritable().

Attachment 64982 [details] [diff] checked in, marking fixed.

Comment 22

[sairuh (rarely reading bugmail)]

rs vrfy --but will test more for bug 115197 when it's resolved.