Last Comment Bug 218242 - nsIFilePicker.files returns returnCancel in mode modeOpenMultiple if result is > 4k
: nsIFilePicker.files returns returnCancel in mode modeOpenMultiple if result i...
Status: RESOLVED DUPLICATE of bug 660833
: helpwanted
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: Trunk
: x86 Windows 2000
-- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Neil Deakin
Mentors:
http://support.microsoft.com/?kbid=13...
Depends on: 172001
Blocks:
  Show dependency treegraph
 
Reported: 2003-09-03 17:32 PDT by (not reading, please use seth@sspitzer.org instead)
Modified: 2011-06-30 15:59 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.20 KB, patch)
2003-09-05 15:55 PDT, Malcolm Rowe
no flags Details | Diff | Splinter Review
Patrch v2 (23.57 KB, patch)
2003-09-10 23:49 PDT, Malcolm Rowe
neil: review-
Details | Diff | Splinter Review

Description User image (not reading, please use seth@sspitzer.org instead) 2003-09-03 17:32:18 PDT
nsIFilePicker.files returns returnCancel in mode modeOpenMultiple if result is > 4k

see http://bugzilla.mozilla.org/show_bug.cgi?id=172001#c10 for how to fix it.

thanks to malcolm-bmo@farside.org.uk for the info.
Comment 1 User image neil@parkwaycc.co.uk 2003-09-04 02:14:56 PDT
Copied the URL from bug 172001 comment 10.

Note that I would prefer G/SetWindowLong(... DWL_USER ...) to G/SetProp().
Comment 2 User image Malcolm Rowe 2003-09-04 06:10:04 PDT
Neil: yup, seconded.

Just a note: We'll want to pass in OFN_ENABLESIZING as well as OFN_ENABLEHOOK, 
since the latter disables the former by default.
Comment 3 User image Malcolm Rowe 2003-09-05 15:55:05 PDT
Created attachment 130982 [details] [diff] [review]
Patch v1
Comment 4 User image Malcolm Rowe 2003-09-05 16:06:14 PDT
Here's a first attempt at a patch. I've tested it locally (on W2K), and it works
fine, as far as I can tell.

Every time the selection changes in the filepicker, it recalculates what the
length of the buffer would be, and reallocates the caller's buffer if necessary.

I've tested the Unicode path, but I'm not able to test the ANSI path, since I
don't have W95/98/etc. Note that you *can't* test the ANSI path on a
Unicode-based system (WNT/2k/etc), as GetOpenFilenameA is implemented as an
ANSI->Unicode 'thunk' function, and won't reallocate the caller's buffer (like
what we now do in nsToolkit). The MSKB article alludes to this in its last
paragraphs, though it's far from clear.

This patch doesn't handle failure from nsMemory::Realloc, though I don't know
what the correct behaviour would be in that case.

Neil: I tried setting DWL_USER, but that caused a crash deep in Windows' DLLs,
so I guess the common dialog uses it. I plumped for GWL_USERDATA instead, which
should also be free for our use.
Comment 5 User image Malcolm Rowe 2003-09-05 16:15:53 PDT
Neil: Before sspitzer reviews this patch properly, could you take a look and
check there's nothing significantly wrong with it?
Comment 6 User image neil@parkwaycc.co.uk 2003-09-10 02:48:58 PDT
I think PR_Malloc and PR_FREEIF are more "null-friendly" functions to use.

Possibly at line 288 of the old nsToolkit.cpp the "Ansification" routine could
substitute the hook procedure.

Also, I think that at line 314 of the old nsToolkit.cpp you should always be
able to reallocate the unicode buffer (rather than anywhere else).

