Return a soft error when we detect the 3rd party problem causing bug 27870.

Return a soft error when we detect the 3rd party problem causing bug 27870.

Return an error when winsock reports it wrote more than we asked it to write.
Remove some debugging.

BUG=27870
TEST=none

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

[wtc, 2010-02-26 20:13:01]

http://codereview.chromium.org/660194/diff/1/4
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/660194/diff/1/4#newcode521
net/socket/tcp_client_socket_win.cc:521: return static_cast<int>(num);
The same correction needs to be applied to the synchronous
completion case.

http://codereview.chromium.org/660194/diff/1/4#newcode698
net/socket/tcp_client_socket_win.cc:698: int rv = num_bytes;
This may generate "uninitialized memory read" error if
WSAGetOverlappedResult fails, because it may not set
num_bytes on error.

I suggest doing just
  int rv;
here, and then use my suggested change below.

http://codereview.chromium.org/660194/diff/1/4#newcode707
net/socket/tcp_client_socket_win.cc:707: num_bytes =
core_->write_buffer_length_;
This should be
  rv = core_->write_buffer_length_;
because you're passing 'rv' to DoWriteCallback.

[wtc, 2010-02-26 20:14:09]

http://codereview.chromium.org/660194/diff/1/4
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/660194/diff/1/4#newcode698
net/socket/tcp_client_socket_win.cc:698: int rv = num_bytes;
On 2010/02/26 20:13:01, wtc wrote:
>
> I suggest doing just
>   int rv;
> here, and then use my suggested change below.

This suggestion isn't entirely right either.  In any case, I'm sure
you can fix this.

[eroman, 2010-02-26 22:31:03]

LGTM.

Based on the discussions in the bug thread, you may want to leave the failure
case as a CHECK, or an error code.

My personal preference is towards an error code (perhaps
ERR_WINSOCK_UNEXPECTED_WRITTEN_BYTES). Since if the error happens infrequently
users may not notice. But if it does happen with regularity, they can retrieve
the error code from the error page, do a search for it and find our bug report
describing the problem and fix.

http://codereview.chromium.org/660194/diff/9/12
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/660194/diff/9/12#newcode705
net/socket/tcp_client_socket_win.cc:705: // than was available.  We try to work
around this problem by silently
Alternately we could return a failure code. Either seems fine to me, but others
on the bug thead may feel differently.

http://codereview.chromium.org/660194/diff/9/12#newcode706
net/socket/tcp_client_socket_win.cc:706: // correcting it.
Can you link to the bug (so interested parties can see the discussion)?

http://codereview.chromium.org/660194/diff/9/12#newcode708
net/socket/tcp_client_socket_win.cc:708: core_->write_buffer_length_ << ".";
nit: can you indent with either 4 spaces, or start the next line with a << and
line it up with the << from previous one.

[vandebo (ex-Chrome), 2010-02-26 22:57:34]

An error code seems to be the way to go.
I added a check at the synchronous return site as well.

http://codereview.chromium.org/660194/diff/1/4
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/660194/diff/1/4#newcode698
net/socket/tcp_client_socket_win.cc:698: int rv = num_bytes;
On 2010/02/26 20:13:01, wtc wrote:
> This may generate "uninitialized memory read" error if
> WSAGetOverlappedResult fails, because it may not set
> num_bytes on error.

I'm not sure how purify works, but I think valgrind does the more intelligent
thing and only emits the error when it's actually used.  Regardless I fix this.

http://codereview.chromium.org/660194/diff/1/4#newcode707
net/socket/tcp_client_socket_win.cc:707: num_bytes =
core_->write_buffer_length_;
On 2010/02/26 20:13:01, wtc wrote:
> This should be
>   rv = core_->write_buffer_length_;
> because you're passing 'rv' to DoWriteCallback.

Yea wow. Fixed.

