44464 - [backend][FILE]support uploading multiple files

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Peter "jag" Annema]

Windows 95, build ID 2000070208
Linux, build ID 2000070220

I suspect this is XP.

Reproducable: always

Steps to take: try to select multiple files using the file select control

Current behaviour: one can only select one file per file select control

Expected behaviour: one can select multiple files per file select control

HTML4 spec, 17.2.1 Control types:
| ...
|   file select 
|     This control type allows the user to select files so that their contents
|     may be submitted with a form. The INPUT element is used to create a file
|     select control.
| ...

HTML4 spec, 17.13.2 Successful controls:
| ...
|   o The current value of a file select is a list of one or more file names.
|   Upon submission of the form, the contents of each file are submitted with
|   the rest of the form data. The file contents are packaged according to
|   the form's content type.
| ...

Comment 1

[Hixie (not reading bugmail)]

*** Bug 44468 has been marked as a duplicate of this bug. ***

Comment 2

[Blake Ross]

I think this might beter fit in xpapps -- can you elaborate more on what 
the 'file select control' is? Is that the file picker?

Comment 3

[Peter "jag" Annema]

The file select control is the form control which allows you to select files for 
uploading on submit.

<input type="file" ... >

I think this is form controls, unless the file picker itself has no support for 
selecting multiple files.

Comment 4

[rods (gone)]

This is partially a file picker issue and partically a form control issue. 

Are file files separated by commas?

This will probably end up as a future

Comment 5

[Eric Pollmann]

See related bug 44065

Comment 6

[Eric Pollmann]

Oh, yeah, that would be the dependency already marked, please ignore!  :)

Comment 7

[bsharma]

Updating QA contact.

Comment 8

[James Salsman]

On the basis of the greater utility, I am changing this 
from blocking bug 46136 to being dependent on it.  There 
was no dissent shown when I did that to duplicates of 
this bug.  Therefore, if there is any dissent here, please
state the reasons in the next day or so if possible.

The detailed rationalle is that if these two are done at 
the same time, all heck could possibly break loose, and 
46135 has many more votes.

Scott and Eric: I still can't figure out how the helper app and mime-times 
rdf database works.  I still haven't found the right place in the 
exthandler code that I promised to look for.  What I need to know is:

What part of the exthandler code reads and writes to and from those databases?

What parts of the other code interact with the mime-types and helper app 
databases?

I think this is a fine enhancement, and I will vote for it when I 
have the votes to spare.

IN THE MEAN TIME:  The owner or submitter should please mark this bug
with the 'correctness' keyword because it is indeed according to the spec.

Comment 9

[Jesse Ruderman]

I think bug 59216 has a patch for this..

Comment 10

[Adrian Havill]

Attachment: patch from bug 59216, with multipart/form-data patches for 58033, 58189, 59242 rolled in

Comment 11

[Adrian Havill]

*** Bug 59216 has been marked as a duplicate of this bug. ***

Comment 12

[James Salsman]

Well, so much for my pragmatic dependency.  This looks done.

Good job, Adrian!

This should probably be asigned to Eric [e-mail]
so that he can review and integrate the patches.

Comment 13

[Adrian Havill]

Note that for the patch, you may wish to remove the CRLF that makes the
Content-Disposition multi-line from lines 89 and 90... some server-side
file-upload POST parsers are simplistic and broken and may get confused by a
(legal according to W3C HTML 4) multi-line header, although this will make the
Content-Disposition line exceptionally long.

Comment 14

[rods (gone)]

I won't be able to take this patch as is (I am guessing), I have several changes 
already in my tree. I would like to get those checked in, then if you could 
re-merge and attach a new patch, that would be helpful thanks.

Comment 15

[Adrian Havill]

rods: using the build from last night which has your 2000-11-30 changes to
ns*FormControlFrame.*, nsFormControlHelper.*, the above patch to nsFormFrame.*
seems to be compatible with your changes and no re-merge/modification of the
patch seems necessary. Could you have a look and put the changes in the tree?
Thanks.

Comment 16

[Adrian Havill]

Attachment: patches XPFE filepicker to allow multiple files to be selected for file upload

Comment 17

[Peter "jag" Annema]

Why not hand back an array through |files| instead of using |getFileAt| and
|getTotalFiles|? That seems more natural to me.

Comment 18

[rods (gone)]

Adrian, please attach a "cvs diff -u" patch. the current patch I can't get to 
work.

Comment 19

[msw]

This patch applies fine when run when cwd = toplevel mozilla directory.  Or see
the patch -p (prune) option to prune off the number of directories you're down.

Comment 20

