New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 713358 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Apr 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Remove buffer size sanity check from URLRequestJob::Read

Project Member Reported by kapishnikov@chromium.org, Apr 19 2017

Issue description

The current DCHECK_LT(buf_size, 1000000) in URLRequestJob::Read() prevents client from passing a buffer greater than 1MB. In some cases, the client may want to allocate enough space to fit the whole response data into a single uninterrupted buffers, which size is equal to the value of the content-length header. Breaking the buffer into multiple parts and then coalescing them imposes the performance penalty. Making multiple calls and moving the buffer position/limit defeats the purpose and introduces code complexity.

Some context:
https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/eDh3xb3kn5Q

Internal bug:
http://b/37406581
 

Comment 1 by mmenke@chromium.org, Apr 19 2017

Content-Length header isn't terribly meaningful for compressed content.  Also, even with multiple read calls, reads will generally only be about 32k, anyways, due to internal buffer sizes (With compression, 32k before decompression using the outermost filter).

Anyhow, I don't feel too strongly about the DCHECK - very few things use URLRequest directly, anyways, just pointing out that larger input buffer size doesn't necessarily have much impact on number of read calls.
That is a good point. It is also the matter of convenience. If a client expects some content, let's say, 2MB long. It is much easier to allocate a single direct 2MB buffer and reuse it until it fills up, rather than creating multiple buffers or setting an artificial limit on the buffer. If the client underestimated the size, a new buffer can be created. I think the DCHECK made sense for the internal sanity check but since the buffer is exposed to the client, we should not enforce some arbitrary size.
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2375f72a0c62be56a6cb80fd2daff66c7748fda4

commit 2375f72a0c62be56a6cb80fd2daff66c7748fda4
Author: kapishnikov <kapishnikov@chromium.org>
Date: Thu Apr 20 03:12:42 2017

Remove buffer size sanity check from URLRequestJob::Read

BUG= 713358 

Review-Url: https://codereview.chromium.org/2829833002
Cr-Commit-Position: refs/heads/master@{#465881}

[modify] https://crrev.com/2375f72a0c62be56a6cb80fd2daff66c7748fda4/net/url_request/url_request_job.cc

Status: Fixed (was: Assigned)
Components: Internals>Network
Components: -Internals>Network>HTTP

Sign in to add a comment