[DevTools] Respond with a close frame

[DevTools] Respond with a close frame

WS spec section 5.5.1:

If an endpoint receives a Close frame and did not previously send a
Close frame, the endpoint MUST send a Close frame in response.  (When
sending a Close frame in response, the endpoint typically echos the
status code it received.)  It SHOULD do so as soon as practical.  An
endpoint MAY delay sending a Close frame until its current message is
sent (for instance, if the majority of a fragmented message is
already sent, an endpoint MAY send the remaining fragments before
sending a Close frame).  However, there is no guarantee that the
endpoint that has already sent a Close frame will continue to process
data.

After both sending and receiving a Close message, an endpoint
considers the WebSocket connection closed and MUST close the
underlying TCP connection.  The server MUST close the underlying TCP
connection immediately; the client SHOULD wait for the server to
close the connection but MAY close the connection at any time after
sending and receiving a Close message, e.g., if it has not received a
TCP Close from the server in a reasonable time period.

BUG=718619
Review-Url: https://codereview.chromium.org/2871573011
Cr-Commit-Position: refs/heads/master@{#470649}
Committed: https://chromium.googlesource.com/chromium/src/+/22b6704f77638e8621311aa2c11e6ea1778885bf

[eostroukhov, 2017-05-09 22:00:28]

eostroukhov@chromium.org changed reviewers:
+ caseq@chromium.org, dgozman@chromium.org

[dgozman, 2017-05-09 23:00:38]

Is it possible to add a test?

https://codereview.chromium.org/2871573011/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/2871573011/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/android_web_socket.cc:66: if
(request_buffer_.length() == encoded_frame.length())
Let's add SendData(const std::string& data) which will do this check and reuse
it here and for close response.

[eostroukhov, 2017-05-09 23:50:52]

The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 23:52:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov, 2017-05-09 23:53:14]

Unfortunately, looks like adding a test would be a significant investment of
time - it looks like it is fairly hard to mock the required parts,
AndroidWebSocket is buried fairly deep being a private field of the private
field.

https://codereview.chromium.org/2871573011/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/2871573011/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/device/android_web_socket.cc:66: if
(request_buffer_.length() == encoded_frame.length())
On 2017/05/09 23:00:38, dgozman wrote:
> Let's add SendData(const std::string& data) which will do this check and reuse
> it here and for close response.

Done.

[commit-bot: I haz the power, 2017-05-10 01:55:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 01:55:10]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[eostroukhov, 2017-05-10 16:28:48]

The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 16:29:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgozman, 2017-05-10 16:55:26]

lgtm

[commit-bot: I haz the power, 2017-05-10 17:14:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 17:14:54]

Dry run: This issue passed the CQ dry run.

[eostroukhov, 2017-05-10 18:10:23]

The CQ bit was checked by eostroukhov@chromium.org

[commit-bot: I haz the power, 2017-05-10 18:11:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 18:26:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494439823625140,
"parent_rev": "77346d2ec5bd54cfcb9cdebe97127b725af60db5", "commit_rev":
"22b6704f77638e8621311aa2c11e6ea1778885bf"}

[commit-bot: I haz the power, 2017-05-10 18:26:36]

Description was changed from

==========
[DevTools] Respond with a close frame

WS spec section 5.5.1:

If an endpoint receives a Close frame and did not previously send a
Close frame, the endpoint MUST send a Close frame in response.  (When
sending a Close frame in response, the endpoint typically echos the
status code it received.)  It SHOULD do so as soon as practical.  An
endpoint MAY delay sending a Close frame until its current message is
sent (for instance, if the majority of a fragmented message is
already sent, an endpoint MAY send the remaining fragments before
sending a Close frame).  However, there is no guarantee that the
endpoint that has already sent a Close frame will continue to process
data.

After both sending and receiving a Close message, an endpoint
considers the WebSocket connection closed and MUST close the
underlying TCP connection.  The server MUST close the underlying TCP
connection immediately; the client SHOULD wait for the server to
close the connection but MAY close the connection at any time after
sending and receiving a Close message, e.g., if it has not received a
TCP Close from the server in a reasonable time period.

BUG=718619
==========

to

==========
[DevTools] Respond with a close frame

WS spec section 5.5.1:

If an endpoint receives a Close frame and did not previously send a
Close frame, the endpoint MUST send a Close frame in response.  (When
sending a Close frame in response, the endpoint typically echos the
status code it received.)  It SHOULD do so as soon as practical.  An
endpoint MAY delay sending a Close frame until its current message is
sent (for instance, if the majority of a fragmented message is
already sent, an endpoint MAY send the remaining fragments before
sending a Close frame).  However, there is no guarantee that the
endpoint that has already sent a Close frame will continue to process
data.

After both sending and receiving a Close message, an endpoint
considers the WebSocket connection closed and MUST close the
underlying TCP connection.  The server MUST close the underlying TCP
connection immediately; the client SHOULD wait for the server to
close the connection but MAY close the connection at any time after
sending and receiving a Close message, e.g., if it has not received a
TCP Close from the server in a reasonable time period.

BUG=718619

Review-Url: https://codereview.chromium.org/2871573011
Cr-Commit-Position: refs/heads/master@{#470649}
Committed:
https://chromium.googlesource.com/chromium/src/+/22b6704f77638e8621311aa2c11e...
==========

[commit-bot: I haz the power, 2017-05-10 18:26:37]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/22b6704f77638e8621311aa2c11e...