218242 - nsIFilePicker.files returns returnCancel in mode modeOpenMultiple if result is > 4k

Status: RESOLVED DUPLICATE of bug 660833

(Toolkit :: UI Widgets, defect)

Description

[(not reading, please use [e-mail] instead)]

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 [e-mail] for the info.

Comment 1

[[e-mail]]

Copied the URL from bug 172001 comment 10.

Note that I would prefer G/SetWindowLong(... DWL_USER ...) to G/SetProp().

Comment 2

[Malcolm Rowe]

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

[Malcolm Rowe]

Attachment: Patch v1

Comment 4

[Malcolm Rowe]

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

[Malcolm Rowe]

Neil: Before sspitzer reviews this patch properly, could you take a look and
check there's nothing significantly wrong with it?

Comment 6

[[e-mail]]

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

[Malcolm Rowe]

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

[Malcolm Rowe]

Attachment: 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

[Malcolm Rowe]

Comment on attachment 131212 [details] [diff] [review]
Patrch v2

Neil, care to review this version?

Comment 10

[[e-mail]]

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

[Malcolm Rowe]

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

[[e-mail]]

> 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

[Malcolm Rowe]

>> 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

[Malcolm Rowe]

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

[[e-mail]]

Fixed on trunk by the patch in bug 234946, and probably wontfix for branch.

Comment 16

[Sebastian Tusk]

This bug is not fixed by bug 234946. It is still present in trunk.

Comment 17

[[e-mail]]

Agreed. Sorry about that.

Comment 18

[[e-mail]]

Really fixed this time!