54789 - Crash saving all attachments - GetFolder mode handled incorrectly

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[Brian Ryner (not reading)]

Currently we don't handle the filepicker's GetFolder mode correctly.  In
particular:

1) we assume there is a filter list.  this is wrong, and causes a crash trying
to save all attachments in mail.

2) we don't even want to show the filter list in GetFolder mode

3) only folders should be shown in this mode.

I have a patch that addresses these problems.

Comment 1

[Brian Ryner (not reading)]

Nominating for RTM based on the fact that this causes a 100% reproduceable crash
on a fairly accessible (and useful!) operation.

Comment 2

[Brian Ryner (not reading)]

Attachment: patch

Comment 3

[Brian Ryner (not reading)]

Attachment: patch #2

Comment 4

[Brian Ryner (not reading)]

the patch i just attached is somewhat cleaner and also addresses the fact that
if no folder is selected, the current directory should be used.

Comment 5

[Peter "jag" Annema]

r=jag on the second patch, adding keywwords (patch, approval), marking critical.

Comment 6

[Peter Trudelle]

rtm+ need info, please remove 'need info' when super-reviewer adds a= to this
bug report.
Changing summary to reflect user problem rather than underlying cause.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

a=ben

Comment 8

[Peter Trudelle]

Removing 'need info', this should be ready to double-plus and land.

Comment 9

[Phil Peterson]

rtm++

Comment 10

[Brian Ryner (not reading)]

checked in on trunk, waiting for branch to open...

Comment 11

[Stuart Parmenter]

r=pavlov

Comment 12

[Brian Ryner (not reading)]

checked in on branch

Comment 13

[John Morrison]

verified on trunk 2000-10-05-21 M18

Comment 14

[Brian Ryner (not reading)]

*** Bug 55452 has been marked as a duplicate of this bug. ***

Comment 15

[jefft]

This still happens on today's build 2000101308.

Comment 16

[jefft]

Reopen it ...

Comment 17

[jefft]

*** Bug 55452 has been marked as a duplicate of this bug. ***

Comment 18

[jefft]

*** Bug 56490 has been marked as a duplicate of this bug. ***

Comment 19

[Brian Ryner (not reading)]

jeff-

Ok, apparently there IS a problem if you try to select a directory via changing
TO that directory and hitting OK.  Selecting the directory from the list (in the
parent directory) works fine for me.  Changing to the target directory and
hitting OK results in nothing happening.  Are you still seeing the crash, or
just this problem where the OK button doesn't work?

Comment 20

[Jim Roskind]

This bug has re-opened.  Clearly the patch that was approved did not heal the
problem.  Pushing the status back to rtm-need-info, and removing "fix in hand"
comment, and patch keyword.
Please update with status, and nominate if you have a safe reviewed fix.

Comment 21

[Peter Trudelle]

Best case resolution for this may be just simple bulletproofing that stops the
crash, if one still exists.  If the only problem is that the OK button is active
when there is no directory selected, then please minus and future this bug.

Comment 22

[Brian Ryner (not reading)]

I don't believe there is still a crash, at least I can't reproduce one.  I'm
waiting for jefft to tell me whether he is seeing a crash or not, and how to
reproduce it. Neither jag or I can make this crash in today's builds.

The problem I *am* seeing, is that if you try to select a folder for the saved
attachments by changing TO that directory (as opposed to clicking on the folder
from the level above and then clicking OK), the filepicker will not go away.
This all boils down to the fact that we weren't clearing the filename textfield
on the directory change, so the name of the folder that you changed into is
still there.  For example:

- Suppose I have a directory under my home directory for saved attachments,
/home/bryner/attachments.  When the filepicker comes up, it will start in
/home/bryner.
- If I select the attachments directory by clicking once on it in the file list
and clicking OK, everything is fine.
- But, if I double-click the attachments directory to change to that directory,
the textfield will still contain the text "attachments" (since clicking on a
file puts its name in the textfield), so clicking OK will try to save in
/home/bryner/attachments/attachments, which does not exist, and the filepicker
doesn't exit because you haven't selected something valid.  Can  be worked
around by manually clearing the textfield.  Note that this problem ONLY
manifests itself in the "Select Folder" mode of the filepicker, which is
currently only used, to my knowledge, in Save All Attachments.


There is an easy, 1-line fix for this to make sure we clear the textfield on
chdir.  If you think this fix is worth it, we can do a trial checkin on the
trunk (after proper reviews and testing), otherwise, ->Future (assuming there is
no crash scenario).

Comment 23

[Peter Trudelle]

No, it is too late for such minor improvements.  Typical case will be to select
directory while it is selected.  If that is the only problem, then this will
turn into rtm-/future.

Comment 24

[jefft]

The biggest problem I am having with my test is I cannot pick any directory at 
all. The drop down directory list is empty. Click on ".." button does nothing. 
Then click on the "Cancel" button it crashed out with the following error 
messages. Adding myself to the cc list. (I am testing with my daily branch daily 
build.)

can't get parent directory
nsWidget::~nsWidget() of toplevel: 32 widgets still exist.
nsWidget::~nsWidget() of toplevel: 29 widgets still exist.
WEBSHELL- = 7
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[JavaScript Warning: "reference to undefined property 
o.retvals.buttonStatus" {file: 
"/home/jefftsai/branch/ns/dist/bin/components/nsFilePicker.js" line: 171}]"  
nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: 
"JS frame :: /home/jefftsai/branch/ns/dist/bin/components/nsFilePicker.js :: 
anonymous :: line 171"  data: yes]
************************************************************
###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 
'mRawPtr != 0', file ../../dist/include/nsCOMPtr.h, line 649
###!!! Break: at file ../../dist/include/nsCOMPtr.h, line 649

Comment 25

[Brian Ryner (not reading)]

That sounds EXACTLY like what I fixed a couple of weeks ago.  Are you sure you
don't have other mods in your tree?  Can you try today's branch build from sweetlou?

Comment 26

[jefft]

I did a grep on my cvsco.log and cvsco-ns.log and I don't have any local 
modifications. I'll try the release build from sweetlou next.

Comment 27

[jefft]

The release build does fix the problem. I am puzzled now how come my branch 
build doesn't. marking the bug fixed.

Comment 28

[jefft]

And verified...