126829 - <input type=file> (file upload) doesn't close file

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Daniel Zirin]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.8+)
Gecko/20020218
BuildID:    2002021803

When submitting an attachment via bugzilla.mozilla.org/attachment.cgi which uses
HTTP PUT via the HTML tag <input type=file>, I noticed I couldn't delete the
file after completing the HTTP PUT successfully.  It appears the browser never
closes the file (until Mozilla is shutdown/restarted).

Reproducible: Always
Steps to Reproduce:
1.Attach a file to this bug (I've been attaching JPEGs)
2.After you attach the file, try to delete it from the PC


Actual Results:  The PC reports a dialog window when attempting to delete the
file saying "Cannot delete <file>: There has been a sharing violation.  The
source or destination file may be in use."

Expected Results:  Should be able to do whatever (delete, move, rename) with a
file that has been recently HTTP PUT to a server.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> It appears the browser never closes the filee

Yep.  We don't.

jkeiser, there's no way to close the file but keep a file stream around which
will reopen it on demand is there?

Comment 2

[John Keiser (jkeiser)]

Oooh.  This may be the reason we were seeing the file stay the same even when it
had changed in between the first time it submitted and when it reloaded.

I'll look into it, but my hunch is we need to create a special file stream that
closes the file when it's done with it, and reopens it on first read().  It's
also possible that the existing file stream can do this and we're doing it wrong.

darin, thoughts?

Comment 3

[Darin Fisher]

currently, session history holds onto an input stream for POST and PUT request
data.  in the old world, this was a reference to a file in a temp dir, so no one
probably noticed the problem.  now we have it referencing the real file to avoid
the temp file.

i think the best way to solve this would be to wrap the file stream in something
that will close the underlying file input stream when all the data has been read
and reopen it when rewound.

jkeiser: there isn't anything in necko to support this.  you're going to have to
roll your own, and perhaps make it generic so it can be added to necko ;-)

Comment 4

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

> something that will close the underlying file input stream when all the data
> has been read and reopen it when rewound.

Just a small nit: IMHO it should reopen on the first read after being rewound. 
(this is probably what you ment, but i just wanted to make sure)

Comment 5

[Darin Fisher]

sicking: sure.. that works for me :-)

Comment 6

[Daniel Zirin]

If it's a browser problem, could it be a Composer problem too?

Comment 7

[John Keiser (jkeiser)]

Probably not.  This is code specific to forms.

Comment 8

[Alfonso Martinez]

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

Comment 9

[John Keiser (jkeiser)]

This is important, and I know how to fix it, but I can't get to it right away.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Marc Boullet]

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

Comment 14

[Tuukka Tolvanen (sp3000)]

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

Comment 15

[Matthias Versen [:Matti]]

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

Comment 16

[Matthias Versen [:Matti]]

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

Comment 17

[Simon Fraser [no longer active]]

Just noticed this on Mac. Holding files open for long periods of time is a
dangerous thing; it's more likely to leave you open to disk corruption in the
event of a crash. I think this should be a mozilla1.0 bug.

Comment 18

[Sebastian Biallas]

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

Comment 19

[Jesse Ruderman]

I ran into this bug yesterday.  I was generating the html files for my site, and
didn't see the error messages generated by 'del' and 'move', so I was confused
for a few minutes about why index.html wasn't being regenerated correctly.  I
didn't realize the lock was created when I ran HTML validation until I saw this
bug on the mostfreq list.

Comment 20

[John Keiser (jkeiser)]

*sigh* yeah, nsbeta1 is probably a good idea.

Comment 21

[Matthias Versen [:Matti]]

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

Comment 22

[Bradley Baetz (:bbaetz)]

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

Comment 23

[Kevin McCluskey (gone)]

nsbeta1+

Comment 24

[Darin Fisher]

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

Comment 25

[John Keiser (jkeiser)]

Attachment: Patch

OK.  This fixes the problem by:
(a) giving FileInputStreams CLOSE_ON_EOF and OPEN_ON_READ functionality, which
together give us the behavior we want--it closes the file the instant Necko is
done with the file, and then reopens when Necko asks for the data again.
(b) giving MultiplexInputStream the ability to skip streams that are erroring
(this allows the user to delete the file and still have the submission work as
expected since it will skip the erroring file)
(c) fixes FileInputStream::ReadSegments to correctly handle the case where the
writer does not consume all data (I was using a raw FileInputStream during
testing).

