Last Comment Bug 660833 - Forms with <input type='file' multiple> does not accept more than 253 files at once.
: Forms with <input type='file' multiple> does not accept more than 253 files a...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla7
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
: 218242 (view as bug list)
Depends on: 702743 668663 692469
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-31 07:07 PDT by christoph.iserlohn
Modified: 2011-11-15 17:16 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Dynamically allocate a buffer big enough to hold the size of multiple selected files. (6.34 KB, patch)
2011-06-03 18:48 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Dynamically allocate a buffer big enough to hold the size of multiple selected files. (5.36 KB, patch)
2011-06-03 20:25 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Dynamically allocates a buffer big enough to hold the size of multiple selected files. (5.64 KB, patch)
2011-06-04 06:15 PDT, Brian R. Bondy [:bbondy]
jmathies: review-
Details | Diff | Splinter Review
Dynamically allocate a buffer big enough to hold the size of multiple selected files. (5.85 KB, patch)
2011-06-22 20:32 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Fix as before but with simplified combo box length code and whitespace fixed (5.69 KB, patch)
2011-06-23 07:10 PDT, Brian R. Bondy [:bbondy]
jmathies: review+
jmathies: checkin+
Details | Diff | Splinter Review

Description christoph.iserlohn 2011-05-31 07:07:22 PDT
User-Agent:       Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/534.24 (KHTML, like Gecko) Chrome/11.0.696.71 Safari/534.24
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1 and Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110531 Firefox/7.0a1

If you have a file input and select more than 253 files in the file-chooser dialog the input form remains empty.

Reproducible: Always

Steps to Reproduce:
<!DOCTYPE html>
<html>
<head>
    <title></title>
</head>
<body>
<form action='#' method='post' enctype='multipart/form-data'>
  <input size='40' name='uploads[]' type='file' multiple>
</form>
</body>
</html>

Actual Results:  
The input form remains empty.

Expected Results:  
The input form contains the file-\path-names.

It works fine on Firefox 3.6.17 and Firefox 4.0.1 under Ubuntu 11.04 (64-Bit)
Comment 1 christoph.iserlohn 2011-05-31 08:33:26 PDT
Further testing revealed that 253 files are not a hard limit. It also depends on the filename-length. For the 253 files the length was 15 characters for each file.
Comment 2 Boris Zbarsky [:bz] (TPAC) 2011-05-31 12:52:46 PDT
http://msdn.microsoft.com/en-us/library/ms646839%28v=vs.85%29.aspx has this to say:

  If the OFN_ALLOWMULTISELECT flag is set and the user selects multiple files,
  the buffer contains the current directory followed by the file names of the
  selected files. 
  ...
  If the buffer is too small, the function returns FALSE and the
  CommDlgExtendedError function returns FNERR_BUFFERTOOSMALL. In this case, the
  first two bytes of the lpstrFile buffer contain the required size, in bytes or
  characters.

In other words, this API expects the caller to provide a fixed-size buffer, and then if the filenames the user selects don't all fit in that buffer it will just return an error.  As far as I can tell you can then resize your buffer and call the API again and the user has to select the files _again_.

We currently use 4096 for the fixed buffer size, which explains the observed behavior.  We don't do the "ask the user again" thing though; we probably should... or something.

