Implement content::WebSocketBlobSender

Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

Also add IPC messages WebSocketHostMsg_SendBlob and
WebSocketMsg_BlobSendComplete which expose the new functionality.

The renderer-side functionality is implemented in http://crrev.com/1574213003.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp714/edit

BUG=571656
TEST=content_unittests

Committed: https://crrev.com/3466ee18e0f32f37887085e3d06576286b7355b1
Cr-Commit-Position: refs/heads/master@{#374110}

[Adam Rice, 2016-01-06 16:22:26]

Description was changed from

==========
[WIP] Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
==========

to

==========
[WIP] Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
==========

[Adam Rice, 2016-01-13 21:49:06]

Description was changed from

==========
[WIP] Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
==========

to

==========
Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
==========

[Adam Rice, 2016-01-20 06:24:53]

Description was changed from

==========
Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
==========

to

==========
Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

Also add IPC messages WebSocketHostMsg_SendBlob and
WebSocketMsg_BlobSendComplete which expose the new functionality.

The renderer-side functionality is implemented in http://crrev.com/1574213003.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
==========

[Adam Rice, 2016-01-20 06:25:21]

ricea@chromium.org changed reviewers:
+ yhirano@chromium.org

[Adam Rice, 2016-01-20 06:31:00]

Description was changed from

==========
Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

Also add IPC messages WebSocketHostMsg_SendBlob and
WebSocketMsg_BlobSendComplete which expose the new functionality.

The renderer-side functionality is implemented in http://crrev.com/1574213003.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
==========

to

==========
Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

Also add IPC messages WebSocketHostMsg_SendBlob and
WebSocketMsg_BlobSendComplete which expose the new functionality.

The renderer-side functionality is implemented in http://crrev.com/1574213003.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
TEST=content_unittests
==========

[dcheng, 2016-01-20 08:30:05]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-01-20 08:30:06]