Testing:
- I have verified on Linux that this gives expected behavior, both with printfs
(to verify that the file is closed and reopened)
- I have verifying that it fixes the Linux side effect: even if you change the
file in between the first submission and the second, the submission doesn't
change.
- I have tested a file URL
- I have observed the raw file post and reloaded a post submission with changes
in the file (or a deleted file).  The content-length changes just fine.  (Note
to self: we're going to have to build an auto-detect-content-type
MIMEInputStream if we ever detect content types via file contents)
- I am uploading this patch to Bugzilla using a patched build.

Comment 26

[Kathleen :Brade]

can you be sure to test open and saving files in both browser and composer?

Comment 27

[Darin Fisher]

Comment on attachment 78969 [details] [diff] [review]
Patch

>Index: netwerk/base/public/nsIFileStreams.idl

>+    /**
>+     * If this is set, the file will be deleted by the time the stream is
>+     * closed.  It may be removed before the stream is closed if it is possible
>+     * to delete it and still read from it).
>+     *
>+     * If OPEN_ON_READ is defined, and the file was recreated after the first
>+     * delete, the file will be deleted again when it is closed again.
>+     */

odd... i don't understand how you can delete a file and then recreate it for
reading
at this level.


>+    const long DELETE_ON_CLOSE = 0x1;

please use this notation for bit field values (1<<x), where x in {0,1,2,...}


