Android device manager should not drop WebSocket frame data even if it arrives together with handsh…

Android device manager should not drop WebSocket frame data even if it arrives together with handshake response

R=dgozman
BUG=475497
Committed: https://crrev.com/67874ebf6052345c1d0bd9a3e0c5bbcd363999b9
Cr-Commit-Position: refs/heads/master@{#325216}

[tyoshino (SeeGerritForStatus), 2015-04-09 12:19:10]

tyoshino@chromium.org changed reviewers:
+ dgozman@chromium.org

[tyoshino (SeeGerritForStatus), 2015-04-09 12:19:10]

This CL contains some renaming, removal of closure arguments. Please tell me if
any of them look not reasonable.

I'd like to get your advice how to test this. Maybe port forwarding browser test
is not sufficient? Not sure.

[tyoshino (SeeGerritForStatus), 2015-04-09 12:23:21]

Created bug entry: http://crbug.com/475497

[dgozman, 2015-04-09 12:47:44]

dgozman@chromium.org changed reviewers:
+ vkuzkokov@chromium.org

[dgozman, 2015-04-09 12:47:45]

Interesting problem. I guess we should not read from the same socket in
different classes (and pass socket around).
Can we refactor the code in such a way that WebSocket works with the net::Socket
exclusively? This will leave HttpRequest with CommandCallback only, and move
http upgrade logic to WebSocket. Vladislav, WDYT?

[tyoshino (SeeGerritForStatus), 2015-04-09 13:31:53]

ps1 -> ps2: Merged good points from horo@'s CL
(https://codereview.chromium.org/1071843003).

> Can we refactor the code in such a way that WebSocket works with the
net::Socket
exclusively?

If you plan to reuse some net/ material to parse the header part in the future,
it might require similar code.

In the normal WebSocket code, we are doing similar. See that
WebSocketBasicStream takes state_.read_buf().
https://code.google.com/p/chromium/codesearch#chromium/src/net/websockets/web...
It's handled here:
https://code.google.com/p/chromium/codesearch#chromium/src/net/websockets/web...

[dgozman, 2015-04-09 14:05:23]

Thank you for fixing this!

> If you plan to reuse some net/ material to parse the header part in the
future, it might require similar code.
Alright, if that's a common net/ practice, we should follow.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_device_manager.cc (right):

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.cc:215: result, "", "",
make_scoped_ptr<net::StreamSocket>(nullptr));
nit: "" -> std::string()

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.cc:233: int
response_total_size_;
Consider renaming to |expected_size_| to emphasize that we wait for this number
of bytes, not that it's exactly the response size.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.cc:235: size_t
response_header_size_;
I'd just call this |header_size_|, since this class does nothing except for
response handling.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_device_manager.h (right):

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.h:26: typedef
base::Callback<void(
nit: while you are here, please move from "typedef" to "using".

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.h:87: // |body_head|
should contain the body (WebSocket frame data) part that has
I think this comment belongs to |HttpUpgradeCallback| definition above.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:74: void
ProcessResponseBuffer(scoped_refptr<net::IOBuffer> response_io_buffer) {
I'd rename this to |io_buffer| to better differentiate from |response_buffer|.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:97: void
OnBytesRead(scoped_refptr<net::IOBuffer> response_io_buffer,
nit: mind moving this code back for cleaner diff?

[tyoshino (SeeGerritForStatus), 2015-04-14 12:24:33]

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_device_manager.cc (right):

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.cc:215: result, "", "",
make_scoped_ptr<net::StreamSocket>(nullptr));
On 2015/04/09 14:05:22, dgozman wrote:
> nit: "" -> std::string()

Done.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.cc:233: int
response_total_size_;
On 2015/04/09 14:05:22, dgozman wrote:
> Consider renaming to |expected_size_| to emphasize that we wait for this
number
> of bytes, not that it's exactly the response size.

Done.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.cc:235: size_t
response_header_size_;
On 2015/04/09 14:05:22, dgozman wrote:
> I'd just call this |header_size_|, since this class does nothing except for
> response handling.

Done.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_device_manager.h (right):

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.h:26: typedef
base::Callback<void(
On 2015/04/09 14:05:22, dgozman wrote:
> nit: while you are here, please move from "typedef" to "using".

Done.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.h:87: // |body_head|
should contain the body (WebSocket frame data) part that has
On 2015/04/09 14:05:22, dgozman wrote:
> I think this comment belongs to |HttpUpgradeCallback| definition above.

Done.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:74: void
ProcessResponseBuffer(scoped_refptr<net::IOBuffer> response_io_buffer) {
On 2015/04/09 14:05:22, dgozman wrote:
> I'd rename this to |io_buffer| to better differentiate from |response_buffer|.

Done.

https://codereview.chromium.org/1078733002/diff/20001/chrome/browser/devtools...
chrome/browser/devtools/device/android_web_socket.cc:97: void
OnBytesRead(scoped_refptr<net::IOBuffer> response_io_buffer,
On 2015/04/09 14:05:22, dgozman wrote:
> nit: mind moving this code back for cleaner diff?

Done.

[dgozman, 2015-04-14 15:48:50]

lgtm

[tyoshino (SeeGerritForStatus), 2015-04-15 07:26:07]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2015-04-15 07:26:07]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1078733002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-15 07:26:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1078733002/80001

[commit-bot: I haz the power, 2015-04-15 10:11:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-15 10:12:20]

Patchset 5 (id:??) landed as
https://crrev.com/67874ebf6052345c1d0bd9a3e0c5bbcd363999b9
Cr-Commit-Position: refs/heads/master@{#325216}