268311 - Rewrite nsIFilePicker.displayDirectory handling and (windows) nsFilePicker::ShowW

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[timeless]

the current code is bad, i'll write a small testcase (which you'll probably have
to feed to jsconsole because someone decided that nsFilePicker requires a
parent, which you can't have in xpcshell).

in short displayDirectory holds onto someone else's file object, and gives it
out, this means that if you set the filepicker's path to object A, get the
filepicker's and store it in object B, and then change the path of object A, the
displaydirectory of the filepicker and the path of object B change. which is not
at all expected.

The code also manages to crash occasionally and doesn't create mDisplayDirectory
at a place where it could return an OOM.

Comment 1

[timeless]

Attachment: changes

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 165075 [details] [diff] [review]
changes

+	if (!mDisplayDirectory)
+		mDisplayDirectory =
do_CreateInstance("@mozilla.org/file/local;1");
+	if (mDisplayDirectory)
+	       
mDisplayDirectory->InitWithNativePath(nsDependentCString(dir_path.Path()));

this is asking to be replaced with NS_NewLocalFile

Comment 3

[[e-mail]]

Comment on attachment 165075 [details] [diff] [review]
changes

This is asking to be replaced with nsBaseFilePicker::[GS]etDisplayDirectory

Comment 4

[timeless]

Attachment: round 2

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 166717 [details] [diff] [review]
round 2

widget/src/beos/nsFilePicker.cpp
+	if (!mDisplayDirectory)
+		mDisplayDirectory =
do_CreateInstance("@mozilla.org/file/local;1");
+	if (mDisplayDirectory)
+	       
mDisplayDirectory->InitWithNativePath(nsDependentCString(dir_path.Path()));

use NS_NewNativeLocalFile

widget/src/xpwidgets/nsBaseFilePicker.cpp
+  nsresult rv = aDirectory->Clone(getter_AddRefs(directory));
+  mDisplayDirectory = do_QueryInterface(directory, &rv);
+  return rv;

you are ignoring the first rv here... please check it, or don't assign the
Clone rv into it. probably check it since that what GetDisplayDirectory does.

+  if (!mDisplayDirectory) {
+    *aDirectory = nsnull;
+    return NS_OK;
+  }

do callers deal with that?

Comment 6

[[e-mail]]

Comment on attachment 166717 [details] [diff] [review]
round 2

>+#ifdef BASEFILEPICKER_HAS_DISPLAYDIRECTORY
OK, I don't understand why the base filepicker is #ifdef'd