>+    /**
>+     * If this is set, the file will close automatically when one reads past the
>+     * end.

nit: it's not possible to "read past the end" ... how about "when end of file
is
reached"


>+     * Additionally, the file will *not* opened *until* a file operation occurs.

s/opened/be opened/

>+     * that requires an open file.

what requires an open file?


> interface nsIFileOutputStream : nsIOutputStream

can you add a behaviorFlags parameter to this one as well?  even though there
are no flags currently, i can imagine that we might want some one day.	let's
not break binary compatibility to do so.


>+ * An output stream that stores up data to write out to another output stream
>+ * and does the entire write periodically, so that fewer writes to the
>+ * underlying output stream are necessary.

nit: "periodically" almost makes it sound like there is a timer running ;-)
perhaps,

 "An output stream that buffers data to write out to another output stream
  and does the entire write only when the buffer is full, so that fewer writes
  to the underlying output stream are necessary."


>Index: netwerk/base/src/nsFileStreams.cpp

>+// XXX Would be nice if there were general functions that did this
>+#define GET_BOOLBIT(bitfield, mask) (((bitfield) & mask) ? PR_TRUE : PR_FALSE)
>+#define SET_BOOLBIT(bitfield, mask, b) ((b) ? ((bitfield) |=  mask) : \
>+                                              ((bitfield) &= ~mask))

see prbit.h, which defines PR_TEST_BIT, PR_SET_BIT, and PR_CLEAR_BIT.


>@@ -552,15 +563,43 @@

>+        if (NS_FAILED(rv) && !GET_BOOLBIT(mBehaviorFlags, OPEN_ON_READ)) {

how about this instead:
	  if (NS_FAILED(rv) || !mFile) {


>+nsFileInputStream::Reopen()

seems like this function should be implemented inline in the header file.


>+NS_IMETHODIMP
>+nsFileInputStream::Init(nsIFile* file, PRInt32 ioFlags, PRInt32 perm,
>+                        PRInt32 behaviorFlags)
>+{
>+    NS_ENSURE_TRUE(!mFD, NS_ERROR_ALREADY_INITIALIZED);

please also add another check for mParent.  if mParent is TRUE, then we
also cannot be initializing this stream, since the stream is really tied
to a nsFileIO.

>@@ -572,18 +611,32 @@

>+    if (GET_BOOLBIT(mBehaviorFlags, DELETE_ON_CLOSE) && mFile) {

check mFile first:
      if (mFile && (mBehaviorFlags & DELETE_ON_CLOSE)) {

>+        if (!GET_BOOLBIT(mBehaviorFlags, OPEN_ON_READ)) {
>+          mFile = nsnull;
>+        }

else what file is there to open?  you just deleted it!


>+#define CHECK_REOPEN \
>+    if (!mFD) { \
>+      if (GET_BOOLBIT(mBehaviorFlags, OPEN_ON_READ)) { \
>+        if (NS_FAILED(Reopen())) { \
>+            return NS_BASE_STREAM_CLOSED; \
>+        } \
>+      } else { \
>+        return NS_BASE_STREAM_CLOSED; \
>+      } \
>+    }

how about making this an inline function in the header file?


>+    // Check if we're at the end of file and need to close
>+    if (GET_BOOLBIT(mBehaviorFlags, CLOSE_ON_EOF)) {
>+        if (PR_Available(mFD) == 0) {
>+            Close();

this works, but maybe you should check the return value of PR_Read
first.	if it returns 0, and the amount requested is non-zero, then
you know that we have reached end of file.  btw: that's the only
way the caller is going to know that it reached EOF, so if you
close the file to soon, you're going to get another call to ::Read
and that means that you're probably going to end up reopening the
file just to let the caller read EOF.


> nsFileInputStream::ReadSegments(nsWriteSegmentFun writer, void * closure, PRUint32 count, PRUint32 *_retval)

sorry, you don't need to implement ReadSegments.  there is no way to
properly implement it for a file stream.  no buffer.. no ReadSegments.
you should stub this function out with a NS_NOTREACHED.

Comment 28

[Darin Fisher]

> how about this instead:
>
  if (NS_FAILED(rv) || !mFile) {

whoops!!  i meant, if (NS_FAILED(rv) && !mFile) {


also, i'm a little worried about the concept of silently ignoring a substream of
nsMultiplexInputStream... seems like this might not work very nicely with
session history if the POST results have been evicted from the cache.  we will
act like we can rePOST when really we cannot... because the POST data no longer
exists :(  there needs to be some sort of user notification in this case.

Comment 29

[John Keiser (jkeiser)]

All changes made, except:

1. re: deleting a file and recreating.  You don't recreate files in the file
input stream but with CLOSE_ON_EOF and OPEN_ON_READ the user can do whatever
they want with the file in between the first read and the second read.

2.
> >+    const long DELETE_ON_CLOSE = 0x1;
> please use this notation for bit field values (1<<x), where x in {0,1,2,...}

I made this change now because y
> >+        if (NS_FAILED(rv) && !GET_BOOLBIT(mBehaviorFlags, OPEN_ON_READ)) {
> how about this instead:
>
  if (NS_FAILED(rv) || !mFile) {

That would fail when DELETE_ON_CLOSE was set but the file had to be deleted
after it was read.
6.
> please also add another check for mParent.  if mParent is TRUE, then we
> also cannot be initializing this stream, since the stream is really tied
> to a nsFileIO.

I take it mFD and mParent are mutually exclusive?ou are the module ownplease
also add another check for mParent.  if mParent is TRUE, then we
also cannot be initializing this stream, since the stream is really tied
to a nsFileIO.

er (I think), but I would like to note that everywhere else I have seen bitmasks
(in and out of Mozilla), they were done with 0xn.  It's really a standard format
for bitmasks.

3.
> >+     * that requires an open file.
> what requires an open file?

It was redundant (meant to be part of the previous sentence), I just removed it.

4.
> see prbit.h, which defines PR_TEST_BIT, PR_SET_BIT, and PR_CLEAR_BIT.


prbit.h is doing some really weird stuff which I am not sure is appropriate for
the purposes.  What do logarithms have to do with bitmasks?

5.
> >+        if (NS_FAILED(rv) && !GET_BOOLBIT(mBehaviorFlags, OPEN_ON_READ)) {
> how about this instead:
>
  if (NS_FAILED(rv) || !mFile) {

That would fail when DELETE_ON_CLOSE was set but the file had to be deleted
after it was read.

6.
> >+        if (!GET_BOOLBIT(mBehaviorFlags, OPEN_ON_READ)) {
> >+          mFile = nsnull;
> >+        }

> else what file is there to open?  you just deleted it!

There isn't now, but it could be created by the user.  The behavior of
OPEN_ON_READ is such that if the *user* deletes the file and then recreates is
later it will be reopened, so we should be consistent when DELETE_ON_CLOSE
occurs and OPEN_ON_READ is defined.

7. (re: CHECK_REOPEN) how about making this an inline function in the header file?

It returns values.  If I made it a function I'd have to do nsresult rv =
CheckReopen(); if (NS_FAILED(rv)) { return rv; } *everywhere*.  I like this
solution better personally.  It's kind of like NS_ENSURE_SUCCESS and other
similar macros.

8.
> >+    // Check if we're at the end of file and need to close
> >+    if (GET_BOOLBIT(mBehaviorFlags, CLOSE_ON_EOF)) {
> >+        if (PR_Available(mFD) == 0) {
> >+            Close();
>
> this works, but maybe you should check the return value of PR_Read
> first.  if it returns 0, and the amount requested is non-zero, then
> you know that we have reached end of file.  btw: that's the only
> way the caller is going to know that it reached EOF, so if you
> close the file to soon, you're going to get another call to ::Read
> and that means that you're probably going to end up reopening the
> file just to let the caller read EOF.

Read() == 0 was my first attempt :)

But remember that itty bitty change to MultiplexInputStream::ReadSegments to
deal with large form submits?  It checks Available() before doing a Read() and
the net effect is it only reads as much as the rest of the file.  It never does
a Read/ReadSegments again after that, at least from the printf()'s.  Now that I
think about it though, the caller's behavior is wrong.  You can't ever assume
that Available() actually represents the number of remaining bytes.  I must
think about how to deal with this.  Once I figure out how to fix the caller I
will make Read() only check for 0 from PR_Read().

9. sorry, you don't need to implement ReadSegments.  there is no way to
properly implement it for a file stream.  no buffer.. no ReadSegments.
you should stub this function out with a NS_NOTREACHED.

Please see the changes I made to the function that make it work.  The file is a
natural buffer.

10.
> also, i'm a little worried about the concept of silently ignoring a substream
> of nsMultiplexInputStream... seems like this might not work very nicely with
> session history if the POST results have been evicted from the cache.  we will 
> act like we can rePOST when really we cannot... because the POST data no
> longer exists :(  there needs to be some sort of user notification in this
> case.

That makes no sense.  Session history just knows about a POST InputStream.  It
doesn't matter what type it is.  If the POST request has been evicted from the
cache, so has the InputStream.  Further, Session History *cannot* rely on error
behavior from the InputStream because it would not work *now*.  The InputStreams
we use in most cases are strings, and those will never error out when you Read()
and Seek(0) them.

Comment 30

[John Keiser (jkeiser)]

Attachment: Patch v1.1

- fixes Darin's comments excepting those already noted
- fixes typo wherein we were using the raw filestream instead of the
bufferedstream (no visible difference, but there is a performance one
presumably)
- fixes nsMultiplexInputStream::ReadSegments() so that it recognizes earlier
when a writer is done, and so that it continues to call ReadSegments on a
particular stream until either a writer is done or no more data can be
extracted from the stream.  This fixes bug 130301 in a different way than it
was fixed before (I verified that this continues to fix the problem).

To elaborate on that last bit, the problem in bug 130301 was that
nsMultiplexStream::ReadSegments() was skipping the current stream when the
writer did not consume all data.  This is because ReadSegments() was *assuming*
that when it did not receive all the data it asked for, that the stream was
done giving it data.  This assumption is bad.  Waiting until the stream returns
0 bytes fixes it, *and* guarantees that ReadSegments() will be called one more
time before EOF so that I don't have to check PR_Available() within
ReadSegments.

Tested this again to verify it sends new data, and as always, I am submitting
this with the patched build.

Comment 31

[Darin Fisher]

i really believe that ReadSegments on a nsFileInputStream should be discouraged.
 it is not an efficient solution.  if callers want to call ReadSegments, then
they should use a buffered input stream, which is a much much performant
solution because it stores the buffer between reads.

point taken w.r.t. (DELETE_ON_CLOSE | OPEN_ON_READ)... i hadn't considered the
case of a user recreating the file.

i still don't understand what is going to be done about notifying the user when
the POST data has dissappeared.  what happens in that case?  session history
holds a reference to your multiplex input stream, but the file input stream
references a deleted file.  if the multiplex input stream suppresses errors from
the substreams, then won't we simply POST an empty stream?  that seems
completely broken.  the POST operation should fail with a meaningful error
message.  does it?

Comment 32

[John Keiser (jkeiser)]

I will make the nsFileInputStream change since that is explicitly what you want.
 I don't personally see the point of denying people the ability to do
ReadSegments (it's easy enough to put a comment there), but maybe people would
just try it, find that it works, and use it, not thinking about performance.

> i still don't understand what is going to be done about notifying the user when
> the POST data has dissappeared.  what happens in that case?  session history
> holds a reference to your multiplex input stream, but the file input stream
> references a deleted file.  if the multiplex input stream suppresses errors from
> the substreams, then won't we simply POST an empty stream?  that seems
> completely broken.  the POST operation should fail with a meaningful error
> message.  does it?

We won't POST an empty stream.  The structure of the MIME stream is like this:

- headers+content-length (this will not error out if file is gone)
- multiplex stream
  - some string form data
  - a file
  - some string form data
  - another file
  - some final string form data

If a file is deleted, everything else will still POST.  The content-length will
still be calculated correctly (it adds 0 for the length of the file stream when
it finds the file gone) and the file stream itself will simply be empty.  This
is *precisely* what should happen when the file is not there, so the stream
uploads exactly the way it should

Comment 33

[Darin Fisher]

> ReadSegments (it's easy enough to put a comment there), but maybe people would
> just try it, find that it works, and use it, not thinking about performance.

exactly my point.  we don't want to make it so easy for people to write bad
code.  we should do what we can to encourage better design, etc.


> it finds the file gone) and the file stream itself will simply be empty.  This
> is *precisely* what should happen when the file is not there, so the stream
> uploads exactly the way it should

huh? what!! if the file has dissappeared, and we still POST and make things look
correct.. the result is still not correct.  the file did not get uploaded as the
user expected.  maybe they forgot that they deleted the file they meant to
upload or mistakenly deleted it.  we should inform them of the fact that the
file no longer exists.  silently POSTing the wrong thing is misleading to the user.

Comment 34

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

What do we do when the user writes an invalid path in the <input type=file>? 
IMHO we should ideally behave the same way if the file is removed and a repost 
is performed.

Comment 35

[Darin Fisher]

it looks like we don't do any checking at all :(

try submitting a patch to bugzilla with a non-existant file path... instead of
an alert box, which i'd expect, you have to wait for bugzilla to tell you that
the patch file is empty.  IMO this is a bug.  we should alert the user to the
fact that the file is non-existent (or maybe we don't have sufficient
permissions to access the file?).  whatever the case may be, the user should be
notified.  granted, this problem is arguably not this bug, but the current patch
to nsMultiplexInputStream seems to work against the "need to alert the user of a
non-existent file when POSTing" problem.

Comment 36

[John Keiser (jkeiser)]

I'm convinced on that point, at least--I think we can make sure that *non-empty
filenames* have an actual filename in them before submitting.  But users do some
wacky things with these file inputs, I'm not sure how this will impact them. 
They got mad when I wouldn't send content-type and filename for empty files
(instead of just name/value), for example.  It broke servers.

As long as we can detect this in the MIMEInputStream *before* we send out any
data via ReadSegments or Read, I can just remove the error-ignore behavior.  But
what happens if the submit errors out halfway through sending the headers to the
server?  And what happens, *period*, if the submission errors out?  Does a
dialog pop up?

Comment 37

[Darin Fisher]

a dialog should popup, but probably doesn't.  at the very least, we should
cancel out the transaction, which would happen if you allowed the error to
propagate.  then some higher level (nsWebShell::EndPageLoad) could examine the
error code (possibly translated from NS_FAILED(rv) to
NS_ERROR_FAILED_TO_SUBMIT_FORM or whatever at the HTTP level) and prompt the
user with an error dialog.

that's how i think it should happen, but as you can see it involves fixing up
some other code to hook up the error to a dialog.

btw: what does IE do?  does it silently POST "the wrong thing" like we do?  or
does it alert the user?

Comment 38

[John Keiser (jkeiser)]

IE submits the bogus filename and empty file, the way that our behavior would
work with this patch.  However, I agree with you that that is the wrong thing to
do.  Empty filenames should submit fine but bogus filenames should not submit at
all.

As long as the submission will just silently fail if the stream returns an
error, and everything will continue to work, then I'm OK with removing
mSkipStreamsOnError--though a bug needs to be opened to pop up a dialog.  The
dialog is far, far out of scope for this bug (unless I can do it in a few lines).

Comment 39

[Darin Fisher]

yup, leave the UI work for another bug... sounds good to me.

Comment 40

[John Keiser (jkeiser)]

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

Comment 41

[John Keiser (jkeiser)]

Attachment: Patch v1.9.9

OK, this is the final patch--semantically.  I have to fix up whitespace since
my editor in Windows did some automatic file fixup of its own.	This patch is
cvs diff -uw.

Changes:
- nsMultiplexInputStream can now handle BASE_STREAM_ERROR
- nsMultiplexInputStream no longer does mSkipStreamsOnError.
- nsIFileInputStream::OPEN_ON_READ has become REOPEN_ON_REWIND, which only
reopens the file when you Seek(0).
- nsDocShell now handles errors from Seek().

So this patch specifically has been tested with:
- submit empty file, nonexistent file and regular file
- user can move or delete file after it has been submitted
- user deletes or moves file after it submits and then reloads
- regular reload after submit still shows data
- large form submit
- submitting this patch with patched build

Two bugs need to be found or filed regardless of when this patch goes in (just
a reminder to self):
1. nsDocShell::DoLoadURI() does not let the user know when the POST stream
fails.	It just bails the submit.
2. input type=file should check whether the file has an error and pop up a
dialog proactively.

Comment 42

[John Keiser (jkeiser)]

Attachment: Patch v2.0

OK, this is the version of the patch with whitespace changes and easier to
read.

Comment 43

[John Keiser (jkeiser)]

Bug 137944 filed on making input type="file" check if file exists *before*
submitting.  Bug 137946 files on making POST resubmission tell the user if
something goes wrong while submitting.

Comment 44

[Darin Fisher]

Comment on attachment 79601 [details] [diff] [review]
Patch v2.0

>Index: netwerk/base/src/nsFileStreams.cpp

>+    if (mFD) {
>+      rv = Close();
>+    if (NS_FAILED(rv)) return rv;

indentation

>+nsFileInputStream::ReadSegments(nsWriteSegmentFun aWriter, void* aClosure,
>+                                PRUint32 aCount, PRUint32* aResult)
> {
>+    return NS_ERROR_NOT_IMPLEMENTED;
> }

please add a NS_NOTREACHED assertion plus a comment as to why this is not
implemented.


>Index: xpcom/io/nsMultiplexInputStream.cpp

>+        PRUint32 read;
>+        rv = stream->ReadSegments(ReadSegCb, &state, aCount, &read);
...
>+        if (rv == NS_BASE_STREAM_CLOSED) {
...
>+        if (read == 0 || rv == NS_BASE_STREAM_CLOSED) {

isn't the case of |rv == NS_BASE_STREAM_CLOSED| already handled above?

otherwise, patch looks good!

Comment 45

[John Keiser (jkeiser)]

Attachment: Patch v2.0.1

- fixes Darin's comments

Comment 46

[Darin Fisher]

Comment on attachment 79652 [details] [diff] [review]
Patch v2.0.1

sr=darin

Comment 47

[Doug Turner (:dougt)]

Comment on attachment 79652 [details] [diff] [review]
Patch v2.0.1

Overall looks okay.  Just a couple of questions and nits.

dangling ) in your comment above DELETE_ON_CLOSE

Seek(0) should be "seek(0".  What if the users wants to seek(10) or any other
value?	Is zero somehow a magic number?

+    *aResult = channel;
+    NS_ADDREF(*aResult);

I know that this isn't you, but the above should be done in one line:

NS_ADDREF(*aResult = channel);

Must you mess around with braces changes in
netwerk/base/src/nsBufferedStreams.cpp?

I would much rather you dump those (G|S)ET_BOOLBIT.  It is easier to read
without them.  Not to mention that i always worry more when we use macros.

adding errror checking in the nsDocShell scares the crap out of me...
seriously.   Why do we need to check for an error when we weren't before.

Comment 48

[John Keiser (jkeiser)]

> dangling ) in your comment above DELETE_ON_CLOSE

Fixed.

> Seek(0) should be "seek(0".  What if the users wants to seek(10) or any other
> value?  Is zero somehow a magic number?

Zero is a magic number: rewind.  See nsMultiplexInputStream's Seek() method for
another example of this.  It doesn't make as much sense to me when doing
Seek(10).  If necessary, we could make any absolute Seek reopen the file.  I'm
not as comfy about that though ... Seek(0) is the only safe seek after you have
closed the file.

> +    *aResult = channel;
> +    NS_ADDREF(*aResult);
> 
> I know that this isn't you, but the above should be done in one line:
> 
> NS_ADDREF(*aResult = channel);

I'll take your word for it that it's more efficient, though it seems to me it
hinders readability.  Fixed.

> Must you mess around with braces changes in
> netwerk/base/src/nsBufferedStreams.cpp?

Fixed.

> I would much rather you dump those (G|S)ET_BOOLBIT.  It is easier to read
> without them.  Not to mention that i always worry more when we use macros.

Hmm, in this case it does make more sense because we are not trying to
selectively set various flags based on booleans.  Fixed.

> adding errror checking in the nsDocShell scares the crap out of me...
> seriously.   Why do we need to check for an error when we weren't before.

Seek(0) can now return FILE_NOT_FOUND.  DocShell needs to know about this so
that it can quit instead of going merrily forward, thinking it is going to be
able to read the stream even though the rewind failed.

Comment 49

[John Keiser (jkeiser)]

Attachment: Patch v2.0.2

- fixes dougt's comments

Comment 50

[Doug Turner (:dougt)]

Comment on attachment 79675 [details] [diff] [review]
Patch v2.0.2

great work.

Comment 51

[John Keiser (jkeiser)]

Comment on attachment 79675 [details] [diff] [review]
Patch v2.0.2

Carrying sr

Comment 52

[John Keiser (jkeiser)]

Comment on attachment 79675 [details] [diff] [review]
Patch v2.0.2

BTW, this patch had a change to nsHTMLSelectElement in it that won't hurt
anything but is not part of the patch.	It will not be committed.

Comment 53

[John Keiser (jkeiser)]

checked in to trunk.

Comment 54

[scottputterman]

vladimire, can you test this on the trunk and update the bug with the results?

This patch is pretty big.  In addition to the actual bug, what areas are
affected and need to be tested?

Comment 55

[John Keiser (jkeiser)]

Just about anything that uses files is potentially affected (though not likely).
 Smoketest suggested.  I have listed what I have tested in the bug, and that's a
decently large amount of stuff.  But I haven't tested everything.

Comment 56

[Mike Kaply [:mkaply]]

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

Comment 57

[Vladimir Ermakov]

verifying on build 2002-04-19-03-trunk win2k, buid 2002-04-19-03-trunk and
2002-04-19-10-trunk on linux,  
While testing on OSX, I came across bug 138645. It makes the keyboard unusable
after I the file is selected.

Comment 58

[Vadim Berezniker]

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

Comment 59

[scottputterman]

adding adt1.0.0-.  Let's look at this again for rtm.

Comment 60

[Matthias Versen [:Matti]]

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

Comment 61

[Asa Dotzler [:asa]]

removing from 138000

Comment 62

[R.K.Aa.]

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

Comment 63

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for approval to checkin to the 1.0 branch.  Please
get drivers a= again, since their previous approval is more than 3 days old. 
After, checking in, please add the fixed1.0 keyword.

Comment 64

[Randell Jesup [:jesup] (needinfo me)]

re-approved for branch checkin.  Please commit asap.

Comment 65

[Matthias Versen [:Matti]]

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

Comment 66

[John Keiser (jkeiser)]

Checked in to branch.

Comment 67

[Boris Zbarsky [:bzbarsky]]

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

Comment 68

[Matthias Versen [:Matti]]

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

Comment 69

[Alexandru Savulov]

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

Comment 70

[Boris Zbarsky [:bzbarsky]]

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

Comment 71

[Boris Zbarsky [:bzbarsky]]

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

Comment 72

[Vladimir Ermakov]

verifying on build 2002-07-17-xx-1.0 win98
and on build 2002-07-17-07-1.0 linux Redhat

Comment 73

[Antonio Passarelli]

looks like is back in version 40.0.3 .
file (*.war) size is aprox 20Mb

Comment 74

[Boris Zbarsky [:bzbarsky]]

Antonio, could you please file a new bug with clear steps to reproduce and cc me on it?  Thank you!

Comment 75

[Antonio Passarelli]

Boris, for sure.
Fact is that that development environment is not available anymore, also firefox has been updated to 41.0.2, so it's virtually impossible to faithfully reproduce it.
As soon and if possible, a test on a simple upload file form will be executed and, if happens again a new bug will be filed and you'll be the first CCed.
Thank you for attention.

PS additional info that can be added is that the subject file is generated by maven and uploaded to a customized Tomcat7 which automatically deploys the archive.