In any case, this is not a DOM issue; this is an issue in the Windows nsIFilePicker implementation, and not a problem on other OSes as comment 0 points out.
Comment 3 Boris Zbarsky [:bz] (TPAC) 2011-05-31 13:02:03 PDT
http://support.microsoft.com/kb/131462 seems to be the way to make this work "sanely" on Windows.  This is probably the worst API I've dealt with recently.  :(
Comment 4 Jim Mathies [:jimm] 2011-05-31 14:29:43 PDT
(In reply to comment #3)
> http://support.microsoft.com/kb/131462 seems to be the way to make this work
> "sanely" on Windows.  This is probably the worst API I've dealt with
> recently.  :(

http://msdn.microsoft.com/en-us/library/bb776913%28v=vs.85%29.aspx

We should investigate using the newer selection dialogs. The dialog we currently use was superseded in Vista. I'd guess the new common item dialogs support all the same features. (I think there's a widget bug on this.)
Comment 5 Brian R. Bondy [:bbondy] 2011-05-31 18:21:44 PDT
I tried searching via Google, via bugzilla, and via scanning list of windows widget bugs,  but couldn't find the other bug Jim mentioned.  

Should I do both the fix and the new dialogs in 2 separate patches on this ticket? Or post a new ticket for the new UI and do the posted task in this ticket?
Comment 6 Boris Zbarsky [:bz] (TPAC) 2011-05-31 18:52:55 PDT
One bug per problem s probably better.
Comment 7 Brian R. Bondy [:bbondy] 2011-06-03 18:34:07 PDT
Regarding the new Common Item Dialogs in Vista and later I added a new ticket here:
https://bugzilla.mozilla.org/show_bug.cgi?id=661991

A patch is coming shortly for the problem reported in this ticket for the GetOpenFileNameW calls.
Comment 8 Brian R. Bondy [:bbondy] 2011-06-03 18:48:12 PDT
Created attachment 537286 [details] [diff] [review]
Dynamically allocate a buffer big enough to hold the size of multiple selected files.

Added a hook to the file picker dialog to resize the buffer needed as one selects more files than the current buffer contains.
Comment 9 Kai Liu 2011-06-03 19:39:54 PDT
>+static void SetFileDialogComboBoxMaxLen(HWND parentHWND, WPARAM len)
>+{
>+  const int CONTROL_ID_OF_DROPDOWN = 0x47C;
>+  HWND hLast = NULL;
>+  do {
>+    hLast = FindWindowEx(parentHWND, hLast, NULL, NULL );
>+    if(hLast && ::GetDlgCtrlID(hLast) == CONTROL_ID_OF_DROPDOWN) {
>+      SendMessage(hLast, CB_LIMITTEXT, len, 0);
>+      break;
>+    }
>+  } while(hLast != NULL);
>+}

Wouldn't SendDlgItemMessage be a better choice here?  Also, does the control ID vary between versions of Windows, or is it always 0x47C?
Comment 10 Brian R. Bondy [:bbondy] 2011-06-03 20:25:55 PDT
Created attachment 537297 [details] [diff] [review]
Dynamically allocate a buffer big enough to hold the size of multiple selected files.

This new patch which will do the same as the last patch, but also not assume a specific control ID.
Comment 11 Brian R. Bondy [:bbondy] 2011-06-03 20:27:17 PDT
I took out the dialog ID and just look for a ComboBox in general now so couldn't use SendDlgItemMessage, but thanks for the suggestion Kai.  Please feel free to review the new patch.
Comment 12 Brian R. Bondy [:bbondy] 2011-06-03 21:15:46 PDT
Just to clarify, I meant please feel free to give feedback on the patch :).
Comment 13 Brian R. Bondy [:bbondy] 2011-06-04 06:15:23 PDT
Created attachment 537343 [details] [diff] [review]
Dynamically allocates a buffer big enough to hold the size of multiple selected files.

Like the last patches, has a hook to resize the buffer needed as one selects more files than the current buffer contains. Also like the old patch sets the combo box max length appropriately.

It will now (as of this patch) also allocate FILE_BUFFER_SIZE more bytes than is needed so that if the user selects a file and holds down shift and down to select  additional items, we will not continuously reallocate
Comment 14 Kai Liu 2011-06-08 19:55:07 PDT
Comment on attachment 537343 [details] [diff] [review]
Dynamically allocates a buffer big enough to hold the size of multiple selected files.

>+static void SetFileDialogComboBoxMaxLen(HWND parentHWND, WPARAM len)

I just played around a little with the open file dialog, and it looks like the combo box text field is not tied in any way to the return buffer and is just cosmetic.  In other words, it's possible for the textbox's maximum length to be large enough to accommodate everything even if the return buffer is not.  And if the textbox is not large enough, GetOpenFileName will still succeed and we will still get the full file list despite the selection being too large for the box.

So increasing the maximum length of the combo textbox is optional and does not affect the core function.