[Christopher Blizzard (:blizzard)]

That is a -u diff.

Comment 21

[rods (gone)]

I got it to work, but nsLocalFile.cpp didn't compile on windows. I needed to 
cast the call to nsMemory to (char*) here is the new diff:


Index: nsLocalFileWin.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v
retrieving revision 1.49
diff -u -r1.49 nsLocalFileWin.cpp
--- nsLocalFileWin.cpp  2000/11/28 23:26:02     1.49
+++ nsLocalFileWin.cpp  2000/12/05 19:50:31
@@ -20,6 +20,7 @@
  *
  * Contributor(s):
  *     Doug Turner <[e-mail]>
+ *     Adrian Havill <[e-mail]>
  */


@@ -1456,6 +1457,26 @@

     *aFileSize = mFileInfo64.size;
     return NS_OK;
+}
+
+NS_IMETHODIMP
+nsLocalFile::GetPathSeparator (char * *aPathSeparator)
+{
+   NS_ENSURE_ARG_POINTER(aPathSeparator);
+   *aPathSeparator = (char*)nsMemory::Alloc(2);
+   NS_ENSURE_TRUE(*aPathSeparator, NS_ERROR_OUT_OF_MEMORY);
+   PL_strcpy(*aPathSeparator, ";");
+   return NS_OK;
+}
+
+NS_IMETHODIMP
+nsLocalFile::GetDirSeparator (char * *aDirSeparator)
+{
+   NS_ENSURE_ARG_POINTER(aDirSeparator);
+   *aDirSeparator = (char*)nsMemory::Alloc(2);
+   NS_ENSURE_TRUE(*aDirSeparator, NS_ERROR_OUT_OF_MEMORY);
+   PL_strcpy(*aDirSeparator, "\\");
+   return NS_OK;
 }

 NS_IMETHODIMP

Comment 22

[rods (gone)]

This is one my list, I am in the middle of getting the patch to build.

Comment 23

[Matthew T (active 1999-2002)]

Since users are used to years and years of these controls only allowing upload 
of a single file, what does the UI for this look like?

Comment 24

[Stuart Parmenter]

the changes to nsIFilePicker are unacceptable.  Adding support for this is going 
to require changing nsIFilePicker methods from:

 93   readonly attribute nsILocalFile file;
100   readonly attribute nsIFileURL fileURL;

to:
  [array, size_is(nfiles)] nsILocalFile getFiles(out unsigned long nfiles)
  [array, size_is(nfiles)] nsIFileURL getFileURLs(out unsigned long nfiles)

(can you have array return types in js?... if not...:)

  void getFiles([array, size_is(nfiles)] out nsILocalFile files, out unsigned 
long nfiles)
  void getFileURLs([array, size_is(nfiles)] out nsIFileURL files, out unsigned 
long nfiles)

The way that the patch(es) suggest to do this is just gross.  Make these changes 
then post a new patch.

Stuart

Comment 25

[Matthew T (active 1999-2002)]

The UI shouldn't require any change to the filepicker. It should work just fine 
with a filepicker that only supports selection of a single file at once.

Comment 26

[Adrian Havill]

Mathew Thomas wrote:
> what does the UI for this look like?