http://codereview.chromium.org/660194/diff/9/12#newcode705
net/socket/tcp_client_socket_win.cc:705: LOG(ERROR) << "Corrected written amount
from " << num_bytes << " to " <<
On 2010/02/26 22:31:03, eroman wrote:
> Alternately we could return a failure code. Either seems fine to me, but
others
> on the bug thead may feel differently.

I think an error code is the best compromise.

http://codereview.chromium.org/660194/diff/9/12#newcode706
net/socket/tcp_client_socket_win.cc:706: core_->write_buffer_length_ << ".";
On 2010/02/26 22:31:03, eroman wrote:
> Can you link to the bug (so interested parties can see the discussion)?

Done.

http://codereview.chromium.org/660194/diff/9/12#newcode708
net/socket/tcp_client_socket_win.cc:708: }
On 2010/02/26 22:31:03, eroman wrote:
> nit: can you indent with either 4 spaces, or start the next line with a << and
> line it up with the << from previous one.

Done.

[eroman, 2010-02-26 23:10:29]

http://codereview.chromium.org/660194/diff/1005/1009
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/660194/diff/1005/1009#newcode518
net/socket/tcp_client_socket_win.cc:518: if (static_cast<int>(num) > buf_len) {
Note that |num| is of type DWORD (which is 32-bit unsigned).

So comparing by casting isn't safe, since static_cast<int>(num) could be
negative.

I suggest reversing the direction of the cast to avoid overflow:

if (num > static_cast<DWORD>(buf_len))

http://codereview.chromium.org/660194/diff/1005/1009#newcode709
net/socket/tcp_client_socket_win.cc:709: rv = num_bytes;
Same comment here as above. Be careful that |num_bytes| could be negative when
cast to an int.

[vandebo (ex-Chrome), 2010-02-27 00:16:38]

On 2010/02/26 23:10:29, eroman wrote:
> http://codereview.chromium.org/660194/diff/1005/1009#newcode518
> net/socket/tcp_client_socket_win.cc:518: if (static_cast<int>(num) > buf_len)
{
> Note that |num| is of type DWORD (which is 32-bit unsigned).
> 
> So comparing by casting isn't safe, since static_cast<int>(num) could be
> negative.

Indeed.  However in both cases the value is passed into the callback, which
takes an int.  So I casted the DWORD value to an int and checked that it is
non-negative. > 2G in a single write shouldn't happen anyway.

[eroman, 2010-02-27 00:31:10]

http://codereview.chromium.org/660194/diff/1012/19
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/660194/diff/1012/19#newcode519
net/socket/tcp_client_socket_win.cc:519: CHECK(rv >= 0);
Why not make this par to of the "if"?

IMO this is the same error we are checking for below
(ERR_WINSOCK_UNEXPECTED_WRITTEN_BYTES) ----> winsock returned some garbage byte
count, which is huge.

http://codereview.chromium.org/660194/diff/1012/19#newcode712
net/socket/tcp_client_socket_win.cc:712: CHECK(rv >= 0);
same comment.

[vandebo (ex-Chrome), 2010-02-27 00:49:36]

On 2010/02/27 00:31:10, eroman wrote:
> http://codereview.chromium.org/660194/diff/1012/19
> File net/socket/tcp_client_socket_win.cc (right):
> 
> http://codereview.chromium.org/660194/diff/1012/19#newcode519
> net/socket/tcp_client_socket_win.cc:519: CHECK(rv >= 0);
> Why not make this par to of the "if"?
> 
> IMO this is the same error we are checking for below
> (ERR_WINSOCK_UNEXPECTED_WRITTEN_BYTES) ----> winsock returned some garbage
byte
> count, which is huge.
> 
> http://codereview.chromium.org/660194/diff/1012/19#newcode712
> net/socket/tcp_client_socket_win.cc:712: CHECK(rv >= 0);
> same comment.

We might cause an excessively large return value with a bug above here, but it
seems unlikely.  Added these to the if clause.

[eroman, 2010-02-27 01:00:10]

LGTM!