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




16 years ago
8 years ago


(Reporter: sspitzer, Unassigned)



Windows 2000

Firefox Tracking Flags

(Not tracked)




(1 attachment, 1 obsolete attachment)

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.
Copied the URL from bug 172001 comment 10.

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

Comment 2

16 years ago
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 4

16 years ago
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.


16 years ago
Attachment #130982 - Flags: review?(sspitzer)

Comment 5

16 years ago
Neil: Before sspitzer reviews this patch properly, could you take a look and
check there's nothing significantly wrong with it?
Assignee: sspitzer → malcolm-bmo


16 years ago


16 years ago
Target Milestone: --- → mozilla1.6alpha
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

16 years ago
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).
Attachment #130982 - Attachment is obsolete: true
Attachment #130982 - Flags: review?(sspitzer)

Comment 8

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

16 years ago
Comment on attachment 131212 [details] [diff] [review]
Patrch v2

Neil, care to review this version?
Attachment #131212 - Flags: review?(neil.parkwaycc.co.uk)
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]
Attachment #131212 - Flags: review?(neil.parkwaycc.co.uk) → review-

Comment 11

16 years ago

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

16 years ago
>> I can ... allocate the Unicode buffer to be the same size as the ANSI buffer.
> Can you be sure of that working?


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

16 years ago
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.
Target Milestone: mozilla1.6alpha → mozilla1.6beta
Product: Browser → Seamonkey
Assignee: malcolm-bmo → nobody
Component: General → XUL Widgets
Product: Mozilla Application Suite → Toolkit
QA Contact: general → xul.widgets
Target Milestone: mozilla1.6beta → ---
Fixed on trunk by the patch in bug 234946, and probably wontfix for branch.
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 16

11 years ago
This bug is not fixed by bug 234946. It is still present in trunk.
Agreed. Sorry about that.
Resolution: FIXED → ---
Really fixed this time!
Last Resolved: 11 years ago8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 660833
You need to log in before you can comment on or make changes to this bug.