As for whether we should increase the limit, in WinXP, the maximum length of the text box is quite large (quite possibly unbounded; in my test I exceeded 37K TCHARS--which is higher than the default limit of 30K TCHARS--and still did not hit the limit).  But as the length of text grew, there started to be noticeable lags in the updating of this textbox, which might be why the limit was cut down to less than 6K TCHARS in Win7.  When the user has that many files selected, I don't think that text box is going to be of much use to the user!  So given the uselessness of the textbox with that many files selected and given the lag associated with updating it, I think it's best that we don't tinker with that limit.

And if we do decide to increase the limit, why not just set it to 0 (make it effectively unbounded) instead of manually adjusting it upwards with each selection?

>+            UINT cbLength = 0; 

If you want to use Windows naming conventions ("Hungarian notation"), that should be cch, not cb (the sample in the KB article is wrong).  cch means "count of characters" and cb means "count of bytes".  Since we are dealing with strings, cch is the correct prefix (this is true even when dealing with 8-bit strings).

That having been said, much of Mozilla's Windows code uses Mozilla's own style conventions (i.e., no need to use "Hungarian" at all).

> It will now (as of this patch) also allocate FILE_BUFFER_SIZE more bytes
> than is needed so that if the user selects a file and holds down shift and
> down to select  additional items, we will not continuously reallocate

If, in one operation, I shift-select a bunch of files, does it send a notification for each item that was selected, or just a single notification for the whole shebang?  I think it's probably the latter (the former wouldn't make much sense), but if MSFT is crazy enough to use the former, then maybe a doubling scheme would be more appropriate.
Comment 15 Brian R. Bondy [:bbondy] 2011-06-10 14:02:52 PDT
Thanks for the comments Kai.

