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

VERIFIED FIXED

Status

--
major
VERIFIED FIXED
17 years ago
2 years ago

People

(Reporter: antonio.xu, Assigned: antonio.xu)

Tracking

Trunk
Sun
Solaris
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
build NETSCAPE_6_2_1_RELEASE
Summary: Suspend when open the directory,it is not allow read. → Suspend when open the directory,it is not allow read.
(Assignee)

Comment 2

17 years ago
Created attachment 61234 [details] [diff] [review]
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

17 years ago
Adding keywords....This needs a  review etc...Antonio you might want to contact
the module owners for a review.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: patch, review
(Assignee)

Comment 4

17 years ago
Created attachment 63039 [details] [diff] [review]
This patch can fix this bug for trunk.

Please review this patch and let me check in.

Comment 5

17 years ago
adding a couple of people that might be able to review this patch. 
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()) {

Blocks: 117228
(Assignee)

Comment 7

17 years ago
Created attachment 63454 [details] [diff] [review]
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.
> 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.
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

17 years ago
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.
(Assignee)

Comment 11

17 years ago
Created attachment 63718 [details] [diff] [review]
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.
Keywords: nsbeta1
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.
Attachment #63718 - Flags: review+

Comment 13

17 years ago
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);
Attachment #63718 - Flags: needs-work+
Created attachment 64982 [details] [diff] [review]
patch addressing jag's whitespace nits

This just updates attachment 63718 [details] [diff] [review] to jag's comments.
Attachment #61234 - Attachment is obsolete: true
Attachment #63039 - Attachment is obsolete: true
Attachment #63454 - Attachment is obsolete: true
Attachment #63718 - Attachment is obsolete: true
Comment on attachment 64982 [details] [diff] [review]
patch addressing jag's whitespace nits

carrying over bryner's r=
Attachment #64982 - Flags: review+

Comment 16

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

sr=jag
Attachment #64982 - Flags: superreview+
(Assignee)

Comment 17

17 years ago
Created attachment 64994 [details] [diff] [review]
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
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

17 years ago
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).
(Assignee)

Comment 20

17 years ago
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.
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.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
rs vrfy --but will test more for bug 115197 when it's resolved.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.