net: Fix a regression that broke file uploading with http authentication

net: Fix a regression that broke file uploading with http authentication

This is a reland of r138168, which was reverted speculatively, but identified unrelated.

When uploading a file with http authentication, UploadData is reused
for a new UploadDataStream. Hence, we should close the files in UploadData
if already opened and read, so we can reread the files from the beginning.

The regression was caused by r123677.  Special thanks to thomas.themel for
identifying the offending patch and helping us to fix the issue.

BUG=128574
TEST=added a unit test; manually confirmed that uploading a file with basic authentication, and multi-round authentication with NTLM and Negotiate worked.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138191

[cbentzel, 2012-05-21 18:02:47]

On 2012/05/21 17:53:15, satorux1 wrote:

+asanka due to HTTP authentication-related issue

[willchan no longer on Chromium, 2012-05-21 18:12:51]

Can you update the changelist description with manual testing instructions,
beyond just the unittest?

https://chromiumcodereview.appspot.com/10408042/diff/1/net/base/upload_data.cc
File net/base/upload_data.cc (right):

https://chromiumcodereview.appspot.com/10408042/diff/1/net/base/upload_data.c...
net/base/upload_data.cc:125: file_stream_->CloseSync();
Is this call necessary? Can't we just delete the file_stream_?

https://chromiumcodereview.appspot.com/10408042/diff/1/net/base/upload_data.h
File net/base/upload_data.h (right):

https://chromiumcodereview.appspot.com/10408042/diff/1/net/base/upload_data.h...
net/base/upload_data.h:172: FileStream* file_stream_;
Hm, why isn't this a scoped_ptr? It looks like we own this.

[willchan no longer on Chromium, 2012-05-21 18:17:35]

https://chromiumcodereview.appspot.com/10408042/diff/1/net/base/upload_data_s...
File net/base/upload_data_stream_unittest.cc (right):

https://chromiumcodereview.appspot.com/10408042/diff/1/net/base/upload_data_s...
net/base/upload_data_stream_unittest.cc:174: scoped_ptr<UploadDataStream>
stream(new UploadDataStream(upload_data_));
Any special reason to use a scoped_ptr here?

https://chromiumcodereview.appspot.com/10408042/diff/1/net/base/upload_data_s...
net/base/upload_data_stream_unittest.cc:182: scoped_ptr<UploadDataStream>
stream(new UploadDataStream(upload_data_));
Ditto

[satorux1, 2012-05-21 19:37:00]

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

http://codereview.chromium.org/10408042/diff/1/net/base/upload_data.cc#newcod...
net/base/upload_data.cc:125: file_stream_->CloseSync();
On 2012/05/21 18:12:51, willchan wrote:
> Is this call necessary? Can't we just delete the file_stream_?

You are right, but I thought closing explicitly would make it clearer that this
code was violating the thread restrictions.

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

http://codereview.chromium.org/10408042/diff/1/net/base/upload_data.h#newcode172
net/base/upload_data.h:172: FileStream* file_stream_;
On 2012/05/21 18:12:51, willchan wrote:
> Hm, why isn't this a scoped_ptr? It looks like we own this.

IIRC, I once tried to make it a scoped_ptr but failed as the UploadData::Element
has to be copyable (the Element class is hold as std::vector<Element> in
UploadData) but making it a scoped_ptr makes it non-copyable.

http://codereview.chromium.org/10408042/diff/1/net/base/upload_data_stream_un...
File net/base/upload_data_stream_unittest.cc (right):

http://codereview.chromium.org/10408042/diff/1/net/base/upload_data_stream_un...
net/base/upload_data_stream_unittest.cc:174: scoped_ptr<UploadDataStream>
stream(new UploadDataStream(upload_data_));
On 2012/05/21 18:17:35, willchan wrote:
> Any special reason to use a scoped_ptr here?

Done.

http://codereview.chromium.org/10408042/diff/1/net/base/upload_data_stream_un...
net/base/upload_data_stream_unittest.cc:182: scoped_ptr<UploadDataStream>
stream(new UploadDataStream(upload_data_));
On 2012/05/21 18:17:35, willchan wrote:
> Ditto

Done.

[willchan no longer on Chromium, 2012-05-21 20:27:40]

lgtm

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

http://codereview.chromium.org/10408042/diff/1/net/base/upload_data.cc#newcod...
net/base/upload_data.cc:125: file_stream_->CloseSync();
On 2012/05/21 19:37:00, satorux1 wrote:
> On 2012/05/21 18:12:51, willchan wrote:
> > Is this call necessary? Can't we just delete the file_stream_?
> 
> You are right, but I thought closing explicitly would make it clearer that
this
> code was violating the thread restrictions. 

OIC. I think the ThreadRestrictions::ScopedAllowIO is enough documentation, so
the explicit CloseSync() is unnecessary IMO. Minor preference only. If you
disagree, then feel free to keep it.

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

http://codereview.chromium.org/10408042/diff/1/net/base/upload_data.h#newcode172
net/base/upload_data.h:172: FileStream* file_stream_;
On 2012/05/21 19:37:00, satorux1 wrote:
> On 2012/05/21 18:12:51, willchan wrote:
> > Hm, why isn't this a scoped_ptr? It looks like we own this.
> 
> IIRC, I once tried to make it a scoped_ptr but failed as the
UploadData::Element
> has to be copyable (the Element class is hold as std::vector<Element> in
> UploadData) but making it a scoped_ptr makes it non-copyable. 