Some random comments... I'll finish looking over the IPC handling bits tomorrow
since it's a bit substantial to finish reviewing tonight, sorry.

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.cc (right):

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:83:
CHECK_EQ(STATE_READ_COMPLETE, next_state_);
Would it possible for the renderer to crash the browser by purposely sending the
IPCs out of order?

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.h (right):

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.h:56: virtual ChannelState
SendFrame(bool fin, const std::vector<char>& data) = 0;
bool is_final_frame would be more descriptive.

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.h:94: enum State {
enum class? Then the members can just be named NONE, READ_SIZE, etc.

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
File content/browser/renderer_host/websocket_host.cc (right):

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_host.cc:541:
scoped_ptr<WebSocketBlobSender> blob_sender;
scoped_ptr<WebSocketBlobSender> blob_sender(std::move(blob_sender_)); is another
alternative (I don't have a strong preference over which one is better, but I
think this version might fit on one line =)

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_host.cc:545:
ignore_result(dispatcher_->BlobSendComplete(routing_id_));
Can you help me understand why it's OK to ignore failures here?

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_host.cc:565: routing_id_,
std::string("Failed to load Blob: error code = ") +
Is the explicit string conversion here needed?

[Adam Rice, 2016-01-20 17:11:21]

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.cc (right):

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:83:
CHECK_EQ(STATE_READ_COMPLETE, next_state_);
On 2016/01/20 08:30:06, dcheng wrote:
> Would it possible for the renderer to crash the browser by purposely sending
the
> IPCs out of order?

There are several layers of protection against this. The only method on this
class that the renderer can trigger directly is Start().
WebSocketHost::OnSendBlob always creates a new WebSocketBlobSender instance
before calling Start(), so it can't put this object in an unusual state.

I have modified WebSocketHost to call ReceivedBadMessage() if OnSendBlob is
called when a Blob is already being sent. I also made OnSendFrame call
ReceivedBadMessage() in that case instead of just ignoring it.

This class won't call channel_->SendFrame() until channel_->GetSendQuota()
returns non-zero, which can't happen until the WebSocket is successfully
connected. In addition, calls to SendFrame() before the connection is
established or during the closing handshake are ignored by
net::WebSocketChannel.

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.h (right):

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.h:56: virtual ChannelState
SendFrame(bool fin, const std::vector<char>& data) = 0;
On 2016/01/20 08:30:06, dcheng wrote:
> bool is_final_frame would be more descriptive.

The names were originally based on the terminology in the spec (RFC 6455). At
this layer of abstraction it doesn't matter whether you're familiar with the
spec or not, but then I'd have to figure out where to draw the line where we
start caring about the spec terminology. I'm not sure where that would be.

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.h:94: enum State {
On 2016/01/20 08:30:06, dcheng wrote:
> enum class? Then the members can just be named NONE, READ_SIZE, etc.

Done.

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
File content/browser/renderer_host/websocket_host.cc (right):

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_host.cc:541:
scoped_ptr<WebSocketBlobSender> blob_sender;
On 2016/01/20 08:30:06, dcheng wrote:
> scoped_ptr<WebSocketBlobSender> blob_sender(std::move(blob_sender_)); is
another
> alternative (I don't have a strong preference over which one is better, but I
> think this version might fit on one line =)

Done.

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_host.cc:545:
ignore_result(dispatcher_->BlobSendComplete(routing_id_));
On 2016/01/20 08:30:06, dcheng wrote:
> Can you help me understand why it's OK to ignore failures here?

A failure here would mean the IPC failed to be sent to the renderer, in which
case we will have already
destroyed the channel including this object here.

If we needed to avoid touching member variables when |this| was deleted we could
check for CHANNEL_DELETED here. But in practice we always just return to the
event loop regardless, so there is no point in doing a check.

If you want the really long version, there's actually a design doc:
https://docs.google.com/document/d/1fMxcEFiVj-H5vmuxhPqsxJAl98GS6_yZ-wqB6j9Wm...

ChannelState may be the only enum in Chromium that has its own design doc.
Although it doesn't mention
the enum by name and is almost entirely concerned with ancient problems that no
longer exist since we
added the enum.

https://codereview.chromium.org/1568523002/diff/320001/content/browser/render...
content/browser/renderer_host/websocket_host.cc:565: routing_id_,
std::string("Failed to load Blob: error code = ") +
On 2016/01/20 08:30:06, dcheng wrote:
> Is the explicit string conversion here needed?

No. Removed.

[yhirano, 2016-01-21 12:07:34]

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.cc (right):

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:101: void
WebSocketBlobSender::OnIOComplete(int rv) {
OnReadComplete would be easier to understand.

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:231: // method only reads
as many bytes as are currently needed.
Strictly speaking, |kBufferSize| needs to be less than or equal to UINT64_MAX to
be safe.

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.h (right):

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.h:38: class CONTENT_EXPORT
WebSocketBlobSender {
Can you add a comment that destroying a WebSocketBlobSender cancels all pending
operations in it?

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
File content/browser/renderer_host/websocket_host.cc (right):

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
content/browser/renderer_host/websocket_host.cc:110: if (first_frame_)
You don't need this branch. Just substituting is enough.

[Adam Rice, 2016-01-21 19:24:34]

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.cc (right):

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:101: void
WebSocketBlobSender::OnIOComplete(int rv) {
On 2016/01/21 12:07:34, yhirano wrote:
> OnReadComplete would be easier to understand.

Changed.

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:231: // method only reads
as many bytes as are currently needed.
On 2016/01/21 12:07:34, yhirano wrote:
> Strictly speaking, |kBufferSize| needs to be less than or equal to UINT64_MAX
to
> be safe.

I changed the inner cast to a strict_cast<>. This means that if sizeof(size_t) >
sizeof(uint64_t) it will emit a compile-time error.

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.h (right):

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.h:38: class CONTENT_EXPORT
WebSocketBlobSender {
On 2016/01/21 12:07:34, yhirano wrote:
> Can you add a comment that destroying a WebSocketBlobSender cancels all
pending
> operations in it?

Done.

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
File content/browser/renderer_host/websocket_host.cc (right):

https://codereview.chromium.org/1568523002/diff/360001/content/browser/render...
content/browser/renderer_host/websocket_host.cc:110: if (first_frame_)
On 2016/01/21 12:07:34, yhirano wrote:
> You don't need this branch. Just substituting is enough.

Done.

[yhirano, 2016-01-25 10:52:23]

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.h (right):

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.h:41: class CONTENT_EXPORT
WebSocketBlobSender {
+final

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender_unittest.cc (right):

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender_unittest.cc:250:
fake_channel_->message().size());
Is it better to verify the message contents rather than just size?

std::vector<char> expected_message(message.begin(), message.end());
EXPECT_EQ(expected_message, fake_channel_->message());

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
File content/browser/renderer_host/websocket_host.cc (right):

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
content/browser/renderer_host/websocket_host.cc:14: #include "base/macros.h"
Can you tell me why you stopped including weak_ptr.h?

https://codereview.chromium.org/1568523002/diff/400001/content/common/websock...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/1568523002/diff/400001/content/common/websock...
content/common/websocket_messages.h:161:
IPC_MESSAGE_ROUTED1(WebSocketMsg_FlowControl, int64_t /* quota */)
Should we write something that in blob-sending mode the meaning of this message
changes? The renderer should not increase the quota amount because it is handled
in the browser process.

[Adam Rice, 2016-01-25 23:04:02]

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.h (right):

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.h:41: class CONTENT_EXPORT
WebSocketBlobSender {
On 2016/01/25 10:52:23, yhirano wrote:
> +final

Done.

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender_unittest.cc (right):

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender_unittest.cc:250:
fake_channel_->message().size());
On 2016/01/25 10:52:23, yhirano wrote:
> Is it better to verify the message contents rather than just size?
> 
> std::vector<char> expected_message(message.begin(), message.end());
> EXPECT_EQ(expected_message, fake_channel_->message());

Done.

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
File content/browser/renderer_host/websocket_host.cc (right):

https://codereview.chromium.org/1568523002/diff/400001/content/browser/render...
content/browser/renderer_host/websocket_host.cc:14: #include "base/macros.h"
On 2016/01/25 10:52:23, yhirano wrote:
> Can you tell me why you stopped including weak_ptr.h?

It is included in websocket_host.h.

https://codereview.chromium.org/1568523002/diff/400001/content/common/websock...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/1568523002/diff/400001/content/common/websock...
content/common/websocket_messages.h:161:
IPC_MESSAGE_ROUTED1(WebSocketMsg_FlowControl, int64_t /* quota */)
On 2016/01/25 10:52:23, yhirano wrote:
> Should we write something that in blob-sending mode the meaning of this
message
> changes? The renderer should not increase the quota amount because it is
handled
> in the browser process.

I added some new text. Please take a look and see if it makes sense.

[Adam Rice, 2016-01-26 00:20:16]

dcheng: Is there anything I can do to help your analysis?

For what it's worth, here's my security analysis of the new IPCs:

WebSocketHostMsg_SendBlob: Three potential attacks:
1. Enumerate existing blobs. If this attack was feasible and useful there are
existing IPCs that an attacker could use to perform it.
2. Exploit vulnerabilities in WebSocketBlobSender by constructing a pathological
Blob. Since Blobs can be constructed recursively from a number of types, and can
include multiple references to the same file or patch of memory they can be
arbitrarily weird, but all that weirdness should be abstracted by
storage::BlobReader.

I am interested in whether we can overflow uint64_t. The answer appears to be
"maybe". I created a 256MB ArrayBuffer, then made a 1GB Blob from that, and
repeatedly did blob = new Blob([blob, blob]) until the browser became unhappy.
blob.size reached 9223372036854776000 (0x80000000000000c0) bytes and then
started returning 0. The 0xc0 is probably a rounding error from conversion to
floating point.

So we can create a Blob which claims to be size 0 but is actually huge.
WebSocketBlobSender will treat this as an empty Blob so I don't think it's an
issue for my code. I can add a unit test for this if you want me to.

We can probably also create a Blob which claims be a sensible size but is
actually much bigger. WebSocketBlobSender will only send the claimed size, not
the "real" size, so that shouldn't be a problem either.

Attack 3. DoS a remote server by sending data really quickly. This is probably
the most likely attack to be performed in practice, but it's not a browser
vulnerability as such. We never promised that our WebSocket implementation would
be slow.


IPC WebSocketMsg_BlobSendComplete. Only handled by the render process, not the
browser process. Contains only a routing ID, no other data. Worst-case scenario,
it could form one stage in a multi-stage privilege escalation. I just added code
to crrev.com/1574213003 to ignore it if unexpected.

[yhirano, 2016-01-26 01:13:37]

lgtm

[dcheng, 2016-01-26 22:09:32]

At a high-level, I think this change is probably fine.

My two concerns are around:
- lots of casting between int / uint64_t / size_t. It looks like part of this is
because various things in //net use int (like
WebSocketChannel::current_send_quota_. Can we change those to reduce the amount
of casting needed?

- there's a mix of DCHECK / CHECK for checking what appear to be preconditions
entirely controlled in the browser process. can we be consistent about this?

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.cc (right):

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:102:
CHECK_EQ(State::READ_COMPLETE, next_state_);
DCHECK_EQ here, line 108, and line 115 (as far as I can tell, this is all driven
by state changes wholly inside the browser process)

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:226: uint64_t quota =
base::checked_cast<uint64_t>(channel_->GetSendQuota());
Ultimately, where does the send quota come from? Is it really OK to kill the
browser process?

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:264: uint64_t bytes_read
= base::checked_cast<uint64_t>(result);
Similar comment to above: it seems like this is the network stack doing this
work.

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:265:
CHECK_GE(bytes_left_, bytes_read);
Similarly, I would expect this to be an assert: it should "never" happen.

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.h (right):

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.h:53: virtual int
GetSendQuota() const = 0;
Can this actually be negative? I'm guessing this is an int because
WebSocketChannel::current_sent_quota_ is ultimately an int too, but it's not
clear to me why that's an int either.

It seems like using uint64_t here would avoid some casts.

[Adam Rice, 2016-01-27 01:52:57]

I have filed a bug crbug.com/581567 to use size_t for quota everywhere, since
it's larger in scope than this CL. I've also changed the code in this CL to use
size_t for quota.

I was using DCHECK() to mean "this can't happen" and CHECK() to mean "this can't
happen, but if somehow it does, you're going to have a very bad time". That
usage was non-standard and I have fixed it.

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.cc (right):

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:102:
CHECK_EQ(State::READ_COMPLETE, next_state_);
On 2016/01/26 22:09:32, dcheng wrote:
> DCHECK_EQ here, line 108, and line 115 (as far as I can tell, this is all
driven
> by state changes wholly inside the browser process)

Done.

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:226: uint64_t quota =
base::checked_cast<uint64_t>(channel_->GetSendQuota());
On 2016/01/26 22:09:32, dcheng wrote:
> Ultimately, where does the send quota come from? Is it really OK to kill the
> browser process?

It comes from net::WebSocketChannel. It's OK in the sense that if this value is
negative you have memory corruption and we can't recover from it. Having said
that, I removed the checked_casts.

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:264: uint64_t bytes_read
= base::checked_cast<uint64_t>(result);
On 2016/01/26 22:09:32, dcheng wrote:
> Similar comment to above: it seems like this is the network stack doing this
> work.

I would expect this checked_cast<> to be optimised out because of the if (result
< 0) expression above. static_cast<> makes me uncomfortable.

Having said that, I changed it.

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:265:
CHECK_GE(bytes_left_, bytes_read);
On 2016/01/26 22:09:32, dcheng wrote:
> Similarly, I would expect this to be an assert: it should "never" happen.

Done.

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.h (right):

https://codereview.chromium.org/1568523002/diff/420001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.h:53: virtual int
GetSendQuota() const = 0;
On 2016/01/26 22:09:32, dcheng wrote:
> Can this actually be negative? I'm guessing this is an int because
> WebSocketChannel::current_sent_quota_ is ultimately an int too, but it's not
> clear to me why that's an int either.
> 
> It seems like using uint64_t here would avoid some casts.

I changed it to size_t, since that is the direction I want to go in. An amount
of quota larger than the available memory would not make sense.

[Adam Rice, 2016-01-29 07:47:30]

dcheng: ping. Does it look okay now?

[dcheng, 2016-01-29 08:23:53]

ipc changes lgtm

If there are places where you'd prefer to have a checked_cast over static_cast,
I guess that's fine. It's not a huge cost. Hopefully we can try to make things
more consistent over time to reduce the amount of casting between integer types
though.

[Adam Rice, 2016-01-29 10:34:10]

ricea@chromium.org changed reviewers:
+ dmurph@chromium.org

[Adam Rice, 2016-01-29 10:34:11]

+dmurph, do you have time to check the use of storage::BlobReader in this CL? I
am particularly interested in whether the code that gets the BlobStorageContext
and StoragePartition in render_process_host_impl.cc, stores it in
websocket_dispatcher_host.h, and uses it to create the BlobReader in
WebSocketHost::OnSendBlob and WebSocketBlobSender::Start looks okay.

But if you could also take a look at the call to BlobReader::Read() in
WebSocketBlobSender::DoRead() I would appreciate it. Is the assumption that
BlobReader::Read() will only return 0 bytes at end-of-file okay?

[dmurph, 2016-02-03 01:35:36]

publishing early comments because I have to go.

Yes, the reader should populate a read length of 0 when it's done (as well as
the DONE enum).

https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:964:
resource_message_filter->blob_storage_context(),
Please use ChromeBlobStorageContext::GetFor(browser_context) instead. Or, you
can call that method once above, and use it for all other people that grab it.

https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.cc (right):

https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:98: return
reader_->total_size();
Note: this is only populated after CalculateSize is called on the reader. It
looks like you have that documented, but could you maybe add a DCHECK in the
total_size() method of the BlobReader? I'm hoping this doesn't break any tests.

DCHECK(total_size_calculated_);

[dmurph, 2016-02-03 01:43:38]

ALSO: you cannot "reuse" the blob reader. Make sure that you class doesn't
expect this, it caused a bug earlier with another client. Instead just
re-construct it.

On Tue, Feb 2, 2016, 5:35 PM  <dmurph@chromium.org> wrote:

> publishing early comments because I have to go.
>
> Yes, the reader should populate a read length of 0 when it's done (as well as
> the DONE enum).
>
>
https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
>
https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
> content/browser/renderer_host/render_process_host_impl.cc:964
<https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...>:
> resource_message_filter->blob_storage_context(),
> Please use ChromeBlobStorageContext::GetFor(browser_context) instead.
> Or, you can call that method once above, and use it for all other people
> that grab it.
>
https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
> File content/browser/renderer_host/websocket_blob_sender.cc (right):
>
https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
> content/browser/renderer_host/websocket_blob_sender.cc:98
<https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...>:
return
> reader_->total_size();
> Note: this is only populated after CalculateSize is called on the
> reader. It looks like you have that documented, but could you maybe add
> a DCHECK in the total_size() method of the BlobReader? I'm hoping this
> doesn't break any tests.
>
> DCHECK(total_size_calculated_);
> https://codereview.chromium.org/1568523002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Adam Rice, 2016-02-03 03:04:46]

https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:964:
resource_message_filter->blob_storage_context(),
On 2016/02/03 01:35:36, dmurph wrote:
> Please use ChromeBlobStorageContext::GetFor(browser_context) instead. Or, you
> can call that method once above, and use it for all other people that grab it.

Thanks! That's exactly the sort of information I was hoping for. I have saved it
into a local variable to avoid calling GetFor() repeatedly.

https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
File content/browser/renderer_host/websocket_blob_sender.cc (right):

https://codereview.chromium.org/1568523002/diff/480001/content/browser/render...
content/browser/renderer_host/websocket_blob_sender.cc:98: return
reader_->total_size();
On 2016/02/03 01:35:36, dmurph wrote:
> Note: this is only populated after CalculateSize is called on the reader. It
> looks like you have that documented, but could you maybe add a DCHECK in the
> total_size() method of the BlobReader? I'm hoping this doesn't break any
tests.
> 
> DCHECK(total_size_calculated_);

I will add this in a separate CL. Expect it soon.

[Adam Rice, 2016-02-03 03:13:18]

On 2016/02/03 01:43:38, dmurph wrote:
> ALSO: you cannot "reuse" the blob reader. Make sure that you class doesn't
> expect this, it caused a bug earlier with another client. Instead just
> re-construct it.

The object itself only ever reads one Blob before being destroyed, so it
shouldn't
be a problem. Just to be on the safe side, I have added DCHECK(!reader_) to the
Start() method.

[dmurph, 2016-02-03 19:30:04]

BlobReader stuff lgtm

https://codereview.chromium.org/1568523002/diff/540001/content/browser/render...
File content/browser/renderer_host/websocket_dispatcher_host.cc (right):

https://codereview.chromium.org/1568523002/diff/540001/content/browser/render...
content/browser/renderer_host/websocket_dispatcher_host.cc:142: return
blob_storage_context_->context();
Just FYI, this isn't immediately populated (look at the GetFor method in
BlobStorageContext), it schedules a task on the IO thread to initialize the
context. This usually isn't a problem, as you're only supposed to interact w/
the blobstoragecontext on the IO thread, but just letting you know this could be
a source of a subtle but in testing code.

[Adam Rice, 2016-02-04 05:48:36]

https://codereview.chromium.org/1568523002/diff/540001/content/browser/render...
File content/browser/renderer_host/websocket_dispatcher_host.cc (right):

https://codereview.chromium.org/1568523002/diff/540001/content/browser/render...
content/browser/renderer_host/websocket_dispatcher_host.cc:142: return
blob_storage_context_->context();
On 2016/02/03 19:30:04, dmurph wrote:
> Just FYI, this isn't immediately populated (look at the GetFor method in
> BlobStorageContext), it schedules a task on the IO thread to initialize the
> context. This usually isn't a problem, as you're only supposed to interact w/
> the blobstoragecontext on the IO thread, but just letting you know this could
be
> a source of a subtle but in testing code.

I already hit this twice. It wasn't too subtle.

[Adam Rice, 2016-02-04 05:50:46]

ricea@chromium.org changed reviewers:
+ asvitkine@google.com

[Adam Rice, 2016-02-04 05:50:47]

+asvitkine for histograms.xml. It's just an addition of two BadMessageReasons.
See bad_message.h for the change.

[Alexei Svitkine (slow), 2016-02-04 16:20:20]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-02-04 16:20:21]

lgtm

[kinuko, 2016-02-05 05:18:05]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-02-05 05:18:07]