The UI looks exactly the same... you have to know it's there to find it
(accidently hold the shift or ctl button while clicking on more than one file).
In fact, to hint that it's possible, I changed the text in the XPFE button from
"Browse..." (which is a UIish that's really OS specific) to "Pick Files..."
(plural File). Try out the XPFE patch to see what it looks like (unfortunately,
I don't have a Win32/Mac patch yet).

Who this will really mess up are server side scripts that blindly accept file
input with no error checking (many of them) to see if more than one file is
entered (when they can't support it). But any server side software that is
accepting file input/data from clients without checking data is going to get
into trouble even without this patch anyway.

Comment 27

[Adrian Havill]

Matthew Thomas wrote:
> The UI shouldn't require any change to the filepicker. It should work just
fine 
> with a filepicker that only supports selection of a single file at once.

It should work with a single-selection filepicker as well. If they have a single
select filepicker, they'll have to enter the file/pathnames in the input widget
by hand.

Relating to your previous comment about being used to the UI, I can't think of a
way how they would use a one-file-a-time filepicker more than once to enter
multiple files without changing the feel and operation of the UI. (IOW, use a
single-file filepicker without manually entering filenames in the text widget to
enter multiple names...) The file selections could be cummulative, but then how
would you delete or change previous entries? Users are used to when they hit the
"Browse..." button, it completely changes the entire contents of the text input
widget, so this would be a change from previous behavior.

Comment 28

[rods (gone)]

The current patch does not build on Windows.

Comment 29

[Matthew T (active 1999-2002)]

> In fact, to hint that it's possible, I changed the text in the XPFE button
> from "Browse..." (which is a UIish that's really OS specific) to "Pick
> Files..." (plural File).

I don't know how you find `Browse ...' to be OS-specific, but `Pick Files ...' 
is definitely unacceptable because it's plural-specific. If you change from 
`Browse ...', you will have millions of users wondering `where's 
the "Browse ..." button?' when Hotmail or Yahoo Mail tells them to click it to 
make an attachment.

> If they have a single select filepicker, they'll have to enter the
> file/pathnames in the input widget by hand.

That is similarly unacceptable. The filepicker should only allow multiple 
selections if the multiple items happen to be in the same directory, so we 
still need a nicer way of handling multiple files on the HTML side, so that 
people can use the filepicker multiple times to select the files one by one.

I suggest:
* When no files have been selected, the field looks like a text field followed
  by a `Browse ...' button, as usual.
* When one file has been specified, the field has `Browse ...', `Add ...', and
  `Remove' buttons.
* When more than one file has been specified, the field becomes a tree, with
  `Add ...' and `Remove' buttons.

Comment 30

[timeless]

Focus Change: This bug is now ONLY for backend support. All front end support 
for this bug and any implementation of it should be handled in bug 63687.

Comment 31

[rods (gone)]

Ok, I'll bite. What exactly is "backend" and what exactly do I need to fix?

Comment 32

[Adrian Havill]

"backend" refers to the first patch/attachment (the fileupload code that works
regardless of whether there's a cool UI front end filepicker to make selecting
the files easy). "frontend" is the filepicker.

I'll submit a new patch tonight with the void pointer type cast fix for VC++...
and there's one other thing I need to check because it looks like file upload
fails on Win32 even with or without this patch (need to double check that).

Santa gave me a VC compiler so I can check whether the patches compile on that
platform or not... either way, the void ptr cast that causes VC++ to bomb out is
in the XPFE filepicker code. The backend code in
<URL:http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19540> should work
on Linux/Win32/Mac, although I've only tested on Linux.

Can someone review/approve the _first_ patch (the second needs work and has
moved to bug 63687) to get it in the tree if they think it's ok?

Comment 33

[Adrian Havill]

I'm resubmitting the backend patch with it resynced to the latest nightely builds.

NS4_COMPATIBLE_MULTIPART_FORM has been defined to 1. When it is one, the client
request will be identical to Netscape 4 and won't use any HTML 4 spec enhancements.

I am testing the HTML 4 spec enhancehents with the common PHP and Perl
fileupload helper libraries. I need someone to test how the HTML 4 spec
enhancements work with the standard ISS ASP file upload libraries.

Comment 34

[Adrian Havill]

Attachment: resynced with nitelies and defaults via macro to Netscape-4ish uploads

Comment 35

[Eric Pollmann]

Will try for Moz 0.9

Comment 36

[Eric Pollmann]

Oops, sorry Rod, wrong bug!

Comment 37

[Vladimir Ermakov]

QA Contact Update

Comment 38

[Adrian Havill]

*** Bug 58033 has been marked as a duplicate of this bug. ***

Comment 39

[Adrian Havill]

*** Bug 59242 has been marked as a duplicate of this bug. ***

Comment 40

[Adrian Havill]

I've tested the patch with the most popular CGI processors. CGI.pm is ok. JSP
(JServ) is ok. PHP is ok within reason... older versions of PHP fail, but they
also fail on Netscape Navigator 4.x. Microsoft's ASP Web Posting Acceptor also
works. They work with all of the headers activated. Obviously, I can't test all
the hand-rolled file-upload libraries, but I've verified that it works with the
libraries above, which seem to be the most popular.

I've rearranged the order of some of the headers to predict what (broken)
parsers may rely on: made Content-Type always last, and Content-Disposition one
line, with the name parameter and the filename parameter before others, in that
order.

Comment 41

[Adrian Havill]

Attachment: updated patch with tree; made more friendly for broken upload parsers

Comment 42

[Adrian Havill]

This bug does not depend on bug 59220.

Comment 43

[Adrian Havill]

Furthermore, it doesn't block bug 44065 because it's front-end and it has
nothing to do with changing the contents of the multipart/form-data (front end
bug 63687, OTOH, does affect the contents of the form submission). This is back
end; the bugs can be fixed independently of each other.

Comment 44

[Adrian Havill]

*** Bug 59220 has been marked as a duplicate of this bug. ***

Comment 45

[Makoto Kato [:m_kato]]

Adrian, your code is a lettle incorrect for I18N implementation.  Because your 
proposal code does not use RFC2231.  If it uses parameter of Content-
Disposition, you should implement RFC2231.

Comment 46

[Adrian Havill]

Kato wrote:
>Adrian, your code is a lettle incorrect for I18N implementation.  Because your 
>proposal code does not use RFC2231.  If it uses parameter of Content-
>Disposition, you should implement RFC2231

*Nothing* I know of implements RFC2231, either generating or interpreting. If 
you implemented RFC2231, the generated data would confuse and fail on almost 
all file-upload form processing systems out there, as the data mangling it does 
to non-ASCII/long lines is extreme.

I submitted a small patch to the MIME decoder for mail (bug 58738) to recognize 
at least the language modifier part of encoded words of the RFC2231/2184, but 
it hasn't been approved yet.

Thus, I don't think generating RFC 2231 compliant messages it at this time is a 
good idea because it would break too many (almost all) server side form 
processors.

BTW, if you want a parser that can handle RFC 2231, check out:
<URL:http://people.redhat.com/havill/imime/>

Comment 47

[Brian King [:kinger]]

What is the current status on this one?

Comment 48

[Adrian Havill]

Brian King wrote:

> What is the current status on this one?

Pollmann checked in a older patch that adds the Transfer-Encoding header, so
this patch will have to be updated/resynced yet again. Will do.

Comment 49

[Adrian Havill]

Attachment: Updated patch with newest nightlies

Comment 50

[rods (gone)]

I really have no time to deal with this, and it should really be reassigninged 
to pollmann.

Comment 51

[rods (gone)]

reassinging

Comment 52

[Eric Pollmann]

This patch should fix bug 83065, moving to 0.9.2.

Comment 53

[Eric Pollmann]

*** Bug 84029 has been marked as a duplicate of this bug. ***

Comment 54

[Eric Pollmann]

*** Bug 83065 has been marked as a duplicate of this bug. ***

Comment 55

[Eric Pollmann]

So far, I have two main concerns with this patch:

1) We default to the new behaviour instead of Nav 4.x.  This is desirable from
the point of view of moving the web forward, but scary as the uploading multiple
files it is not in general (or even fringe) use.  How many current file upload
sites would be broken if a user tried to upload multiple files to them?  My
guess: almost all of them.

2) Using a specific character to separate the multiple filenames.  For example,
the character chosen for UNIX was : but : is a perfectly valid character in a
filename for UNIX.  ("echo 'test' > foo:bar" works fine) If we are to go this
route, we must escape the separator char.  Otherwise, it might be preferable to
pass a list of filename as an nsVoidArray.

My feeling is that we should make this a pref and have it default to off for the
time being.  Thoughts?

Comment 56

[Eric Pollmann]

:(  Well, the patch for the filepicker is out of date...  After spending a few
hours working on integrating it into my tree, I'm finding it slow going.  In the
interests of getting at least the regressions fixed, I'll come up with a reduced
patch that does not enable multiple file picking, but addresses the other
aspects of this bug (fixing header order, re-enabling getLeafName).  Adrian, do
you think that you'll have time to work on the other?  If not, it seems likely
that it will get pushed a milestone or so...

Comment 57

[timeless]

I'd suggest we use ' ' as the file seperator and escape ' ' from filenames as 
%20

Comment 58

[Eric Pollmann]

...and escape %20 from filenames as?  ;)  (I don't know about you, but I do
happen to have a fair number of files with %20 in the name...)  I do agree
through that the logic would be simplified by use of a single, platform neutral
character, since it will likely need to be escaped anyway.

Would it work to first replace all <escape character> with <escape
character><escape character>, then simply separate with <escape character>? 
(Where <escape character> could be any of *?&<>()'"`{}[],;%#@^<space><x01>...
just so long as it is not a common character in a file name on *any* platform.)

For a second there, I was going to suggest replacing all \ with \\ then <escape
character> with \<escape character> but realized that would cause a lot of extra
work for Win32...

Comment 59

[timeless]

why escape %20? are you telling me the intention of %20 isn't to represent a 
space?

yeah imo path seperators should be forbotten. that means :/\; this left ',' and 
' ' (along w/ a whole slew of others which just didn't seem like good ideas).   

MacOS's file path seperator is :
unix's is /, windows is \
windows uses : for drive seperation
ntfs uses it to indicate forks notepad "file:bar" on an ntfs partition is a 
nifty thing to do.
dos %PATH% seperator is ';'
unix's $PATH seperator is ':'

so between ',' and ' ' I picked ' ' because it's commonly used to deliminate 
parameters in both dos and unix (any spaces w/in a parameter are either escaped 
or the entire parameter is quoted).

I suppose that in addition to saying not to escape %20, I should suggest that 
we unescape and then escape (or just optimize this to mean basically what i 
said).

Comment 60

[Eric Pollmann]

The problem is not that %20 should not represent a space in a file name, it is
that if I try to upload two files named:

/home/pollmann/music/Britney%20Spears:%20Oops%20I%20Did%20It%20Again.mp3
/home/pollmann/music/Another worthless song.mp3

If we first unescape to:
/home/pollmann/music/Britney Spears: Oops I Did It Again.mp3
/home/pollmann/music/Another worthless song.mp3

Then escape to:
/home/pollmann/music/Britney%20Spears:%20Oops%20I%20Did%20It%20Again.mp3
/home/pollmann/music/Another%20worthless%20song.mp3

Then, we delimit these with a space, then hand them to
nsFormFrame::ProcessAsMultipart, the problem becomes: How do we find the files
to open them?  The original file name was lost for one of these files...

I'm probably missing something from what you said above, but the point is: we
need to escape the escaping character, one way or another.  No big deal though,
as I don't think that I'll be able to get this in for 0.9.2 anyway (other than
the regression fix patch I'll attach later)

Comment 61

[James Salsman]

> My feeling is that we should make this a pref and have it default to off
> for the time being.  Thoughts?

Yes, please, and please use %00 as a delimiter if possible, or %1e if NUL is 
not possible.

%1e is the official ASCII for RS -- Record Seperator -- and sometimes printed 
as "^^".  Will that break Unicode filenames?  I hope not.  If so, maybe there 
is a Unicode version of ASCII RS.  I don't know.  It seems like years since 
the last time I typed "man ascii" anyway.

Cheers,
James "waiting for Godot's input helper apps code stability" Salsman

Comment 62

[Eric Pollmann]

Attachment: Reduced patch - fix regressions only (no multiple file support)

Comment 63

[Eric Pollmann]

*** Bug 83442 has been marked as a duplicate of this bug. ***

Comment 64

[Eric Pollmann]

Moved the previous patch to bug 83065.  We need to get at least the regressions
fixed for this milestones.

Since the multiple file support portions of this patch are not working right
now, and there are a few issues to address in a very short time, pusing one
milestone.

Comment 65

[Christopher Hoess (gone)]

Reassigning this-there's a rotted patch here that might be resurrectable.

Comment 66

[James Salsman]

Untangling from bug 43105, which is a MailNews/Compositon "Attach File" bug,
not a file upload via HTTP bug.

Also, be careful.  As Eric pointed out somewhere, this might be a _de_jure_ 
standard, but as a _de_facto_ matter there are might not actually be any 
servers which correctly handle such a multiple-file multipart/file-data 
POSTs.  Is there a counter-example?

Comment 67

[rods (gone)]

moving to 0.9.6

Comment 68

[rods (gone)]

Moving to Future

Comment 69

[Boris Zbarsky [:bzbarsky]]

*** Bug 112596 has been marked as a duplicate of this bug. ***

Comment 70

[rods (gone)]

Alex, please take a look at this.

Comment 71

[Alexandru Savulov]

let's try an 1.0

setting milestone

Comment 72

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

i've looked through the rfc's concerning this. I see no real prolem with
implementing this once bug XXX is fixed. However we can't allow users to upload
multiple files in all <input type=file>s since that would very likely break many
sites (ie people select several files and the server can't handle that, thus
pissing off site admins).

So my suggestion is that we add a "multiple" attribute to <input> which would
allow the user to select several files. I know that this isn't very
"spec-frienly", however that wouldn't be the first time, nor would we be the
only one...

Comment 73

[m_mozilla]

I'm write CGI scripts and FWIW I cast my vote in the "make them re-write their
CGI scripts camp.

*if* a nonstandard attribute is to be considered, I suggest that the default
behavior (syntactically valid HTML, no nonstandard attribute) behave in the
standards compliant fashion and support mutiple-file upload.

correct behavior under valid HTML is defined. Behavior under invalid HTML
(adding that homemade attribute) is not defined, and so could be made to do
whatever folks find worth implementing. I suspect that unless marketroids
insist, most mozilla developers will be hesitant to code support for
not-already-in-wide-use nonstandard HTML.

...but you never know...

-matt

Comment 74

[Kevin McCluskey (gone)]

Moving to Moz1.1. Engineers are overloaded with higher priority bugs.

Comment 75

[John Keiser (jkeiser)]

The backend now supports it, as of bug 120682.