Ugh, so fragile :( This code sucks.

[asanka, 2012-05-21 20:28:20]

LGTM.

[satorux1, 2012-05-21 20:40:46]

will submit shortly.

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

http://codereview.chromium.org/10408042/diff/1/net/base/upload_data.cc#newcod...
net/base/upload_data.cc:125: file_stream_->CloseSync();
On 2012/05/21 20:27:40, willchan wrote:
> On 2012/05/21 19:37:00, satorux1 wrote:
> > On 2012/05/21 18:12:51, willchan wrote:
> > > Is this call necessary? Can't we just delete the file_stream_?
> > 
> > You are right, but I thought closing explicitly would make it clearer that
> this
> > code was violating the thread restrictions. 
> 
> OIC. I think the ThreadRestrictions::ScopedAllowIO is enough documentation, so
> the explicit CloseSync() is unnecessary IMO. Minor preference only. If you
> disagree, then feel free to keep it.

Let me keep this for now as we also close explicitly in the destructor too.
We'll fix the thread restriction violation anyway, in the not distant future.

[commit-bot: I haz the power, 2012-05-21 20:45:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/satorux@chromium.org/10408042/9001

[commit-bot: I haz the power, 2012-05-21 20:45:53]

Presubmit check for 10408042-9001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use ScopedAllowIO. Post a task to the blocking pool or the
FILE thread instead.
    net/base/upload_data.cc:124

Presubmit checks took 7.1s to calculate.

[wtc, 2012-05-21 22:54:41]

Patch set 2 LGTM.  Feel free to ignore my suggestion below.
I'm just offering it for your reference.  Your solution results
in more compact code.

https://chromiumcodereview.appspot.com/10408042/diff/9001/net/base/upload_dat...
File net/base/upload_data.cc (right):

https://chromiumcodereview.appspot.com/10408042/diff/9001/net/base/upload_dat...
net/base/upload_data.cc:128: }
As a performance optimization, we can leave file_stream_
open and simply seek to the beginning of the file:

  if (file_stream_) {
    // Temporarily allow until fix: http://crbug.com/72001.
    base::ThreadRestrictions::ScopedAllowIO allow_io;
    int64 offset = file_range_offset_? file_range_offset_ : 0;
    int64 rv = file->SeekSync(FROM_BEGIN, offset);
    if (rv < 0) {
       DLOG(WARNING) << "Failed to seek \"" << file_path_.value()
                    << "\" to offset: " << offset << " (" << rv
                    << ")";
      file_stream_->CloseSync();
      delete file_stream_;
      file_stream_ = NULL;
    }
  }

[commit-bot: I haz the power, 2012-05-21 23:12:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/satorux@chromium.org/10408042/9001

[commit-bot: I haz the power, 2012-05-21 23:12:41]

Presubmit check for 10408042-9001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use ScopedAllowIO. Post a task to the blocking pool or the
FILE thread instead.
    net/base/upload_data.cc:124

Presubmit checks took 1.9s to calculate.

[wtc, 2012-05-21 23:13:11]

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

http://codereview.chromium.org/10408042/diff/1/net/base/upload_data.h#newcode172
net/base/upload_data.h:172: FileStream* file_stream_;

On 2012/05/21 19:37:00, satorux1 wrote:
>
> IIRC, I once tried to make it a scoped_ptr but failed as the
UploadData::Element
> has to be copyable (the Element class is hold as std::vector<Element> in
> UploadData) but making it a scoped_ptr makes it non-copyable.

Should we use base/memory/linked_ptr.h to deal with this?
    base::link_ptr<FileStream> file_stream_;
should allow UploadData::Element to be copyable.  But I'm
not sure about the current opinion about base/memory/linked_ptr.h.

[satorux1, 2012-05-21 23:14:40]

http://codereview.chromium.org/10408042/diff/9001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/10408042/diff/9001/net/base/upload_data.cc#new...
net/base/upload_data.cc:128: }
On 2012/05/21 22:54:41, wtc wrote:
> As a performance optimization, we can leave file_stream_
> open and simply seek to the beginning of the file:
> 
>   if (file_stream_) {
>     // Temporarily allow until fix: http://crbug.com/72001.
>     base::ThreadRestrictions::ScopedAllowIO allow_io;
>     int64 offset = file_range_offset_? file_range_offset_ : 0;
>     int64 rv = file->SeekSync(FROM_BEGIN, offset);
>     if (rv < 0) {
>        DLOG(WARNING) << "Failed to seek \"" << file_path_.value()
>                     << "\" to offset: " << offset << " (" << rv
>                     << ")";
>       file_stream_->CloseSync();
>       delete file_stream_;
>       file_stream_ = NULL;
>     }
>   } 

Thanks. I'm going to stick with my fix for now for easier merges to m19 and m20
branches. I'll revisit this at a later time.

[willchan no longer on Chromium, 2012-05-21 23:38:55]

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

http://codereview.chromium.org/10408042/diff/1/net/base/upload_data.h#newcode172
net/base/upload_data.h:172: FileStream* file_stream_;
On 2012/05/21 23:13:11, wtc wrote:
> 
> On 2012/05/21 19:37:00, satorux1 wrote:
> >
> > IIRC, I once tried to make it a scoped_ptr but failed as the
> UploadData::Element
> > has to be copyable (the Element class is hold as std::vector<Element> in
> > UploadData) but making it a scoped_ptr makes it non-copyable.
> 
> Should we use base/memory/linked_ptr.h to deal with this?
>     base::link_ptr<FileStream> file_stream_;
> should allow UploadData::Element to be copyable.  But I'm
> not sure about the current opinion about base/memory/linked_ptr.h.

I don't think we should do this. The fundamental problem is that this isn't
truly copyable. The FileStream should *not* be shared across
UploadData::Elements. The way it works right now is fragile, I think we rely on
convention that we don't actually ever copy the vector. The Elements are only
copied when resizing the vector.