Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED DUPLICATE of bug 660833

Status

()

Toolkit
XUL Widgets
RESOLVED DUPLICATE of bug 660833
14 years ago
6 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Unassigned)

Tracking

({helpwanted})

Trunk
x86
Windows 2000
helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

23.57 KB, patch
neil@parkwaycc.co.uk
: review-
Details | Diff | Splinter Review
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.
Depends on: 172001

Comment 1

14 years ago
Copied the URL from bug 172001 comment 10.

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

Comment 2

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

14 years ago
Created attachment 130982 [details] [diff] [review]
Patch v1

Comment 4

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

Updated

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

Comment 5

14 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

Updated

14 years ago
Status: NEW → ASSIGNED

Updated

14 years ago
Target Milestone: --- → mozilla1.6alpha

Comment 6

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

14 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

14 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

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

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

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

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

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

14 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
Status: ASSIGNED → NEW
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.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 16

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