Fix the case where the browser livelocks if we cannot open a file.

Fix the case where the browser livelocks if we cannot open a file.

If one tries to upload a file that one doesn't have read access to,
the browser livelocks. It tries to read from the file, gets nothing
but spins forever because it knows that it hasn't finished reading.

To address this, firstly we add a check at stat() time to make sure
that we can read the file. However, this doesn't take care of the case
where the access() call was incorrect, or the permissions have changed
under us. In this case, we replace the missing file with NULs.

BUG=30850
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42152

[darin (slow to review), 2010-01-12 07:57:45]

http://codereview.chromium.org/541022/diff/1/5
File net/base/upload_data.cc (right):

http://codereview.chromium.org/541022/diff/1/5#newcode28
net/base/upload_data.cc:28: if (!file_util::PathIsReadable(file_path_))
on windows, it might be nice to combine these two calls since you can get the
file size from an open file handle.

[wtc, 2010-01-12 19:51:02]

I assume the livelock occurs in the while loop in
UploadDataStream::FillBuf(), right?

I have some questions and suggested changes.

http://codereview.chromium.org/541022/diff/1/3
File base/file_util_posix.cc (right):

http://codereview.chromium.org/541022/diff/1/3#newcode309
base/file_util_posix.cc:309: // If the path doesn't exist, test the parent dir.
Do you know why we do this?  This behavior is not specified
in file_util.h.

The Windows implementation doesn't seem to do this.

http://codereview.chromium.org/541022/diff/1/4
File base/file_util_win.cc (right):

