DNS: Don't spin on unexpected EOF reading TCP response

DNS: Don't spin on unexpected EOF reading TCP response

DnsTCPAttempt doesn't handle when read returns 0, which is EOF.
It keeps reading over and over, which will peg the CPU.

Fix it so it stops reading with ERR_CONNECTION_CLOSED,
and add unit tests for both ways the connection can close.

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

[Deprecated (see juliatuttle), 2014-07-31 00:57:56]

PTAL, mmenke.

FYI, szym.

[mmenke, 2014-07-31 16:08:14]

On 2014/07/31 00:57:56, ttuttle wrote:
> PTAL, mmenke.
> 
> FYI, szym.

Think we should probably have a unit test for both the cases.  Do you really
think we need histograms for this?

[szym, 2014-07-31 16:19:56]

On 2014/07/31 16:08:14, mmenke wrote:
> On 2014/07/31 00:57:56, ttuttle wrote:
> > PTAL, mmenke.
> > 
> > FYI, szym.
> 
> Think we should probably have a unit test for both the cases.  Do you really
> think we need histograms for this?

Thanks for catching this, ttuttle.
Agreed on both points with mmenke.

[Deprecated (see juliatuttle), 2014-07-31 22:04:06]

On 2014/07/31 16:19:56, szym wrote:
> On 2014/07/31 16:08:14, mmenke wrote:
> > On 2014/07/31 00:57:56, ttuttle wrote:
> > > PTAL, mmenke.
> > > 
> > > FYI, szym.
> > 
> > Think we should probably have a unit test for both the cases.  Do you really
> > think we need histograms for this?
> 
> Thanks for catching this, ttuttle.
> Agreed on both points with mmenke.

Added unit tests, removed histograms, and commented a particularly confusing bit
of another unit test. PTAL?

[mmenke, 2014-08-01 15:38:43]

https://codereview.chromium.org/422323004/diff/40001/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/422323004/diff/40001/net/dns/dns_transaction....
net/dns/dns_transaction.cc:497: return ReadIntoBuffer();
I think this would be much clearer with:

// Note no argument.  Could have a DCHECK_NE(ERR_IO_PENDING, rv);
// before the call to it, if you wanted, though not sure it gets us
// much, given our other DCHECKs about that.
int DoReadResponse() {
  next_state_ = STATE_READ_RESPONSE_COMPLETE;
  return socket_->Read(
      buffer_.get(),
      buffer_->BytesRemaining(),
      base::Bind(&DnsTCPAttempt::OnIOComplete,
                 base::Unretained(this)));
}

int DoReadResponseComplete(int rv) {
  DCHECK_NE(ERR_IO_PENDING, rv);
  if (rv < 0)
    return rv;
  if (rv == 0)
    return ERR_CONNECTION_CLOSED;

  if (buffer_->BytesRemaining() > 0) {
    next_state_ = STATE_READ_RESPONSE;
    return OK;
  }

  // Parse the response here.
}


And do the same when reading the length.  This is how a lot of loops in net/
work, and it makes all reads, including the first one, follow the same basic
path through the code.

https://codereview.chromium.org/422323004/diff/40001/net/dns/dns_transaction_...
File net/dns/dns_transaction_unittest.cc (right):

https://codereview.chromium.org/422323004/diff/40001/net/dns/dns_transaction_...
net/dns/dns_transaction_unittest.cc:104: void AddError(int error, IoMode mode) {
Maybe AddReadError?

https://codereview.chromium.org/422323004/diff/40001/net/dns/dns_transaction_...
net/dns/dns_transaction_unittest.cc:967: ASYNC,
Maybe make this sync, too?

[Deprecated (see juliatuttle), 2014-08-01 22:01:11]

PTAL?

https://codereview.chromium.org/422323004/diff/40001/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/422323004/diff/40001/net/dns/dns_transaction....
net/dns/dns_transaction.cc:497: return ReadIntoBuffer();
On 2014/08/01 15:38:42, mmenke wrote:
> I think this would be much clearer with:
> 
> // Note no argument.  Could have a DCHECK_NE(ERR_IO_PENDING, rv);
> // before the call to it, if you wanted, though not sure it gets us
> // much, given our other DCHECKs about that.
> int DoReadResponse() {
>   next_state_ = STATE_READ_RESPONSE_COMPLETE;
>   return socket_->Read(
>       buffer_.get(),
>       buffer_->BytesRemaining(),
>       base::Bind(&DnsTCPAttempt::OnIOComplete,
>                  base::Unretained(this)));
> }
> 
> int DoReadResponseComplete(int rv) {
>   DCHECK_NE(ERR_IO_PENDING, rv);
>   if (rv < 0)
>     return rv;
>   if (rv == 0)
>     return ERR_CONNECTION_CLOSED;
> 
>   if (buffer_->BytesRemaining() > 0) {
>     next_state_ = STATE_READ_RESPONSE;
>     return OK;
>   }
> 
>   // Parse the response here.
> }
> 
> 
> And do the same when reading the length.  This is how a lot of loops in net/
> work, and it makes all reads, including the first one, follow the same basic
> path through the code.

Done.

https://codereview.chromium.org/422323004/diff/40001/net/dns/dns_transaction_...
File net/dns/dns_transaction_unittest.cc (right):

https://codereview.chromium.org/422323004/diff/40001/net/dns/dns_transaction_...
net/dns/dns_transaction_unittest.cc:104: void AddError(int error, IoMode mode) {
On 2014/08/01 15:38:42, mmenke wrote:
> Maybe AddReadError?

Done.

https://codereview.chromium.org/422323004/diff/40001/net/dns/dns_transaction_...
net/dns/dns_transaction_unittest.cc:967: ASYNC,
On 2014/08/01 15:38:42, mmenke wrote:
> Maybe make this sync, too?

Done.

[mmenke, 2014-08-04 15:44:29]

LGTM

https://codereview.chromium.org/422323004/diff/60001/net/dns/dns_transaction_...
File net/dns/dns_transaction_unittest.cc (right):

https://codereview.chromium.org/422323004/diff/60001/net/dns/dns_transaction_...
net/dns/dns_transaction_unittest.cc:912: // itself succeeds.
Add a period after section, and capitalize since.

https://codereview.chromium.org/422323004/diff/60001/net/dns/dns_transaction_...
net/dns/dns_transaction_unittest.cc:912: // itself succeeds.
Also, should the "question section" be replaced with "answer section"?

[Deprecated (see juliatuttle), 2014-08-04 19:29:23]

https://codereview.chromium.org/422323004/diff/60001/net/dns/dns_transaction_...
File net/dns/dns_transaction_unittest.cc (right):

https://codereview.chromium.org/422323004/diff/60001/net/dns/dns_transaction_...
net/dns/dns_transaction_unittest.cc:912: // itself succeeds.
On 2014/08/04 15:44:28, mmenke wrote:
> Also, should the "question section" be replaced with "answer section"?

Done.

https://codereview.chromium.org/422323004/diff/60001/net/dns/dns_transaction_...
net/dns/dns_transaction_unittest.cc:912: // itself succeeds.
On 2014/08/04 15:44:28, mmenke wrote:
> Add a period after section, and capitalize since.

Done.

[Deprecated (see juliatuttle), 2014-08-04 19:29:36]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-08-04 19:31:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ttuttle@chromium.org/422323004/80001

[commit-bot: I haz the power, 2014-08-04 20:49:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-05 04:56:52]

Change committed as 287456