https://codereview.chromium.org/1568523002/diff/540001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1568523002/diff/540001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:876:
ChromeBlobStorageContext* chrome_blob_storage_context =
nit: this class name is very unfortunate (should be changed) as we're in
content/ not chrome/.  Can we at least not use 'chrome_' prefix on the variable
name?

Also: maybe we should store this as scoped_refptr<>

[Adam Rice, 2016-02-05 07:21:48]

https://codereview.chromium.org/1568523002/diff/540001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1568523002/diff/540001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:876:
ChromeBlobStorageContext* chrome_blob_storage_context =
On 2016/02/05 05:18:06, kinuko wrote:
> nit: this class name is very unfortunate (should be changed) as we're in
> content/ not chrome/.  Can we at least not use 'chrome_' prefix on the
variable
> name?
> 
> Also: maybe we should store this as scoped_refptr<>

I have been trying to think of a better name for ChromeBlobStorageContext.
Maybe BrowserBlobStorageContext?

I removed chrome_ from the name of the variable and
made it a scoped_refptr.

[Adam Rice, 2016-02-08 01:13:00]

kinuko: This CL no longer applies cleanly against ToT. Would you like me to
rebase now, or after you have finished your review?

[kinuko, 2016-02-08 01:53:43]

kinuko@chromium.org changed reviewers:
- asvitkine@google.com