http://codereview.chromium.org/541022/diff/1/4#newcode221
base/file_util_win.cc:221: CreateFile(path.value().c_str(), FILE_READ_DATA,
I'm surprised that we have to test if a file is readable
by trying to open it for reading.  But I couldn't find a
suitable Win32 function for this purpose with a quick
search.

In NSPR we're using the CRT function _access:
http://msdn.microsoft.com/en-us/library/1w06ktdy(VS.80).aspx
Not sure if that's better than our solution here.

http://codereview.chromium.org/541022/diff/1/6
File net/base/upload_data.h (right):

http://codereview.chromium.org/541022/diff/1/6#newcode35
net/base/upload_data.h:35: int64 expected_length() const { return
expected_file_length_; }
Nit: rename this method expected_file_size and the
underlying member expected_file_size_.

http://codereview.chromium.org/541022/diff/1/6#newcode68
net/base/upload_data.h:68: mutable int64 expected_file_length_;
Does this member need to be mutable?

http://codereview.chromium.org/541022/diff/1/7
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/541022/diff/1/7#newcode81
net/base/upload_data_stream.cc:81: // If a file is visible to us but not
readable then, at this point,
What does "visible but not readable" mean?

http://codereview.chromium.org/541022/diff/1/7#newcode104
net/base/upload_data_stream.cc:104: memset(buf_->data() + buf_len_, 0, count);
Please add a comment here to note that we're filling in NULs.
I had a little trouble finding this code when I read your
comment at lines 82-83.

Is filling in NULs the right thing to do?  Can we fail the
upload or somehow break out of the while loop to avoid the
livelock?

[agl, 2010-01-25 14:14:09]

I decided to change how this works. Rather than cope with the case where the
file access changes between check and use, we now open the file once and use the
handle from there. The only situation that can still mess us up is if the file
shrinks between stat() and read(). In this case we still pad with NULs.

http://codereview.chromium.org/541022/diff/1/4
File base/file_util_win.cc (right):

http://codereview.chromium.org/541022/diff/1/4#newcode221
base/file_util_win.cc:221: CreateFile(path.value().c_str(), FILE_READ_DATA,
On 2010/01/12 19:51:02, wtc wrote:
> In NSPR we're using the CRT function _access:
> http://msdn.microsoft.com/en-us/library/1w06ktdy%28VS.80%29.aspx
> Not sure if that's better than our solution here.

Ah, _access looks good. I've switched to using that. Thanks.

http://codereview.chromium.org/541022/diff/1/5
File net/base/upload_data.cc (right):

http://codereview.chromium.org/541022/diff/1/5#newcode28
net/base/upload_data.cc:28: if (!file_util::PathIsReadable(file_path_))
On 2010/01/12 07:57:45, darin wrote:
> on windows, it might be nice to combine these two calls since you can get the
> file size from an open file handle.

PathIsReadable no longer obtains a file handle, so there's nothing to merge now.

http://codereview.chromium.org/541022/diff/1/6
File net/base/upload_data.h (right):

http://codereview.chromium.org/541022/diff/1/6#newcode35
net/base/upload_data.h:35: int64 expected_length() const { return
expected_file_length_; }
On 2010/01/12 19:51:02, wtc wrote:
> Nit: rename this method expected_file_size and the
> underlying member expected_file_size_.

Done.

http://codereview.chromium.org/541022/diff/1/6#newcode68
net/base/upload_data.h:68: mutable int64 expected_file_length_;
On 2010/01/12 19:51:02, wtc wrote:
> Does this member need to be mutable?

It's changed inside of GetContextLength, which is a const member. However, I
tried working backwards and removing consts and it wasn't so bad. So this is no
longer mutable.

http://codereview.chromium.org/541022/diff/1/7
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/541022/diff/1/7#newcode81
net/base/upload_data_stream.cc:81: // If a file is visible to us but not
readable then, at this point,
On 2010/01/12 19:51:02, wtc wrote:
> What does "visible but not readable" mean?

For example, /etc/shadow is visible (you can ls -l it), but you cannot read it.
If there's a clearer way to express this please let me know.

http://codereview.chromium.org/541022/diff/1/7#newcode104
net/base/upload_data_stream.cc:104: memset(buf_->data() + buf_len_, 0, count);
On 2010/01/12 19:51:02, wtc wrote:
> Please add a comment here to note that we're filling in NULs.
> I had a little trouble finding this code when I read your
> comment at lines 82-83.

Done.

> Is filling in NULs the right thing to do?  Can we fail the
> upload or somehow break out of the while loop to avoid the
> livelock?

At this point in the code, I don't think that there's a lot that we can do. Note
that, in the current patch set, the only situation where we pad with NULs is if
the file shrunk under us.

[wtc, 2010-01-25 20:07:11]

http://codereview.chromium.org/541022/diff/1/7
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/541022/diff/1/7#newcode81
net/base/upload_data_stream.cc:81: // If a file is visible to us but not
readable then, at this point,
On 2010/01/25 14:14:09, agl wrote:
>
> For example, /etc/shadow is visible (you can ls -l it), but you cannot read
it.
> If there's a clearer way to express this please let me know.

Got it.  Thanks for the explanation.

How about "visible in directory listing but not readable"?

[wtc, 2010-01-26 01:43:22]

I made a pass through Patch Set 3, but I couldn't
understand it completely.  Sorry.  I will try again
tomorrow when my head is fresh.

I don't understand your changes to upload_data.{h,cc}.
SetToFilePathRange becomes much more complicated, while
GetContentLength becomes much simpler.  Why?

Re: RefCountedPlatformFile: I'm not convinced that this
class needs to be reference-counted.  I think you just
need a regular class that closes the PlatformFile handle
in the destructor.  Did I miss something?

http://codereview.chromium.org/541022/diff/6001/6002
File base/platform_file.h (right):

http://codereview.chromium.org/541022/diff/6001/6002#newcode60
base/platform_file.h:60: bool PlatformFileLength(PlatformFile file, uint64*
out_length);
Nit: How about renaming this function PlatformFileSize
or GetPlatformFileSize?

http://codereview.chromium.org/541022/diff/6001/6002#newcode70
base/platform_file.h:70: file_ = kInvalidPlatformFileValue;
Nit: this isn't necessary in a destructor, but I can see it's
useful for debugging.

http://codereview.chromium.org/541022/diff/6001/6002#newcode78
base/platform_file.h:78: PlatformFile take() {
In our source tree, this operation is called "release".
See the release() methods of scoped_ptr of scoped_refptr.

http://codereview.chromium.org/541022/diff/6001/6004
File base/platform_file_win.cc (right):

http://codereview.chromium.org/541022/diff/6001/6004#newcode82
base/platform_file_win.cc:82: *out_length = size;
Does this compile on the Windows try bot?

I believe you need to say
  *out_length = size.QuadPart;
because LARGE_INTEGER is a struct:
http://msdn.microsoft.com/en-us/library/aa383713(VS.85).aspx

http://codereview.chromium.org/541022/diff/6001/6005
File net/base/file_stream.h (right):

http://codereview.chromium.org/541022/diff/6001/6005#newcode53
net/base/file_stream.h:53: // with the non-default constructor.
Should we document that this version of Open can only be
called on an empty FileStream object (one that hasn't been
associated with a base::PlatformFile)?

http://codereview.chromium.org/541022/diff/6001/6006
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/541022/diff/6001/6006#newcode346
net/base/file_stream_posix.cc:346: int FileStream::Open(base::PlatformFile file,
int open_flags) {
Should we re-implement the non-default constructor to
call this Open method?

http://codereview.chromium.org/541022/diff/6001/6006#newcode359
net/base/file_stream_posix.cc:359: return 0;
Nit: change 0 => OK.

http://codereview.chromium.org/541022/diff/6001/6007
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/541022/diff/6001/6007#newcode186
net/base/file_stream_win.cc:186: }
Add
  return OK;
at the end of the function.  See the original Open method
above.

http://codereview.chromium.org/541022/diff/6001/6009
File net/base/upload_data.h (right):

http://codereview.chromium.org/541022/diff/6001/6009#newcode60
net/base/upload_data.h:60: base::PlatformFile TakePlatformFile();
Take => Release?

http://codereview.chromium.org/541022/diff/6001/6010
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/541022/diff/6001/6010#newcode93
net/base/upload_data_stream.cc:93: // If the file was truncated between the time
that we opened it and
rv == 0 means the file was truncated.

rv < 0 means Read failed with an error.

So the comment "If the file was truncated ..." isn't exactly
accurate.

[agl, 2010-01-26 13:39:03]

> I don't understand your changes to upload_data.{h,cc}.
> SetToFilePathRange becomes much more complicated, while
> GetContentLength becomes much simpler.  Why?

Previously, SetToFilePathRange just recorded the information about the file and
GetContentLength stat'ed the file. Since we want to open the file as soon as
possible and keep the file handle around, SetToFilePathRange now opens the file
and stat's it. GetContentLength is reduced to just echoing the previously
discovered information.

> Re: RefCountedPlatformFile: I'm not convinced that this
> class needs to be reference-counted.  I think you just
> need a regular class that closes the PlatformFile handle
> in the destructor.  Did I miss something?

The issue is that Elements are treated as values and copied around. I looked at
removing this, but it seems that UploadData is used in IPC serialisation and is
limited by the requirements of those templates. Making the PlatformFiles
reference counted seemed to be the best way to be obviously correct.

http://codereview.chromium.org/541022/diff/6001/6002
File base/platform_file.h (right):

http://codereview.chromium.org/541022/diff/6001/6002#newcode60
base/platform_file.h:60: bool PlatformFileLength(PlatformFile file, uint64*
out_length);
On 2010/01/26 01:43:22, wtc wrote:
> Nit: How about renaming this function PlatformFileSize
> or GetPlatformFileSize?

Done.

http://codereview.chromium.org/541022/diff/6001/6002#newcode78
base/platform_file.h:78: PlatformFile take() {
On 2010/01/26 01:43:22, wtc wrote:
> In our source tree, this operation is called "release".
> See the release() methods of scoped_ptr of scoped_refptr.

Done.

http://codereview.chromium.org/541022/diff/6001/6004
File base/platform_file_win.cc (right):

http://codereview.chromium.org/541022/diff/6001/6004#newcode82
base/platform_file_win.cc:82: *out_length = size;
On 2010/01/26 01:43:22, wtc wrote:
> Does this compile on the Windows try bot?

I sent off the code review before the Windows try bot was done and figured this
out. Thanks.

http://codereview.chromium.org/541022/diff/6001/6005
File net/base/file_stream.h (right):

http://codereview.chromium.org/541022/diff/6001/6005#newcode53
net/base/file_stream.h:53: // with the non-default constructor.
On 2010/01/26 01:43:22, wtc wrote:
> Should we document that this version of Open can only be
> called on an empty FileStream object (one that hasn't been
> associated with a base::PlatformFile)?

Done.

http://codereview.chromium.org/541022/diff/6001/6006
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/541022/diff/6001/6006#newcode346
net/base/file_stream_posix.cc:346: int FileStream::Open(base::PlatformFile file,
int open_flags) {
On 2010/01/26 01:43:22, wtc wrote:
> Should we re-implement the non-default constructor to
> call this Open method?

Done.

http://codereview.chromium.org/541022/diff/6001/6006#newcode359
net/base/file_stream_posix.cc:359: return 0;
On 2010/01/26 01:43:22, wtc wrote:
> Nit: change 0 => OK.

Done.

http://codereview.chromium.org/541022/diff/6001/6007
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/541022/diff/6001/6007#newcode186
net/base/file_stream_win.cc:186: }
On 2010/01/26 01:43:22, wtc wrote:
> Add
>   return OK;
> at the end of the function.  See the original Open method
> above.

Done.

http://codereview.chromium.org/541022/diff/6001/6009
File net/base/upload_data.h (right):

http://codereview.chromium.org/541022/diff/6001/6009#newcode60
net/base/upload_data.h:60: base::PlatformFile TakePlatformFile();
On 2010/01/26 01:43:22, wtc wrote:
> Take => Release?

Done.

http://codereview.chromium.org/541022/diff/6001/6010
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/541022/diff/6001/6010#newcode93
net/base/upload_data_stream.cc:93: // If the file was truncated between the time
that we opened it and
On 2010/01/26 01:43:22, wtc wrote:
> rv == 0 means the file was truncated.
> 
> rv < 0 means Read failed with an error.
> 
> So the comment "If the file was truncated ..." isn't exactly
> accurate.

Done.

[wtc, 2010-01-26 20:00:26]

OK, I understand the CL much better now.

The code in upload_data_stream.cc is very complicated, so
I'd like to read it again after you have addressed my
comments below.

Most of my comments below are just nits.  Please read
in particular the comment marked with "BAD".  I hope we
can do something about it.

http://codereview.chromium.org/541022/diff/6014/9001
File base/platform_file.h (right):

http://codereview.chromium.org/541022/diff/6014/9001#newcode59
base/platform_file.h:59: // *out_length is set to the length of the file, in
bytes.
Nit: also change out_length to out_size or just 'size'.

http://codereview.chromium.org/541022/diff/6014/9001#newcode62
base/platform_file.h:62: class RefCountedPlatformFile :
Please add a comment to explain how this class is used.
You said you need it reference-counted so that it can be
copied around.  I guess this is why we can't add
DISALLOW_COPY_AND_ASSIGN to this class?

http://codereview.chromium.org/541022/diff/6014/9005
File net/base/file_stream_posix.cc (left):

http://codereview.chromium.org/541022/diff/6014/9005#oldcode302
net/base/file_stream_posix.cc:302: // If the file handle is opened with
base::PLATFORM_FILE_ASYNC, we need to
Please copy this comment (lines 302-303 in the original code)
to the new Open function.

http://codereview.chromium.org/541022/diff/6014/9005
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/541022/diff/6014/9005#newcode299
net/base/file_stream_posix.cc:299: FileStream::FileStream(base::PlatformFile
file, int flags) {
This constructor still needs the
    : file_(base::kInvalidPlatformFileValue)
initializer, so that the IsOpen() test in Open() will
return false.

http://codereview.chromium.org/541022/diff/6014/9006
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/541022/diff/6014/9006#newcode123
net/base/file_stream_win.cc:123: FileStream::FileStream(base::PlatformFile file,
int flags) {
Same here: this constructor needs the
    : file_(INVALID_HANDLE_VALUE)
initializer.

http://codereview.chromium.org/541022/diff/6014/9007
File net/base/upload_data.cc (right):

http://codereview.chromium.org/541022/diff/6014/9007#newcode47
net/base/upload_data.cc:47: uint64 file_length;
Nit: file_length => file_size

http://codereview.chromium.org/541022/diff/6014/9007#newcode53
net/base/upload_data.cc:53: if (offset > file_length)
It seems that we should return in this case (the offset is
invalid).  Your code is equivalent to returning because it
causes 'length' to be set to 0.  It's just less obvious.

http://codereview.chromium.org/541022/diff/6014/9008
File net/base/upload_data.h (right):

http://codereview.chromium.org/541022/diff/6014/9008#newcode28
net/base/upload_data.h:28: file_range_length_(kuint64max), file_(NULL) {
Nit: the file_(NULL) initializer isn't necessary because
the default constructor for file_ takes care of that.

Do you think kuint64max is the right initial value for
file_range_length_?  It seems that we should initialize
file_range_length_ to 0, as you do at the beginning of
SetToFilePathRange.

http://codereview.chromium.org/541022/diff/6014/9009
File net/base/upload_data_stream.cc (left):

http://codereview.chromium.org/541022/diff/6014/9009#oldcode78
net/base/upload_data_stream.cc:78: next_element_remaining_ = 0;  // Default to
reading nothing.
I think we still need this line, otherwise next_element_remaining_ may not have
the
right value if next_element_stream_.Seek fails.
Or is next_element_remaining_ already 0 at this point?

http://codereview.chromium.org/541022/diff/6014/9009
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/541022/diff/6014/9009#newcode74
net/base/upload_data_stream.cc:74: int flags = base::PLATFORM_FILE_OPEN |
BAD: the 'flags' value here needs to be kept in sync with
the 'flags' value we use in upload_data.cc.  This is bad.
I hope we can come up with a better solution.  For example,
does next_element_stream_ really need the flags we used to
open the file?

http://codereview.chromium.org/541022/diff/6014/9009#newcode76
net/base/upload_data_stream.cc:76:
next_element_stream_.Open(element.ReleasePlatformFile(), flags);
Would be nice to document why it's safe ignore the return
value of next_element_stream_.Open().  (Because we already
checked above that next_element_stream_ is not open.)

http://codereview.chromium.org/541022/diff/6014/9009#newcode90
net/base/upload_data_stream.cc:90: int n =
Let's still use the variable name 'count' here to avoid
potential style nit with one-letter variable name 'n'.

[agl, 2010-02-10 15:40:50]

http://codereview.chromium.org/541022/diff/6014/9001
File base/platform_file.h (right):

http://codereview.chromium.org/541022/diff/6014/9001#newcode59
base/platform_file.h:59: // *out_length is set to the length of the file, in
bytes.
On 2010/01/26 20:00:27, wtc wrote:
> Nit: also change out_length to out_size or just 'size'.

Done.

http://codereview.chromium.org/541022/diff/6014/9001#newcode62
base/platform_file.h:62: class RefCountedPlatformFile :
On 2010/01/26 20:00:27, wtc wrote:
> Please add a comment to explain how this class is used.
> You said you need it reference-counted so that it can be
> copied around.  I guess this is why we can't add
> DISALLOW_COPY_AND_ASSIGN to this class?

Have added comment, mostly pointing to the comments in base/ref_counted.h. Also,
we can add DISALLOW_COPY_AND_ASSIGN since users should always get a pointer to
this class via a scoped_refptr, so I have done so.

http://codereview.chromium.org/541022/diff/6014/9005
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/541022/diff/6014/9005#newcode299
net/base/file_stream_posix.cc:299: FileStream::FileStream(base::PlatformFile
file, int flags) {
On 2010/01/26 20:00:27, wtc wrote:
> This constructor still needs the
>     : file_(base::kInvalidPlatformFileValue)
> initializer, so that the IsOpen() test in Open() will
> return false.

Done.

http://codereview.chromium.org/541022/diff/6014/9006
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/541022/diff/6014/9006#newcode123
net/base/file_stream_win.cc:123: FileStream::FileStream(base::PlatformFile file,
int flags) {
On 2010/01/26 20:00:27, wtc wrote:
> Same here: this constructor needs the
>     : file_(INVALID_HANDLE_VALUE)
> initializer.

Done.

http://codereview.chromium.org/541022/diff/6014/9007
File net/base/upload_data.cc (right):

http://codereview.chromium.org/541022/diff/6014/9007#newcode47
net/base/upload_data.cc:47: uint64 file_length;
On 2010/01/26 20:00:27, wtc wrote:
> Nit: file_length => file_size

Done.

http://codereview.chromium.org/541022/diff/6014/9007#newcode53
net/base/upload_data.cc:53: if (offset > file_length)
On 2010/01/26 20:00:27, wtc wrote:
> It seems that we should return in this case (the offset is
> invalid).  Your code is equivalent to returning because it
> causes 'length' to be set to 0.  It's just less obvious.

Done.

http://codereview.chromium.org/541022/diff/6014/9008
File net/base/upload_data.h (right):

http://codereview.chromium.org/541022/diff/6014/9008#newcode28
net/base/upload_data.h:28: file_range_length_(kuint64max), file_(NULL) {
On 2010/01/26 20:00:27, wtc wrote:
> Nit: the file_(NULL) initializer isn't necessary because
> the default constructor for file_ takes care of that.
> 
> Do you think kuint64max is the right initial value for
> file_range_length_?  It seems that we should initialize
> file_range_length_ to 0, as you do at the beginning of
> SetToFilePathRange.

Done.

http://codereview.chromium.org/541022/diff/6014/9009
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/541022/diff/6014/9009#newcode74
net/base/upload_data_stream.cc:74: int flags = base::PLATFORM_FILE_OPEN |
On 2010/01/26 20:00:27, wtc wrote:
> BAD: the 'flags' value here needs to be kept in sync with
> the 'flags' value we use in upload_data.cc.  This is bad.
> I hope we can come up with a better solution.  For example,
> does next_element_stream_ really need the flags we used to
> open the file?

There is some use to documenting what you expect to be able to do with the file
descriptor, but I agree that it's not a lot. It doesn't bother me too much since
you don't actually have to keep this in sync with anything; the values are only
used for DCHECKs. However, I've changed the FileStream API to only take an
is_async flag now and removed this.

http://codereview.chromium.org/541022/diff/6014/9009#newcode76
net/base/upload_data_stream.cc:76:
next_element_stream_.Open(element.ReleasePlatformFile(), flags);
On 2010/01/26 20:00:27, wtc wrote:
> Would be nice to document why it's safe ignore the return
> value of next_element_stream_.Open().  (Because we already
> checked above that next_element_stream_ is not open.)

Done.

http://codereview.chromium.org/541022/diff/6014/9009#newcode90
net/base/upload_data_stream.cc:90: int n =
On 2010/01/26 20:00:27, wtc wrote:
> Let's still use the variable name 'count' here to avoid
> potential style nit with one-letter variable name 'n'.

Done.

[wtc, 2010-02-11 14:58:34]

Darin: I'm out of the office today.  If you have some spare
time, could you review this CL today?  If not, I'll review
it tomorrow.

The most important (and also most difficult) part to review
is upload_data_stream.cc.

Thanks!

[darin (slow to review), 2010-02-12 09:16:22]

looks great overall.  just some comments / questions...

http://codereview.chromium.org/541022/diff/11015/10002
File base/platform_file.h (right):

http://codereview.chromium.org/541022/diff/11015/10002#newcode66
base/platform_file.h:66: public
base::RefCountedThreadSafe<RefCountedPlatformFile> {
it'd be nice if we had something general like shared_ptr<T> so that we could
easily turn any class into something that can be reference counted.

http://codereview.chromium.org/541022/diff/11015/10010
File net/base/upload_data.h (right):

http://codereview.chromium.org/541022/diff/11015/10010#newcode58
net/base/upload_data.h:58: return static_cast<uint64>(bytes_.size());
implicit cast from size_t to uint64 should be ok, no?

http://codereview.chromium.org/541022/diff/11015/10010#newcode120
net/base/upload_data.h:120: std::vector<Element>& elements() {
this goes against google style.  if you need to mutate the returned vector, then
better change it to be a pointer type instead of a reference type.  reference
types should only be used with const.

http://codereview.chromium.org/541022/diff/11015/10011
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/541022/diff/11015/10011#newcode78
net/base/upload_data_stream.cc:78:
next_element_stream_.Open(element.ReleasePlatformFile(),
it's too bad this function call forces us to make data_ be non-const.

http://codereview.chromium.org/541022/diff/11015/10011#newcode95
net/base/upload_data_stream.cc:95: static_cast<uint64>(size_remaining)));
same question about casting from size_t to uint64.

[wtc, 2010-02-12 22:50:35]

LGTM.  Please update the description of the CL to reflect
the current code.

I suggest a way to undo the removal of 'const' below.
(The RefCountedPlatformFile type probably will also be
unnecessary.)  It requires us to leave the files open
until all of them are uploaded.

http://codereview.chromium.org/541022/diff/11015/10002
File base/platform_file.h (right):

http://codereview.chromium.org/541022/diff/11015/10002#newcode59
base/platform_file.h:59: // *out_length is set to the length of the file, in
bytes.
*out_length => *size

http://codereview.chromium.org/541022/diff/11015/10002#newcode63
base/platform_file.h:63: // the handle is closed. See the comments in
base/ref_counted.h for details on
Nit: "handle" => "file handle" or "PlatformFile"

http://codereview.chromium.org/541022/diff/11015/10005
File net/base/file_stream.h (right):

http://codereview.chromium.org/541022/diff/11015/10005#newcode35
net/base/file_stream.h:35: // |is_async| if true, operations are performed
asynchronously.
Nit: |is_async| if true => If |is_async| is true

http://codereview.chromium.org/541022/diff/11015/10005#newcode135
net/base/file_stream.h:135: int open_flags_;
I think we can simply remove open_flags_.  I'm not sure if
those DCHECKs are useful...

But it's fine to not touch this.

http://codereview.chromium.org/541022/diff/11015/10006
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/541022/diff/11015/10006#newcode463
net/base/file_stream_posix.cc:463: // We better be open for reading.
reading => writing

http://codereview.chromium.org/541022/diff/11015/10010
File net/base/upload_data.h (right):

http://codereview.chromium.org/541022/diff/11015/10010#newcode120
net/base/upload_data.h:120: std::vector<Element>& elements() {
It seems that we can undo the 'const' removal now.
I don't see the expected_file_length_ member any more.

http://codereview.chromium.org/541022/diff/11015/10011
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/541022/diff/11015/10011#newcode78
net/base/upload_data_stream.cc:78:
next_element_stream_.Open(element.ReleasePlatformFile(),
On 2010/02/12 09:16:22, darin wrote:
> it's too bad this function call forces us to make data_ be non-const.

Another option is to leave the files open until the entire
upload is done.  I think this is fine because the new code
already opens the files earlier than before, so it should
be okay to close them later than before.

[wtc, 2010-02-12 22:53:40]

http://codereview.chromium.org/541022/diff/11015/10011
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/541022/diff/11015/10011#newcode79
net/base/upload_data_stream.cc:79: false /* not async */);
Also, replacing open_flags with is_async here still has
the same problem that we know how another class
(UploadData::Element) opened the file.  Since we can't
eliminate this problem entirely, we can just go back to
open_flags to make this changelist smaller.

[wtc, 2010-02-20 05:54:22]

I believe I found a bug.  Please see my comment marked with
"BUG" below.

http://codereview.chromium.org/541022/diff/16001/16005
File net/base/file_stream.h (right):

http://codereview.chromium.org/541022/diff/16001/16005#newcode47
net/base/file_stream.h:47: void Release();
Instead of adding a Release() method, perhaps we can just
add a 'bool owns_file_' argument to the Open() method at
line 59, and change the Close() method to close file_ only
if owns_file_ is true.

http://codereview.chromium.org/541022/diff/16001/16005#newcode61
net/base/file_stream.h:61: // Returns true if Open succeeded and Close has not
been called.
Should this comment be updated to say "Close or Release has
not been called"?

http://codereview.chromium.org/541022/diff/16001/16009
File net/base/upload_data.cc (right):

http://codereview.chromium.org/541022/diff/16001/16009#newcode43
net/base/upload_data.cc:43: file_->release();
BUG:  I believe file_->release() leaks the PlatformFile handle!

We should say
  file_ = NULL;

http://codereview.chromium.org/541022/diff/16001/16009#newcode62
net/base/upload_data.cc:62: file_path_= path;
Why do we set file_path_ to path in this case?
This no longer matches the comments on lines 76-77 in
upload_data.h.

http://codereview.chromium.org/541022/diff/16001/16009#newcode79
net/base/upload_data.cc:79: file_ = new base::RefCountedPlatformFile(file);
We may no longer need the RefCountedPlatformFile class now.

http://codereview.chromium.org/541022/diff/16001/16010
File net/base/upload_data.h (left):

http://codereview.chromium.org/541022/diff/16001/16010#oldcode98
net/base/upload_data.h:98: uint64 GetContentLength() const;
Can we add back this 'const'?

http://codereview.chromium.org/541022/diff/16001/16012
File net/base/upload_data_stream.h (left):

http://codereview.chromium.org/541022/diff/16001/16012#oldcode17
net/base/upload_data_stream.h:17: explicit UploadDataStream(const UploadData*
data);
Can we add back this 'const'  and the 'const' at line 45 now?
I guess the data_->CloseFiles() call in the destructor require
that data_ be non-const.