> the maximum length of the text box is quite large (quite possibly unbounded; in my test I exceeded 37K... 

For me on Win7 w/ FF4 there is a max cap at 260.  Note that when pasting text it bypasses the limit

> increasing the maximum length of the combo textbox is optional and does not affect the core function.

Agree that it is optional, as this bug description was only for allowing more selections, not being able to type them.  If the reviewer does not want this, I can take it out.

> If, in one operation, I shift-select a bunch of files, does it send a notification for each item... 

Yes it will send a new notification for each additional item that is selected.  This is why I decided to allocate more than is needed, otherwise we would have to reallocate on each selection change when holding shift + down arrow.

> then maybe a doubling scheme would be more appropriate.

I considered a doubling as well, but thought allocating a constant amount so that a reallocation is only needed (and not using too much memory) every let's say ~100 or so selection changes.
Again will do as reviewer requests.

> If you want to use Windows naming conventions ...
I'll remove this on my next update of the patch after the reviewer reviews.

Thanks again for your comments Kai, much appreciated.
Comment 16 Jim Mathies [:jimm] 2011-06-22 09:08:17 PDT
Comment on attachment 537343 [details] [diff] [review]
Dynamically allocates a buffer big enough to hold the size of multiple selected files.

Review of attachment 537343 [details] [diff] [review]:
-----------------------------------------------------------------

r-'ing for now so I can take a look at the updates. No big issues though. As far as the combo box is concerned, increasing that size as well seems fine.

::: widget/src/windows/nsFilePicker.cpp
@@ +116,5 @@
> +      SetFileDialogComboBoxMaxLen(GetParent(hwnd), FILE_BUFFER_SIZE);
> +      break;
> +    case WM_NOTIFY:
> +      {
> +        LPOFNOTIFY lpofn = (LPOFNOTIFYW) lParam;

LPOFNOTIFYW lpofn = ..

Projects use different default character widths so always be explicit in your use.

@@ +123,5 @@
> +        if (lpofn->hdr.code == CDN_SELCHANGE) {
> +            HWND parentHWND = GetParent(hwnd);
> +
> +            // Get the required size for the selected files buffer
> +            UINT cbLength = 0; 

nit - as Kai points out we don't use this notation. There's no harm in it and you'll find it around in the source, but generally try to use something readable like 'newBufLength'.

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

@@ +124,5 @@
> +            HWND parentHWND = GetParent(hwnd);
> +
> +            // Get the required size for the selected files buffer
> +            UINT cbLength = 0; 
> +            int bufferSize = CommDlg_OpenSave_GetSpec(parentHWND, NULL, 0);

Use the W variant here as well and with similar calls.

@@ +135,5 @@
> +            // multiple files, the buffer contains the current directory 
> +            // followed by the file names of the selected files. So
> +            // make room for the directory
> +            bufferSize = CommDlg_OpenSave_GetFolderPath(parentHWND, NULL, 0);
> +            if(bufferSize >= 0)

Either check the flag here (in the event someone makes use of FilePickerHook in some other case) or clear up the comment explaining FilePickerHook is specific to the OFN_ALLOWMULTISELECT case. (I'd go with the comment clean up.)
Comment 17 Brian R. Bondy [:bbondy] 2011-06-22 09:10:57 PDT
Thanks for the feedback Jim, I'll make these changes and submit a new patch.
Comment 18 Brian R. Bondy [:bbondy] 2011-06-22 20:32:21 PDT
Created attachment 541276 [details] [diff] [review]
Dynamically allocate a buffer big enough to hold the size of multiple selected files.

Updated patch with Jim's comments.  Thanks for the suggestions.
Comment 19 Kai Liu 2011-06-22 21:06:42 PDT
As I briefly mentioned in comment 14, sending CB_LIMITTEXT with a length of zero makes the input box effectively unbounded; set it once during the dialog init and you won't have to bother with it again.
Comment 20 Brian R. Bondy [:bbondy] 2011-06-23 04:14:37 PDT
I'll submit a new patch tonight Jim for re-review with the simplified  CB_LIMITTEXT code as Kai mentioned.
Comment 21 Jim Mathies [:jimm] 2011-06-23 05:32:35 PDT
(In reply to comment #20)
> I'll submit a new patch tonight Jim for re-review with the simplified 
> CB_LIMITTEXT code as Kai mentioned.

(some tabs snuck into that comment update as well.)
Comment 22 Brian R. Bondy [:bbondy] 2011-06-23 06:03:43 PDT
Noticed will fix that too.
Comment 23 Brian R. Bondy [:bbondy] 2011-06-23 07:10:50 PDT
Created attachment 541369 [details] [diff] [review]
Fix as before but with simplified combo box length code and whitespace fixed
Comment 24 Brian R. Bondy [:bbondy] 2011-06-23 07:17:19 PDT
Also forgot to mention that the old patch was setting the wrong combo box with class "ComboBox" instead of "ComboBoxEx32" which is the one we wanted to set.  So this new patch actually works for the max text length.
Comment 25 Jim Mathies [:jimm] 2011-06-23 07:35:08 PDT
Comment on attachment 541369 [details] [diff] [review]
Fix as before but with simplified combo box length code and whitespace fixed

Review of attachment 541369 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/windows/nsFilePicker.cpp
@@ +111,5 @@
> +        HWND comboBox = FindWindowEx(GetParent(hwnd), NULL, 
> +                                     L"ComboBoxEx32", NULL );
> +        if(comboBox)
> +          SendMessage(comboBox, CB_LIMITTEXT, 0, 0);
> +      }

According to msdn, the default limit 30K characters. Do we even need this?
Comment 26 Brian R. Bondy [:bbondy] 2011-06-23 07:37:12 PDT
That's what MSDN says, but for me on win7 x64 it is a 260 character limit, so I think we should keep it.
Comment 27 Jim Mathies [:jimm] 2011-06-29 13:18:52 PDT
Comment on attachment 541369 [details] [diff] [review]
Fix as before but with simplified combo box length code and whitespace fixed

http://hg.mozilla.org/integration/mozilla-inbound/rev/84533f5e86bf
Comment 28 Marco Bonardo [::mak] 2011-06-30 05:53:14 PDT
http://hg.mozilla.org/mozilla-central/rev/84533f5e86bf
Comment 29 neil@parkwaycc.co.uk 2011-06-30 15:59:29 PDT
*** Bug 218242 has been marked as a duplicate of this bug. ***
Comment 30 Vlad [QA] 2011-09-02 00:24:41 PDT
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 3
Comment 31 Brian R. Bondy [:bbondy] 2011-09-03 12:54:02 PDT
This fix for Vista and Windows 7 has to be undone.  It will be fixed once we do the new Common File Dialogs in Vista+ from Bug 661991.  The fix will remain in place for XP and below though.  See 684506 Comment 5 for details.

All work and other details being done in Bug 684506.

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