Limit the total memory usage for Stream instances

Limit the total memory usage for Stream instances

Stream instances report their memory usage to StreamRegistry to get
approval. If rejected, they unregisters themselves.

writer_ and reader_ are cleared immediately when memory usage
violation happens. Added task runner DCHECKs to ByteStreamReaderImpl
and ByteStreamWriterImpl so that we can check we're not violating
thread restriction.

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

[kinuko, 2013-08-14 09:32:23]

https://codereview.chromium.org/22908008/diff/5001/content/browser/byte_strea...
File content/browser/byte_stream.cc (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/byte_strea...
content/browser/byte_stream.cc:243: return input_contents_size_ +
output_size_used_;
Could this overflow?

https://codereview.chromium.org/22908008/diff/5001/content/browser/byte_stream.h
File content/browser/byte_stream.h (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/byte_strea...
content/browser/byte_stream.h:159: virtual size_t TotalBufferedBytes() const =
0;
nit: GetTotalBufferedBytes() per naming convention?

https://codereview.chromium.org/22908008/diff/5001/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:635: if (!stream.get())
This can happen now?

...oh I see, looks like yes.

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
File content/browser/streams/stream.cc (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
content/browser/streams/stream.cc:75: size_t new_size =
writer_->TotalBufferedBytes() + size;
ditto. Could this overflow?

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
File content/browser/streams/stream_registry.cc (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
content/browser/streams/stream_registry.cc:58: total_memory_usage_ -=
iter->second->last_total_buffered_bytes();
Can we DCHECK this doesn't make total_memory_usage < 0 ?

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
File content/browser/streams/stream_registry.h (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
content/browser/streams/stream_registry.h:61: uint64 max_memory_usage_;
Any reason you use different types for total and max?

https://codereview.chromium.org/22908008/diff/8001/content/browser/streams/st...
File content/browser/streams/stream.cc (right):

https://codereview.chromium.org/22908008/diff/8001/content/browser/streams/st...
content/browser/streams/stream.cc:83: reader_.reset();
Are we going to call Abort() here after your other patch is landed?

https://codereview.chromium.org/22908008/diff/8001/content/browser/streams/st...
File content/browser/streams/stream_registry.cc (right):

https://codereview.chromium.org/22908008/diff/8001/content/browser/streams/st...
content/browser/streams/stream_registry.cc:72: size_t usage_of_others =
total_memory_usage_ - last_size;
nit: using iter->second->last_total_buffered_bytes() for last_size  may feel
slightly more consistent with UnregisterStream.

[tyoshino (SeeGerritForStatus), 2013-08-15 04:15:33]

https://codereview.chromium.org/22908008/diff/5001/content/browser/byte_strea...
File content/browser/byte_stream.cc (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/byte_strea...
content/browser/byte_stream.cc:243: return input_contents_size_ +
output_size_used_;
On 2013/08/14 09:32:23, kinuko wrote:
> Could this overflow?

Added overflow check in Write()

https://codereview.chromium.org/22908008/diff/5001/content/browser/byte_stream.h
File content/browser/byte_stream.h (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/byte_strea...
content/browser/byte_stream.h:159: virtual size_t TotalBufferedBytes() const =
0;
On 2013/08/14 09:32:23, kinuko wrote:
> nit: GetTotalBufferedBytes() per naming convention?

Done.

https://codereview.chromium.org/22908008/diff/5001/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:635: if (!stream.get())
On 2013/08/14 09:32:23, kinuko wrote:
> This can happen now?
> 
> ...oh I see, looks like yes.

Yes. Needs to ignore failures now.

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
File content/browser/streams/stream.cc (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
content/browser/streams/stream.cc:75: size_t new_size =
writer_->TotalBufferedBytes() + size;
On 2013/08/14 09:32:23, kinuko wrote:
> ditto. Could this overflow?

Added overflow check.

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
File content/browser/streams/stream_registry.cc (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
content/browser/streams/stream_registry.cc:58: total_memory_usage_ -=
iter->second->last_total_buffered_bytes();
On 2013/08/14 09:32:23, kinuko wrote:
> Can we DCHECK this doesn't make total_memory_usage < 0 ?

Done.

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
File content/browser/streams/stream_registry.h (right):

https://codereview.chromium.org/22908008/diff/5001/content/browser/streams/st...
content/browser/streams/stream_registry.h:61: uint64 max_memory_usage_;
On 2013/08/14 09:32:23, kinuko wrote:
> Any reason you use different types for total and max?

Changed to use size_t

https://codereview.chromium.org/22908008/diff/8001/content/browser/streams/st...
File content/browser/streams/stream.cc (right):

https://codereview.chromium.org/22908008/diff/8001/content/browser/streams/st...
content/browser/streams/stream.cc:83: reader_.reset();
On 2013/08/14 09:32:23, kinuko wrote:
> Are we going to call Abort() here after your other patch is landed?

Right!

https://codereview.chromium.org/22908008/diff/8001/content/browser/streams/st...
File content/browser/streams/stream_registry.cc (right):

https://codereview.chromium.org/22908008/diff/8001/content/browser/streams/st...
content/browser/streams/stream_registry.cc:72: size_t usage_of_others =
total_memory_usage_ - last_size;
On 2013/08/14 09:32:23, kinuko wrote:
> nit: using iter->second->last_total_buffered_bytes() for last_size  may feel
> slightly more consistent with UnregisterStream.

I wondered if either way is better. OK, I'll do so.

Also updated method description and renamed method name to reflect current
implementation.

[tyoshino (SeeGerritForStatus), 2013-08-16 11:14:09]

https://codereview.chromium.org/22908008/diff/21001/content/browser/byte_stre...
File content/browser/byte_stream.cc (right):

https://codereview.chromium.org/22908008/diff/21001/content/browser/byte_stre...
content/browser/byte_stream.cc:197:
DCHECK(my_task_runner_->RunsTasksOnCurrentThread());
reverted these changes. download code crashes if I add this. see
https://codereview.chromium.org/22925003/.

[kinuko, 2013-08-19 15:30:24]

lgtm, sorry for the delay.

https://codereview.chromium.org/22908008/diff/25002/content/browser/streams/s...
File content/browser/streams/stream_registry.h (right):

https://codereview.chromium.org/22908008/diff/25002/content/browser/streams/s...
content/browser/streams/stream_registry.h:37: // returns false.. Otherwise,
updates |total_memory_usage_| and returns true.
nit: double periods '..'

[tyoshino (SeeGerritForStatus), 2013-08-20 07:18:38]

creis, could you please review this for jam?

https://codereview.chromium.org/22908008/diff/25002/content/browser/streams/s...
File content/browser/streams/stream_registry.h (right):

https://codereview.chromium.org/22908008/diff/25002/content/browser/streams/s...
content/browser/streams/stream_registry.h:37: // returns false.. Otherwise,
updates |total_memory_usage_| and returns true.
On 2013/08/19 15:30:24, kinuko wrote:
> nit: double periods '..'

Done.

[Charlie Reis, 2013-08-20 22:24:52]

Looks mostly fine, but I'm not sure why there's two different limits being used
in the checks.

https://codereview.chromium.org/22908008/diff/39001/content/browser/byte_stre...
File content/browser/byte_stream.cc (right):

https://codereview.chromium.org/22908008/diff/39001/content/browser/byte_stre...
content/browser/byte_stream.cc:215: output_size_used_ - input_contents_size_;
Any reason not to use GetTotalBufferedBytes here?  I think that would be clearer
and less susceptible to future bugs if the implementation changes.

https://codereview.chromium.org/22908008/diff/39001/content/browser/byte_stre...
content/browser/byte_stream.cc:270: input_contents_size_ + output_size_used_ >
total_buffer_size_;
Again, please use GetTotalBufferedBytes here and below for clarity.

https://codereview.chromium.org/22908008/diff/39001/content/browser/byte_stre...
File content/browser/byte_stream_unittest.cc (right):

https://codereview.chromium.org/22908008/diff/39001/content/browser/byte_stre...
content/browser/byte_stream_unittest.cc:103: TEST_F(ByteStreamTest,
ByteStream_PushBack) {
Is it possible to add a test that writing more data than we have space for
fails?

https://codereview.chromium.org/22908008/diff/39001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/22908008/diff/39001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:680: if
(!GetStreamForURL(src_url))
Might help to document when GetStreamForURL can return null.

https://codereview.chromium.org/22908008/diff/39001/content/browser/streams/s...
File content/browser/streams/stream_registry.cc (right):

https://codereview.chromium.org/22908008/diff/39001/content/browser/streams/s...
content/browser/streams/stream_registry.cc:77: usage_of_others + new_size >
max_memory_usage_)
nit: Please change the second half of the check to look more like the first
(i.e., new_size > max_memory_usage_ - usage_of_others).

Why do we need both checks?  Should we just be comparing against
kDefaultMaxMemoryUsage in all the places of this CL rather than max(), or is
there a reason to let things go up to max() in some places?

https://codereview.chromium.org/22908008/diff/39001/content/browser/streams/s...
File content/browser/streams/stream_registry.h (right):

https://codereview.chromium.org/22908008/diff/39001/content/browser/streams/s...
content/browser/streams/stream_registry.h:45: void set_max_memory_usage(size_t
size) {
We usually end such method names with _for_testing() to help prevent others from
calling them in normal code.

https://codereview.chromium.org/22908008/diff/39001/content/browser/streams/s...
content/browser/streams/stream_registry.h:57: // with this 5Aregistry.
Is 5Aregistry a typo?

[tyoshino (SeeGerritForStatus), 2013-08-21 05:37:28]

https://codereview.chromium.org/22908008/diff/39001/content/browser/byte_stre...
File content/browser/byte_stream.cc (right):

https://codereview.chromium.org/22908008/diff/39001/content/browser/byte_stre...
content/browser/byte_stream.cc:215: output_size_used_ - input_contents_size_;
On 2013/08/20 22:24:52, creis wrote:
> Any reason not to use GetTotalBufferedBytes here?  I think that would be
clearer
> and less susceptible to future bugs if the implementation changes.

No reason. Replaced.

https://codereview.chromium.org/22908008/diff/39001/content/browser/byte_stre...
content/browser/byte_stream.cc:270: input_contents_size_ + output_size_used_ >
total_buffer_size_;
On 2013/08/20 22:24:52, creis wrote:
> Again, please use GetTotalBufferedBytes here and below for clarity.

Done.

https://codereview.chromium.org/22908008/diff/39001/content/browser/byte_stre...
File content/browser/byte_stream_unittest.cc (right):

https://codereview.chromium.org/22908008/diff/39001/content/browser/byte_stre...
content/browser/byte_stream_unittest.cc:103: TEST_F(ByteStreamTest,
ByteStream_PushBack) {
On 2013/08/20 22:24:52, creis wrote:
> Is it possible to add a test that writing more data than we have space for
> fails?

Added a test that passes size_t max as len parameter.

https://codereview.chromium.org/22908008/diff/39001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/22908008/diff/39001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:680: if
(!GetStreamForURL(src_url))
On 2013/08/20 22:24:52, creis wrote:
> Might help to document when GetStreamForURL can return null.

Added some comments here and on GetStreamForURL definition.

https://codereview.chromium.org/22908008/diff/39001/content/browser/streams/s...
File content/browser/streams/stream_registry.cc (right):

https://codereview.chromium.org/22908008/diff/39001/content/browser/streams/s...
content/browser/streams/stream_registry.cc:77: usage_of_others + new_size >
max_memory_usage_)
On 2013/08/20 22:24:52, creis wrote:
> nit: Please change the second half of the check to look more like the first
> (i.e., new_size > max_memory_usage_ - usage_of_others).
> 
> Why do we need both checks?  Should we just be comparing against
> kDefaultMaxMemoryUsage in all the places of this CL rather than max(), or is
> there a reason to let things go up to max() in some places?

Oh.. right. Removed numeric_limits check from Stream and StreamRegistry.

https://codereview.chromium.org/22908008/diff/39001/content/browser/streams/s...
File content/browser/streams/stream_registry.h (right):

https://codereview.chromium.org/22908008/diff/39001/content/browser/streams/s...
content/browser/streams/stream_registry.h:45: void set_max_memory_usage(size_t
size) {
On 2013/08/20 22:24:52, creis wrote:
> We usually end such method names with _for_testing() to help prevent others
from
> calling them in normal code.

Done.

https://codereview.chromium.org/22908008/diff/39001/content/browser/streams/s...
content/browser/streams/stream_registry.h:57: // with this 5Aregistry.
On 2013/08/20 22:24:52, creis wrote:
> Is 5Aregistry a typo?

Yes.. Fixed.

[Charlie Reis, 2013-08-21 19:13:55]

LGTM with the suggestions below, if there's a reason for ByteStreamWriter and
Stream to use different limits.  (If not, perhaps make them use the same limit?)

https://codereview.chromium.org/22908008/diff/56011/content/browser/byte_stre...
File content/browser/byte_stream.cc (right):

https://codereview.chromium.org/22908008/diff/56011/content/browser/byte_stre...
content/browser/byte_stream.cc:214: size_t space_limit =
std::numeric_limits<size_t>::max() -
Can you add a comment explaining why this class uses max() while the Stream and
StreamRegistry classes use kDefaultMaxMemoryUsage?  I'm not sure I understand.

https://codereview.chromium.org/22908008/diff/56011/content/browser/byte_stre...
File content/browser/byte_stream_unittest.cc (right):

https://codereview.chromium.org/22908008/diff/56011/content/browser/byte_stre...
content/browser/byte_stream_unittest.cc:595: std::numeric_limits<size_t>::max(),
Is this actually creating a ByteStream of max() size?  Maybe it would be less
stressful on the bots to create a smaller ByteStream and then try to write max()
bytes into it?  (I'm not sure if this is allocating memory for the stream or
not.)

[tyoshino (SeeGerritForStatus), 2013-08-22 02:19:02]

https://codereview.chromium.org/22908008/diff/56011/content/browser/byte_stre...
File content/browser/byte_stream.cc (right):

https://codereview.chromium.org/22908008/diff/56011/content/browser/byte_stre...
content/browser/byte_stream.cc:214: size_t space_limit =
std::numeric_limits<size_t>::max() -
On 2013/08/21 19:13:56, creis wrote:
> Can you add a comment explaining why this class uses max() while the Stream
and
> StreamRegistry classes use kDefaultMaxMemoryUsage?  I'm not sure I understand.

ByteStream is used by other component (content/browser/download). So, I
refrained from setting limit. I've put TODO to talk to rdsmith about your
suggestion.

https://codereview.chromium.org/22908008/diff/56011/content/browser/byte_stre...
File content/browser/byte_stream_unittest.cc (right):

https://codereview.chromium.org/22908008/diff/56011/content/browser/byte_stre...
content/browser/byte_stream_unittest.cc:595: std::numeric_limits<size_t>::max(),
On 2013/08/21 19:13:56, creis wrote:
> Is this actually creating a ByteStream of max() size?  Maybe it would be less
> stressful on the bots to create a smaller ByteStream and then try to write
max()
> bytes into it?  (I'm not sure if this is allocating memory for the stream or
> not.)

buffer_size is used to throttle write operation. We don't actually allocate any
memory of the specified size.

[commit-bot: I haz the power, 2013-08-22 02:32:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoshino@chromium.org/22908008/70001

[commit-bot: I haz the power, 2013-08-22 07:43:35]

Change committed as 218933