[kinuko, 2016-02-08 01:53:46]

Either works for me. lgtm anyways

[Adam Rice, 2016-02-08 08:58:47]

The CQ bit was checked by ricea@chromium.org

[Adam Rice, 2016-02-08 08:58:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, dcheng@chromium.org, dmurph@chromium.org,
asvitkine@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1568523002/#ps580001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-02-08 08:59:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568523002/580001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568523002/580001

[commit-bot: I haz the power, 2016-02-08 12:06:17]

Description was changed from

==========
Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

Also add IPC messages WebSocketHostMsg_SendBlob and
WebSocketMsg_BlobSendComplete which expose the new functionality.

The renderer-side functionality is implemented in http://crrev.com/1574213003.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
TEST=content_unittests
==========

to

==========
Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

Also add IPC messages WebSocketHostMsg_SendBlob and
WebSocketMsg_BlobSendComplete which expose the new functionality.

The renderer-side functionality is implemented in http://crrev.com/1574213003.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
TEST=content_unittests
==========

[commit-bot: I haz the power, 2016-02-08 12:06:18]

Committed patchset #30 (id:580001)

[commit-bot: I haz the power, 2016-02-08 12:07:25]

Description was changed from

==========
Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

Also add IPC messages WebSocketHostMsg_SendBlob and
WebSocketMsg_BlobSendComplete which expose the new functionality.

The renderer-side functionality is implemented in http://crrev.com/1574213003.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
TEST=content_unittests
==========

to

==========
Implement content::WebSocketBlobSender

Add the class WebSocketBlobSender to send Blobs over WebSockets from
within the browser process.

This CL also includes the necessary changes to WebSocketDispatcherHost
and WebSocketHost to use the new class.

Also add IPC messages WebSocketHostMsg_SendBlob and
WebSocketMsg_BlobSendComplete which expose the new functionality.

The renderer-side functionality is implemented in http://crrev.com/1574213003.

See design doc at
https://docs.google.com/document/d/1CDiXB9pBumhFVVfmIn1CRI6v6byxyqWu2urEE9xp7...

BUG=571656
TEST=content_unittests

Committed: https://crrev.com/3466ee18e0f32f37887085e3d06576286b7355b1
Cr-Commit-Position: refs/heads/master@{#374110}
==========

[commit-bot: I haz the power, 2016-02-08 12:07:26]

Patchset 30 (id:??) landed as
https://crrev.com/3466ee18e0f32f37887085e3d06576286b7355b1
Cr-Commit-Position: refs/heads/master@{#374110}

[Nico, 2016-02-08 19:03:54]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-02-08 19:03:55]

https://codereview.chromium.org/1568523002/diff/580001/content/common/websock...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/1568523002/diff/580001/content/common/websock...
content/common/websocket_messages.h:134:
IPC_MESSAGE_ROUTED0(WebSocketMsg_BlobSendComplete);
please remove this trailing semicolon:

..\..\content/common/websocket_messages.h(134,51) :  error: expected expression
IPC_MESSAGE_ROUTED0(WebSocketMsg_BlobSendComplete);
                                                  ^
..\..\tools\ipc_fuzzer\message_tools\message_list.cc(39,15) :  error: invalid
application of 'sizeof' to an incomplete type 'msginfo []'
static_assert(MSGTABLE_SIZE, "check your headers for an extra semicolon");
              ^~~~~~~~~~~~~


https://build.chromium.org/p/chromium.fyi/builders/CrWinAsan/builds/2840/step...

[Adam Rice, 2016-02-09 06:18:58]

https://codereview.chromium.org/1568523002/diff/580001/content/common/websock...
File content/common/websocket_messages.h (right):

https://codereview.chromium.org/1568523002/diff/580001/content/common/websock...
content/common/websocket_messages.h:134:
IPC_MESSAGE_ROUTED0(WebSocketMsg_BlobSendComplete);
On 2016/02/08 at 19:03:55, Nico wrote:
> please remove this trailing semicolon:

Sorry about that. This was fixed by mbarbella in
https://codereview.chromium.org/1680753002/