Check for consistent style e.g. space between if (

A -p on your diff would have been nice. Also, you mistyped ;; on one line.
Comment 7 User image Malcolm Rowe 2003-09-10 04:02:34 PDT
Comment on attachment 130982 [details] [diff] [review]
Patch v1

Cancelling review request. Neil, thanks for the review - I might get back to
you in more detail when I'm nearer a compiler.

I was originally trying to avoid increasing the coupling between nsFilePicker
and nsToolkit (hence the reason that nsFilePicker chooses the hook procedure),
though I later realised that nsToolkit's open/save dialog procedures aren't
called by anything other than nsFilePicker.

Perhaps I should just move the relevant routines out of nsToolkit and into
nsFilePicker - the GetOpenFileName and GetSaveFileName functions in nsToolkit
really have nothing to do with the rest of nsToolkit, which is all about
message pump management.

Otherwise, the Unicode hook will be in nsFilePicker and the ANSI hook will be
in nsToolkit, which seems crazy.

Updated patch to follow (with -p this time - I wasn't aware of that option).
Comment 8 User image Malcolm Rowe 2003-09-10 23:49:52 PDT
Created attachment 131212 [details] [diff] [review]
Patrch v2

Same as the last patch, with the filepicker routines from nsToolkit moved into
nsFilePicker, the nsToolkit ConvertAtoW / WtoA functions exposed as static
public members, and Neil's comments (comment 6) addressed.
Comment 9 User image Malcolm Rowe 2003-09-10 23:53:54 PDT
Comment on attachment 131212 [details] [diff] [review]
Patrch v2

Neil, care to review this version?
Comment 10 User image neil@parkwaycc.co.uk 2003-09-13 10:56:18 PDT
Comment on attachment 131212 [details] [diff] [review]
Patrch v2

OK, so I see that this version creates more issues than it resolves. Perhaps
you could put both hook procedures in nsToolkit?
Your moved reallocation of the unicode buffer uses the wrong size, the new
aFileNameW->nMaxFile should now be lenW+2, also I just noticed, shouldn't there
be more alloc failure checking?

[I don't like that while loop to calculate lenA but I can't improve it except
by replacing lenA+1 with ++lenA]
Comment 11 User image Malcolm Rowe 2003-09-13 14:18:43 PDT
Neil:

I could move the hook procedures to nsToolkit, but I thought that it was better
to move the filepicker routines to nsFilePicker, as (a) nsToolkit doesn't
otherwise have anything to do with the filepicker, and (b) nobody calls the
filepicker routines in nsToolkit apart from nsFilePicker. Placing the hook
procedures in nsToolkit would work, but would imply that it was possible for
other callers to use the filepicker routines, which isn't strictly true.

The reallocation of the Unicode buffer doesn't actually use the wrong size in
practice, but there's actually no reason for any of that length computation to
be there - I can just remove it, and reallocate the Unicode buffer to be the
same size as the resized ANSI buffer.

Regarding allocation checking: yes, I agree that there needs to be some, though
it's not entirely clear what the correct behaviour should be.

I assume that PR_Realloc won't destroy the source buffer if it can't reallocate
it? If that's true, then in the hook procedures, I could assign to a temporary
and only store the result back to the file buffer if the reallocation succeeded.
 If there's not enough space available in the buffer, the open/save dialog will
exit with an error (like it does now), but it won't cause a crash.

In the ANSI 'thunk' procedure (CallOpenSaveFileNameA), we could return a failure
if we couldn't reallocate the source Unicode buffer to fit (and again, we should
go through a temporary to ensure that a failure doesn't leak the original buffer).

Was there anything else that you specifically didn't like? I'm not sure what
alternative arrangement would make more sense, but suggestions are very welcome!
Comment 12 User image neil@parkwaycc.co.uk 2003-09-13 14:27:14 PDT
> I could move the hook procedures to nsToolkit
No, leave that bit how it was in your first patch, I've warmed to it :-)

> I can ... allocate the Unicode buffer to be the same size as the ANSI buffer.
Can you be sure of that working?
Comment 13 User image Malcolm Rowe 2003-09-17 08:58:26 PDT
>> I can ... allocate the Unicode buffer to be the same size as the ANSI buffer.
> Can you be sure of that working?

No.

I was assuming that all ANSI characters were in the BMP, and would therefore 
take only a single UTF-16 character. I'm not so sure that's true, so I'll just 
calculate the required size and resize the Unicode buffer according to that.
Comment 14 User image Malcolm Rowe 2003-10-22 02:56:23 PDT
pushing out. I'll try to get some time to clean up the first patch (attachment 
130982 [details] [diff] [review]) for 1.6b. This isn't that critical, anyway.
Comment 15 User image neil@parkwaycc.co.uk 2008-05-20 16:16:07 PDT
Fixed on trunk by the patch in bug 234946, and probably wontfix for branch.
Comment 16 User image Sebastian Tusk 2008-09-12 07:44:56 PDT
This bug is not fixed by bug 234946. It is still present in trunk.
Comment 17 User image neil@parkwaycc.co.uk 2008-09-12 12:56:52 PDT
Agreed. Sorry about that.
Comment 18 User image neil@parkwaycc.co.uk 2011-06-30 15:59:29 PDT
Really fixed this time!

*** This bug has been marked as a duplicate of bug 660833 ***

Note You need to log in before you can comment on or make changes to this bug.