>+NS_IMETHODIMP nsBaseFilePicker::SetDisplayDirectory(nsILocalFile *aDirectory)
>+{
>+  if (mDisplayDirectory)
>+    return mDisplayDirectory->InitWithFile(aDirectory);
>+  nsCOMPtr<nsIFile> directory;
>+  nsresult rv = aDirectory->Clone(getter_AddRefs(directory));
>+  mDisplayDirectory = do_QueryInterface(directory, &rv);
>+  return rv;
I see no null test for aDirectory... the old filepicker code below accepted a
null directory which means reset to default (which is ~ on gtk1, but would be
last used on windows, say).

>-  set displayDirectory(a) { this.mDisplayDirectory = a; },
>-  get displayDirectory()  { return this.mDisplayDirectory; },
>+  set displayDirectory(a) { this.mDisplayDirectory = a.clone().QueryInterface(Components.interfaces.nsILocalFile); },
No null test again... add an a && as per the getter?

sr=me with the null setters fixed.

Comment 7

[timeless]

mozilla/widget/src/windows/nsFilePicker.h 	1.21
mozilla/widget/src/windows/nsFilePicker.cpp 	1.79
mozilla/widget/src/qt/nsFilePicker.h 	1.2
mozilla/widget/src/qt/nsFilePicker.cpp 	1.2 
mozilla/widget/src/xpwidgets/nsBaseFilePicker.h 	1.10
mozilla/widget/src/xpwidgets/nsBaseFilePicker.cpp 	1.26
mozilla/widget/src/photon/nsFilePicker.h 	1.7
mozilla/widget/src/photon/nsFilePicker.cpp 	1.9
mozilla/widget/src/os2/nsFilePicker.h 	1.15
mozilla/widget/src/os2/nsFilePicker.cpp 	1.47
mozilla/widget/src/mac/nsFilePicker.h 	1.18
mozilla/widget/src/mac/nsFilePicker.cpp 	1.63
mozilla/widget/src/gtk2/nsFilePicker.h 	1.4
mozilla/widget/src/gtk2/nsFilePicker.cpp 	1.9
mozilla/widget/src/cocoa/nsFilePicker.mm 	1.8
mozilla/widget/src/cocoa/nsFilePicker.h 	1.7
mozilla/widget/src/beos/nsFilePicker.h 	1.12
mozilla/widget/src/beos/nsFilePicker.cpp 	1.28
mozilla/xpfe/components/filepicker/src/nsFilePicker.js.in 	1.47
mozilla/widget/src/xpwidgets/nsBaseFilePicker.cpp 	1.27
mozilla/widget/src/gtk2/nsFilePicker.h 	1.5
mozilla/widget/src/qt/nsFilePicker.h 	1.3

Comment 8

[Joe Sabash [:JoeS1]]

Thunderbird trunk build version 0.6+ (20050114)fails to select image file for
Insert Image function. This worked in the 20050113 build. Any connection with
this checkin ?

Comment 9

[Frank Wein [:mcsmurf]]

(In reply to comment #8)
> Thunderbird trunk build version 0.6+ (20050114)fails to select image file for
> Insert Image function. This worked in the 20050113 build. Any connection with
> this checkin ?

Probably, this is covered by Bug 278422 (TB trunk uses the same code there).

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

I'm pretty sure this caused bug 279336.

Comment 11

[Kathleen :Brade]

Can someone back out this patch so we don't have the huge regressions in saving
for Composer and image insertion (which affects mail as well as composer)?

Reopening bug to keep it on the radar.  (My apologies if others prefer to track
this in the bugs that haven't yet seen much traction.)

Comment 12

[Joe Sabash [:JoeS1]]

(In reply to comment #11)
> Can someone back out this patch so we don't have the huge regressions in saving
> for Composer and image insertion (which affects mail as well as composer)?
> 
> Reopening bug to keep it on the radar.  (My apologies if others prefer to track
> this in the bugs that haven't yet seen much traction.)
I don't think any apologies are needed in this case. The results of this checkin
were known the day after they were checked in. To inconvenience the trunk
testers in such a blatent manner, seems to require an apology from the parties
that checked it in. The fallout from this checkin might seem to be insignificant
to some, but the users that test the trunk builds and use these functions daily,
probably would disagree.
Joe

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

With my editor module owner hat on, I am asking this patch to be backed out from
the tree. The side-effects of this checkin on the editor are too important with
unability to correctly save or select images on some platforms, in both Composer
and Messenger.
Since no fix for reported side-effects is in progress, since it's now almost
impossible to really test the editor in Composer or Messenger, I would like this
to be backed out as soon as possible. A fix with such side-effects should not
have been checked in. Thanks.

Comment 14

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

We have waited enough now. Asking.

Comment 15

[Brendan Eich [:brendan]]

timeless: back out, try again without the regressions.

/be

Comment 16

[timeless]

Attachment: round 3

Comment 17

[[e-mail]]

Comment on attachment 172337 [details] [diff] [review]
round 3

>+  if (!mDisplayDirectory)
>+    mDisplayDirectory = do_CreateInstance("@mozilla.org/file/local;1");
>+  if (mDisplayDirectory)
>+    mDisplayDirectory->GetNativePath(initialDir);
>   // If no display directory, re-use the last one.
>   if(initialDir.IsEmpty())
As per photon, there's no point creating the directory here because its path
will be empty.

>+  char *path = getenv( "HOME" );
>+  if( path ) {
>+    mDisplayDirectory->InitWithNativePath( nsDependentCString(path) );
>+  }
Nit: move the creation into the if block too.

>+  this.mDisplayDirectory = null;
>+  if (lastDirectory) {
>+    try {
>+      var dir = Components.classes[LOCAL_FILE_CONTRACTID].createInstance(nsILocalFile);
>+      dir.initWithPath(lastDirectory);
>+      this.mDisplayDirectory = dir;
>+    } catch (e) {}
>+  }
You might prefer to clone the directory as per the C++

r=me with the nits fixed.

Comment 18

[[e-mail]]

(In reply to comment #17)
>You might prefer to clone the directory as per the C++
I take that back, it's a string - but maybe it shouldn't be!

Comment 19

[timeless]

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

Comment 20

[timeless]

Comment on attachment 172337 [details] [diff] [review]
round 3

mozilla/widget/src/os2/nsFilePicker.cpp 	1.48
mozilla/widget/src/photon/nsFilePicker.cpp	1.10
mozilla/widget/src/windows/nsFilePicker.cpp	1.80
mozilla/widget/src/xpwidgets/nsBaseFilePicker.cpp	1.28
mozilla/xpfe/components/filepicker/src/nsFilePicker.js.in	1.48

Comment 21

[Joe Sabash [:JoeS1]]

Insert image via filepicker now OK with version 0.6+ (20050125)
Winxp pro Sp2

Comment 22

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Thanks.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

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