Crash saving all attachments - GetFolder mode handled incorrectly

VERIFIED FIXED

Status

()

P2
critical
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: bryner, Assigned: bryner)

Tracking

({crash, platform-parity})

Trunk
x86
Linux
crash, platform-parity
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm+ need info])

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

18 years ago
Nominating for RTM based on the fact that this causes a 100% reproduceable crash
on a fairly accessible (and useful!) operation.
Status: NEW → ASSIGNED
Keywords: crash, pp, rtm
Whiteboard: FIX IN HAND
(Assignee)

Comment 3

18 years ago
Created attachment 15904 [details] [diff] [review]
patch #2
(Assignee)

Comment 4

18 years ago
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

18 years ago
r=jag on the second patch, adding keywwords (patch, approval), marking critical.
Severity: major → critical
Keywords: approval, patch

Comment 6

18 years ago
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.
Priority: P3 → P2
Summary: GetFolder mode handled incorrectly → Crash saving all attachments - GetFolder mode handled incorrectly
Whiteboard: FIX IN HAND → [rtm+ need info] FIX IN HAND
Target Milestone: --- → M19

Comment 8

18 years ago
Removing 'need info', this should be ready to double-plus and land.
Whiteboard: [rtm+ need info] FIX IN HAND → [rtm+] FIX IN HAND

Comment 9

18 years ago
rtm++
Whiteboard: [rtm+] FIX IN HAND → [rtm++] FIX IN HAND
(Assignee)

Comment 10

18 years ago
checked in on trunk, waiting for branch to open...

Comment 11

18 years ago
r=pavlov
(Assignee)

Comment 12

18 years ago
checked in on branch
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 13

18 years ago
verified on trunk 2000-10-05-21 M18
Keywords: vbranch
(Assignee)

Comment 14

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

Comment 15

18 years ago
This still happens on today's build 2000101308.

Comment 16

18 years ago
Reopen it ...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

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

Comment 18

18 years ago
*** Bug 56490 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 19

18 years ago
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?
Status: REOPENED → ASSIGNED

Comment 20

18 years ago
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.
Keywords: patch
Whiteboard: [rtm++] FIX IN HAND → [rtm need info]

Comment 21

18 years ago
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.
Whiteboard: [rtm need info] → [rtm+ need info]
(Assignee)

Comment 22

18 years ago
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

18 years ago
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

18 years ago
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

(Assignee)

Comment 25

18 years ago
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

18 years ago
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

18 years ago
The release build does fix the problem. I am puzzled now how come my branch 
build doesn't. marking the bug fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Comment 28

18 years ago
And verified...
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.